5.1 unbuffered queries #17649

ADmad · 2024-04-01T17:36:45Z

I have been playing around to add back support for unbuffered queries and have come up with a POC. The implementation is a bit different than earlier since we got rid of statement decoration in 5.0.

Digging into the topic I learned that at PDO level postgres is always buffered, sqlite is always unbuffered, only mysql has the option to toggle buffered/unbuffered at connection level.

Here are a few noteworthy points about the implementation:

At the ORM level for non joined association a separate query is made to fetch related records of each primary record. So for Posts hasMany Comments if a query returns 2 posts then there will be one query each to fetch related Comments unlike for buffered results where all related comments are fetched using a single query.
In unbuffered mode the Statement instance is iterated which returns a generator. Since generators can't be rewind, trying to iterate the resultset again or using any collection method which internally iterates will throw an exception. Personally I think that's a reasonable limitation. An alternative would be to try and transparently re-execute the query.
Trying to make Statement::getIterator() directly return the PDOStatment instance failed as the results are later wrapped in collection for decoration and Collection::unwrap() has return type Iterator instead of Traversable.
For simplicity I have made ResultSet simply extend Collection.

TODO:

Fix the one failing test
Lot of additional tests required

Unbufferred results are only available when directly iterating the SelectQuery instance. Calling SelectQuery::all() with always returned a buffered result set.

src/Database/Query/SelectQuery.php

othercorey · 2024-04-02T02:45:48Z

src/Database/Statement/Statement.php

+        }
+
+        $this->closeCursor();
+    }


I'm not sure I see how this and fetchAll() should co-exist. Won't the statement be empty if we call one or the other?

One needs to pick one of the options, either call fetchAll() or iterate the instance.

ADmad · 2024-04-02T07:15:57Z

Hmm.. an ORM test which uses unbuffered queries and contains a hasMany assoc. (which cause a new query) fails on mysql with

SQLSTATE[HY000]: General error: 2014 Cannot execute queries while other unbuffered queries are active.  Consider using PDOStatement::fetchAll().  Alternatively, if your code is only ever going to run against mysql, you may enable query buffering by setting the PDO::MYSQL_ATTR_USE_BUFFERED_QUERY attribute.

I tried the same on 4.x and there's no error. Trying to walk through the 4.x code I haven't been able to figure out why the same error doesn't occur on 4.x.

src/Database/Query/SelectQuery.php

src/ORM/EagerLoader.php

markstory · 2024-04-06T03:43:31Z

src/ORM/EagerLoader.php

+        $results = (new Collection($results))
+            ->map(function ($row) use ($query, $external, $assocData) {


This is an interesting solution.

src/ORM/EagerLoader.php

src/ORM/ResultSetFactory.php

ADmad · 2024-04-07T14:43:18Z

I tried the same on 4.x and there's no error. Trying to walk through the 4.x code I haven't been able to figure out why the same error doesn't occur on 4.x.

I figured out why the error doesn't occur on 4.x. Even if you have bufferring turned off, if you contain hasMany/belongsToMany associations the eagerloader will implicitly start buffering by using the BufferedStatement.

cakephp/src/ORM/EagerLoader.php

Line 809 in fe40e4f

if (!($statement instanceof BufferedStatement)) {

Since new queries can't be executed for the same connection for unbuffered MySQL queries, containing externally loaded associations will implicitly buffer results.

othercorey · 2024-04-07T18:31:16Z

I figured out why the error doesn't occur on 4.x. Even if you have bufferring turned off, if you contain hasMany/belongsToMany associations the eagerloader will implicitly start buffering by using the BufferedStatement.

That sounds right.

Fix errors reported by static analysis

ADmad · 2024-04-08T05:38:33Z

This is ready for another round of review.

If associations which require additional queries are contained then even for unbuffered queries all results will be pre-fetched since MySQL doesn't allow running new queries when an unbuffered query is already active.

I'll work on adding more tests after this.

ADmad · 2024-04-09T16:35:42Z

@nicosp Can you please try out this branch and provide feedback whether it restores support for your use case?

nicosp · 2024-04-09T18:07:59Z

@nicosp Can you please try out this branch and provide feedback whether it restores support for your use case?

Sure. I will try it in a couple of days. Thank you for attempting to fix this

othercorey · 2024-04-11T06:52:06Z

src/Database/Driver/Mysql.php

+
+        if ($query instanceof SelectQuery) {
+            try {
+                $this->getPdo()->setAttribute(PDO::MYSQL_ATTR_USE_BUFFERED_QUERY, $query->isBufferedResultsEnabled());


I believe we did the the same in 4.x, so this should be ok. I'm wondering if setting the attribute per query is necessary or affects performance at all

Yes it's the same as 4.x, no idea about the performance implications.

Lets not overcomplicate things. If this worked in 4.x it will be good enough here too. If it creates a performance regression we can patch around it.

othercorey · 2024-04-11T06:56:51Z

src/ORM/ResultSet.php

@@ -30,184 +28,8 @@
 * @template T of \Cake\Datasource\EntityInterface|array
 * @implements \Cake\Datasource\ResultSetInterface<T>
 */
-class ResultSet implements ResultSetInterface
+class ResultSet extends Collection implements ResultSetInterface


I approve of extending Collection directly here. I'm interested in making Collection extensible and more customizable with collection iterators instead of CollectionInterface.

Not sure what ResultSetInterface means in the future, but I think this is ok for 5.1.

Not sure what ResultSetInterface means in the future, but I think this is ok for 5.1.

ResultSetInterface doesn't really hold much meaning. Maybe in the future we can just drop it and make ORM\Query\SelectQuery:all()'s return type as Collection/CollectionInterface.

othercorey · 2024-04-11T07:05:11Z

src/ORM/ResultSetFactory.php

+            $results = (new Collection($results))
+                ->map(function ($row) use ($data) {
+                    return $this->groupResult($row, $data);
+                });


Hopefully users don't iterator over the result set multiple times. Should we unwrap this before passing to ResultSet()?

Is the non-array path the unbuffered statement so we can't unwrap this? Should we check it's a statement?

Yes a non array path means it's an buffered statement. But since the Collection can handle any iterable, I don't want to narrow this down unnecessarily using a Statement check.

othercorey · 2024-04-11T07:18:11Z

src/Database/Query/SelectQuery.php

@@ -111,13 +119,6 @@ public function all(): iterable
    {
        if ($this->_results === null || $this->_dirty) {
            $this->_results = $this->execute()->fetchAll(StatementInterface::FETCH_TYPE_ASSOC);
-            if ($this->_resultDecorators) {


Moving this to the statement seems good.

Yes, Statement was already decorating the records using the typemap so moving user provided decorators there too makes sense, so all decorations are now in one place.

nicosp · 2024-04-15T12:40:09Z

@nicosp Can you please try out this branch and provide feedback whether it restores support for your use case?

With light testing on a preminary 5.x branch it's working for me. Let me know if there is anything more I can do to help

markstory

Nice work 👏

markstory · 2024-04-19T03:04:19Z

src/Database/Driver/Mysql.php

+
+        if ($query instanceof SelectQuery) {
+            try {
+                $this->getPdo()->setAttribute(PDO::MYSQL_ATTR_USE_BUFFERED_QUERY, $query->isBufferedResultsEnabled());


Lets not overcomplicate things. If this worked in 4.x it will be good enough here too. If it creates a performance regression we can patch around it.

src/Database/Query/SelectQuery.php

Co-authored-by: Mark Story <mark@mark-story.com>

ADmad · 2024-04-26T11:54:44Z

I was hoping to add some more tests but have been quite busy and won't be able to get to it anytime soon.

So if we are happy with the changes this can be merged.

othercorey · 2024-04-26T12:11:20Z

I was hoping to add some more tests but have been quite busy and won't be able to get to it anytime soon.

So if we are happy with the changes this can be merged.

Do you have an idea what kind of tests you wanted?

ADmad · 2024-04-26T12:35:56Z

Do you have an idea what kind of tests you wanted?

Haven't got around to even that yet :). It's just that there's currently only 1 ORM test with results buffering turned off. Would be nice a few tests at DBAL and ORM levels to check that actually only 1 record is fetched at a time with buffering disabled.

Also have ORM tests which fetch hasmany/belongstomany associations with buffering turned off, to show we get the expected results (albeit with buffering implicitly turned off).

othercorey · 2024-04-26T18:18:24Z

Let's leave the PR open for a bit to see if one of us finds time to confirm no issues.

ADmad added 4 commits April 1, 2024 22:48

Move all results decoration inside Statement.

f691d99

Make Statement iterable.

f661b8f

Add support for unbuffered select queries.

cfeca32

Unbufferred results are only available when directly iterating the SelectQuery instance. Calling SelectQuery::all() with always returned a buffered result set.

Add supported for unbuffered ORM resultsets.

7b84af7

ADmad added this to the 5.1.0 milestone Apr 1, 2024

Fix test

47db92a

othercorey reviewed Apr 2, 2024

View reviewed changes

src/Database/Query/SelectQuery.php Outdated Show resolved Hide resolved

othercorey reviewed Apr 2, 2024

View reviewed changes

src/Database/Query/SelectQuery.php Show resolved Hide resolved

othercorey reviewed Apr 2, 2024

View reviewed changes

Fix test

765615d

markstory reviewed Apr 6, 2024

View reviewed changes

Fix loading of external associations for unbuffered queries.

c22706c

Since new queries can't be executed for the same connection for unbuffered MySQL queries, containing externally loaded associations will implicitly buffer results.

ADmad added 2 commits April 8, 2024 00:07

Rename property

6032873

Fix Statement instance creation in Sqlserver driver.

a5f17eb

ADmad force-pushed the 5.1-unbuffered-queries branch from f76adfa to 2d1d1af Compare April 7, 2024 18:40

Update StatementInterface.

de5c56b

Fix errors reported by static analysis

ADmad force-pushed the 5.1-unbuffered-queries branch from 2d1d1af to de5c56b Compare April 7, 2024 18:51

ADmad added 2 commits April 8, 2024 00:32

Fix test

ee026f9

Optimize ResultSet creation

6b10a6c

othercorey reviewed Apr 11, 2024

View reviewed changes

markstory approved these changes Apr 21, 2024

View reviewed changes

Fix description

5fec99a

Co-authored-by: Mark Story <mark@mark-story.com>

ADmad marked this pull request as ready for review April 26, 2024 11:53

markstory merged commit 6ea9621 into 5.next May 11, 2024
9 of 10 checks passed

markstory deleted the 5.1-unbuffered-queries branch May 11, 2024 14:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

5.1 unbuffered queries #17649

5.1 unbuffered queries #17649

ADmad commented Apr 1, 2024 •

edited

othercorey Apr 2, 2024

ADmad Apr 7, 2024

ADmad commented Apr 2, 2024 •

edited

markstory Apr 6, 2024

ADmad commented Apr 7, 2024

othercorey commented Apr 7, 2024

ADmad commented Apr 8, 2024 •

edited

ADmad commented Apr 9, 2024

nicosp commented Apr 9, 2024

othercorey Apr 11, 2024 •

edited

ADmad Apr 11, 2024

markstory Apr 19, 2024

othercorey Apr 11, 2024

ADmad Apr 11, 2024

othercorey Apr 11, 2024

othercorey Apr 11, 2024

ADmad Apr 11, 2024

othercorey Apr 11, 2024

ADmad Apr 11, 2024

nicosp commented Apr 15, 2024

markstory left a comment

markstory Apr 19, 2024

ADmad commented Apr 26, 2024 •

edited

othercorey commented Apr 26, 2024

ADmad commented Apr 26, 2024 •

edited

othercorey commented Apr 26, 2024

		$results = (new Collection($results))
		->map(function ($row) use ($query, $external, $assocData) {

5.1 unbuffered queries #17649

5.1 unbuffered queries #17649

Conversation

ADmad commented Apr 1, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ADmad commented Apr 2, 2024 • edited

Choose a reason for hiding this comment

ADmad commented Apr 7, 2024

othercorey commented Apr 7, 2024

ADmad commented Apr 8, 2024 • edited

ADmad commented Apr 9, 2024

nicosp commented Apr 9, 2024

othercorey Apr 11, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicosp commented Apr 15, 2024

markstory left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ADmad commented Apr 26, 2024 • edited

othercorey commented Apr 26, 2024

ADmad commented Apr 26, 2024 • edited

othercorey commented Apr 26, 2024

ADmad commented Apr 1, 2024 •

edited

ADmad commented Apr 2, 2024 •

edited

ADmad commented Apr 8, 2024 •

edited

othercorey Apr 11, 2024 •

edited

ADmad commented Apr 26, 2024 •

edited

ADmad commented Apr 26, 2024 •

edited