Implement statement classes using \Iterator to fix memory usage regression and prevent rewinding

Regardless of this patch being applied, memory usage is surprisingly high if you select all rows of a large table without actually fetching them, for example:

echo format_size(memory_get_usage()), PHP_EOL;
// prints 25.8 MB
$result = \Drupal::database()->select('search_index')
    ->fields('search_index')
    ->execute();
echo format_size(memory_get_usage()), PHP_EOL;
// prints 111.59 MB
unset($result);
echo format_size(memory_get_usage()), PHP_EOL;
// prints 26.04 MB

And it takes more time to execute() the bigger the table. So is the whole table being read from the database even if you don't fetch it?

#42 and #43 are very interesting, thanks @mfb.

It’s worth trying to do that in the MR and see the impact?

Could be worth experimenting but according to https://www.php.net/manual/en/mysqlinfo.concepts.buffering.php USE_BUFFERED is needed if you want to keep executing other queries while iterating the results; and sounds like it should be more performant for typical queries.

Yeah, no that doesn't work, but at least this little side tangent allowed me to learn about buffered queries

So #44 is not a good idea, reverted and returned to RTBC.

#45is right, we can't do queries while an unbuffered query is active, see this clearly in Drupal\Tests\mysql\Kernel\mysql\SyntaxTest::testAllowSquareBrackets from the test run:

PHPUnit 9.5.26 by Sebastian Bergmann and contributors.

Testing Drupal\Tests\mysql\Kernel\mysql\SyntaxTest
E                                                                   1 / 1 (100%)

Time: 00:00.993, Memory: 4.00 MB

There was 1 error:

1) Drupal\Tests\mysql\Kernel\mysql\SyntaxTest::testAllowSquareBrackets
Drupal\Core\Database\DatabaseExceptionWrapper: 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.: select "name" from "test58670028test" where "name" = :value; Array
(
    [:value] => [square]
)


/var/www/html/core/modules/mysql/src/Driver/Database/mysql/ExceptionHandler.php:46
/var/www/html/core/lib/Drupal/Core/Database/Connection.php:812
/var/www/html/core/tests/Drupal/KernelTests/Core/Database/DriverSpecificSyntaxTestBase.php:34
/var/www/html/vendor/phpunit/phpunit/src/Framework/TestResult.php:728

It would be hard to change that generally for Drupal now. So this issue could help for specific cases when passing a value for PDO::MYSQL_ATTR_USE_BUFFERED_QUERY in the query $options['pdo'] when iterating through large data sets in a controlled manner, i.e. ensuring no other queries are run in the in-between the cursor navigation.

Added some docs based on @catch feedback. Thanks!

Both points of @catch have been addressed.
Back to RTBC.

I have the mysqli driver failing tests with the patch here applied. Setting to NR while I figure out whether the problem is here or in the mysqli driver code.

No, the changes are not BC, because ::markResultsetIterable and friends are not called by downstream code ::execute and ::fetch* methods currently, obviously. So we'll need to deprecate StatementWrapper and introduce a new statement class instead, e.g. StatementWrapperIterator.

For review again.

The testbot is failing for PostgreSQL.

I just picked for testing a version of pgsql too old for D10.

Updated IS with latest evolution

Updated CR.

All the code changes look good to me.
The IS and the CR are in order.
For me it is RTBC.

Isn't the smallest change here

  public function getIterator() {
    while (($row = $this->fetch()) !== FALSE) {
      yield $row;
    }
  }

We don't need any deprecations - we don't couple this any more or any less to the underlying PDO Statement.

#60 didn't really think about it so far... definitely worth trying, thanks!

#60 as a patch.

 PHPUnit 9.5.26 by Sebastian Bergmann and contributors.
13

14
Runtime:       PHP 8.1.14
15
Configuration: /home/runner/work/d8-unit/d8-unit/core/phpunit.xml.dist
18

19
Testing Drupal\Tests\migrate\Kernel\TrackChangesTest
20
F                                                                   1 / 1 (100%)
21

22
Time: 00:00.671, Memory: 10.00 MB
23

24
There was 1 failure:
25

26
1) Drupal\Tests\migrate\Kernel\TrackChangesTest::testTrackChanges
27
Migration failed with source plugin exception: Cannot rewind a generator that was already run in /home/runner/work/d8-unit/d8-unit/core/modules/migrate/src/Plugin/migrate/source/SourcePluginBase.php line 384
28
Failed asserting that two strings are equal.
29
--- Expected
30
+++ Actual
31
@@ @@
32
-'status'
33
+'error'
34

35
/home/runner/work/d8-unit/d8-unit/vendor/phpunit/phpunit/src/Framework/Constraint/Equality/IsEqual.php:96
36
/home/runner/work/d8-unit/d8-unit/core/modules/migrate/tests/src/Kernel/MigrateTestBase.php:205
37
/home/runner/work/d8-unit/d8-unit/core/modules/migrate/src/MigrateExecutable.php:191
36
/home/runner/work/d8-unit/d8-unit/core/modules/migrate/tests/src/Kernel/MigrateTestBase.php:177
37
/home/runner/work/d8-unit/d8-unit/core/modules/migrate/tests/src/Kernel/TrackChangesTest.php:106
38
/home/runner/work/d8-unit/d8-unit/vendor/phpunit/phpunit/src/Framework/TestResult.php:728 

So #62 fails on this

  /**
   * Rewinds the iterator.
   *
   * Implementation of \Iterator::rewind() - subclasses of SourcePluginBase
   * should implement initializeIterator() to do any class-specific setup for
   * iterating source records.
   */
  #[\ReturnTypeWillChange]
  public function rewind() {
    $this->getIterator()->rewind();
    $this->next();
  }

when rewinding the StatementWrapper iterator, \ArrayIterator (current HEAD) is rewindable, Generator (after the patch) is not.

https://www.php.net/manual/en/generator.rewind.php

If iteration has already begun, this will throw an exception.

So for me #62 is breaking BC. Back to RTBC for the MR.

@mondrake hmmm.... but in the MR

  /**
   * {@inheritdoc}
   *
   * @internal This method should not be called directly.
   */
  public function rewind(): void {
    // Nothing to do: our DatabaseStatement can't be rewound.
  }

So i'm not sure that that test is actually real. You can't rewind the MR either. it just does error properly.

The MR no-ops. The Generator throws an exception.

I do not know why the migrate plugin rewinds the iterator. But no matter why, HEAD and MR are fine, #62 fails. That’s enough for me to say #62 is not BC.

Note: StatementPrefetch (used by Sqlite) implements \Iterator and its rewind() method is exactly equal to the MR (actually, copy/pasted)

I do not know why the migrate plugin rewinds the iterator.

Yeah but what it is doing fails. Like silently. So it's bogus API.

Yes. To fix that, though, we need to fix the bogus API and the bogus usage of the API.

IMHO we can do the former here, taking care also of StatementPrefetch, but as a deprecation, because otherwise we introduce a BC break.

The latter, though, should be a followup.

Deprecated rewinding the statement.

Last commit should fail on the same tests as #62, but here with a deprecation instead. Only for mysql and pgsql here. Next, adjust StatementPrefetch to do the same for SQLite.

Split the \Iterator related methods in a trait, so that the new class uses it but also StatementPrefetch can start using it. Added a deprecation ignore for the statement rewinding error thrown by TrackChangesTest that should be fixed separately.

Ended up rewriting entirely StatementPrefetchIterator, benefitting from the StatementIteratorTrait - now both StatementWrapperIterator and StatementPrefetchIterator share the \Iterator implementation code. In the process, added a test for fetchAllAssoc() to FetchTest.

Filed 🐛 Drupal\migrate\Plugin\migrate\source\SourcePluginBase::rewind() is rewinding database statements Fixed as a separate issue.

Couple of tweaks. Now the \Iterator methods (current(), valid(), key() etc.) are implemented ONLY in the iterator trait code and not used anywhere by the statement code.

In my opinion, these methods should be there only to allow PHP internals to execute the foreach() loop on the object implementing \Iterator. They should *not* be called directly by consuming code.

Rebased

The MR looks good.

The IS and the CR need to be updated.

Updated IS and issue title. Appreciate a review before going for the CR update.

@mondrake: Could you address my remark on calling markResultsetIterable() twice gives a unexpected result. The rest of the MR looks great.

Thanks @daffie for finding #78. All comments in MR addressed.

I still think we are taking the wrong more disruptive approach here. as pointed out in #66 the bit on #62 that fails is actually failing on HEAD it's just not telling us it is because we've incorrectly implemented the API. I think we should fix that here rather then introducing more disruption to correct usages.

I do not think we are disrupting anything. We're improving and cleaning up the statement classes, rather. SQLite has code that has been useless for 15 years. Non-core drivers (are there many?) can still use the previous implementations until Drupal 11 comes. But, obviously that is my opinion. Please mark this as won't fix if you disagree so not to spend more time on moonwatching.

This code is a big performance improvement for SQLite so we need to do something. I just think that fixing this should not require us to refactor our statement wrapper classes yet again.

All code changes look good to me.
I have updated the CR for SQLite.
For me it is RTBC.

The current fix still allows the bogus rewind to occur in the migrate code. Doing a no op is not a fix.

This MR adds and skips a deprecation. Without an on issue discussion as to whether this is correct here. In my opinion it is not correct. If we are to go the way of the most recent MRs then we need to fix the code that triggers the deprecation in core. Because adding it to the skip list makes it harder for code that triggers it to discover.

Let's postpone on 🐛 Drupal\migrate\Plugin\migrate\source\SourcePluginBase::rewind() is rewinding database statements Fixed . Solving that will remove the need to skip the deprecation here, and hopefully can be done independently from this issue.

#86 has been fixed. Now I think we can do the following patch.

... or, alternatively, fail when a statement is tried to be rewound (deprecation error now, excpetion in d11), which I would prefer. See MR.

🐛 Drupal\migrate\Plugin\migrate\source\SourcePluginBase::rewind() is rewinding database statements Fixed has landed and the MR has been updated for it.
Back to RTBC.

There's no need for all the change to issue a deprecation or even better an exception. If you iterate twice over a statement that's an error and your code is not working the way you think it is. In my opinion there is no need for an deprecation in this instance - we can move straight to an exception.

But also adding the deprecation/exception is not part of fixing the memory issue that this issue was originally filed for. That's fixed by #87 and it adds a test that proves consistent rewind behaviour on all core drivers.

Here's a patch that issues a deprecation on rewind without all the API change in the MR.

Looking at #91 in more depth I see no reason for no result statements to behave differently.

#92 looks a lot more straightforward. I think if we wanted to do the broader changes in the MR anyway, we could look at a follow-up task for that.

Please correct me if I am wrong...

More conceptual than anything, but if I read correctly #92 will throw deprecation only if a cursor has been fully fetched. But, in #65, we found that

rewinding the StatementWrapper iterator, \ArrayIterator (current HEAD) is rewindable, Generator (after the patch) is not.

https://www.php.net/manual/en/generator.rewind.php

If iteration has already begun, this will throw an exception.

Aren't we getting different results in #92 for the case of full fetching vs. the case of partial fetching? Maybe we need a separate test for that?

@mondrake so your comment got me to thinking about writing a test for that.

  public function testStatementRewindForEach() {
    $statement = $this->connection->query('SELECT * FROM {test}');

    $count = 0;
    foreach ($statement as $row) {
      $count++;
      $this->assertNotNull($row);
      if ($count > 2) {
        break;
      }
    }
    $this->assertGreaterThan(0, $count);

    // Continue iterating through the same statement.
    $count = 0;
    foreach ($statement as $row) {
      $count++;
      $this->assertNotNull($row);
    }
    $this->assertSame(2, $count);
  }

Both the MR and patch fail this test and incorrect trigger a deprecation. I think we should do #87 and add the above test and then, maybe, file a follow-up to think about how to do the deprecation or exception on rewind. DB statements in Drupal have not been rewindable for years and only our error in #3174662: Encapsulate \PDOStatement instead of extending from it → made this possible for MySQL and Postgres... but not SQLite.

When you change the test in #95 to assert on actual counts it reveals an existing bug in resumption of looping around a statement in \Drupal\Core\Database\StatementPrefetch - ie. SQLite. I;ve added the test here and added @todo for SQLite because I think adding the test coverage here is more important than fixing the existing bug here.

+++ b/core/tests/Drupal/KernelTests/Core/Database/StatementTest.php
@@ -43,4 +43,47 @@ public function testRepeatedInsertStatementReuse() {
+  /**
+   * Tests a follow-on iteration on a statement using foreach.
+   */
+  public function testStatementForEachTwice() {
+    $statement = $this->connection->query('SELECT * FROM {test}');
+
+    $count = 0;
+    foreach ($statement as $row) {
+      $count++;
+      $this->assertNotNull($row);
+      if ($count > 2) {
+        break;
+      }
+    }
+    $this->assertSame(3, $count);
+
+    // Continue iterating through the same statement.
+    foreach ($statement as $row) {
+      $count++;
+      $this->assertNotNull($row);
+    }
+    // @todo Fix SQLite bug.
+    $this->assertSame($this->connection->databaseType() === 'sqlite' ? 5 : 4, $count);
+  }

I think this is wrong expectation... you cannot start a

foreach

, break, and resume with another foreach from where you left.

Internally PHP always calls (unconditionally, I assume) rewind() when starting a foreach loop,

This is the first method called when starting a foreach loop. It will not be executed after foreach loops.

https://www.php.net/manual/en/iterator.rewind.php

Working on getting the MR green.

The MR now incorporates tests from #96, with the assumption that

1. rewinding a non-empty statement after fetching has started throws a deprecation and no-ops in D10, will throw exception in D11
2. rewinding an empty statement no-ops

The beauty of the MR is that since all the iteration code is in the trait, fixing it there fixes both statment classes in core. I like the trait and am planning to use in in contrib for mysqli and DruDbal drivers, that require their own statement classes anyway (they're not PDO based).

The added testes look good to me.
Back to RTBC.

Thank you.

I wonder whether we should use this occasion, we have two new statement classes, also to have a new StatementInterface, fully typehinted, so to finally leave PHP 5 behnd.

The deprecation has been removed.
The IS has been updated.
Back to RTBC.

Added a review to the MR. I have some concerns about scope. For example code is removed from \Drupal\sqlite\Driver\Database\sqlite\Statement that is not related to fixing this. We're also changing calls to low level PDO functions that are not part of fixing this change.

The conversions from switches to matches could happen in a follow-up.

code is removed from \Drupal\sqlite\Driver\Database\sqlite\Statement that is not related to fixing this

no it is related to fixing this, because that piece of code is doing strange things with the resultset that are incompatible with the new trait that keeps the iterator related properties private on the base class. But yes, that can be removed independently from this, I opened a spin-off for that and we'll have to wait that before continuing here.

TIL match(). Thanks!

Postponed on 📌 Remove obsolete code from Drupal\sqlite\Driver\Database\sqlite\Statement Fixed .

Meant to postpone.

Well that was fast. Working on @alexpott’s feedback.

Addressed @alexpott's feedback inline in the MR, with the exception of two points where I'd like to have a further thinking before doing changes. Thanks!

All remarks have been addressed.
Back to RTBC.

Found an issue with latest changes, which uncovers a lack of test coverage.

The problem was with '0' being confused with FALSE in fetchField, i.e. a valid scalar interpreted as end of recordset.

I was also very tempted to add a trait to split out the code that emulates the conversion, in StatementPrefecth, of data in FETCH_ASSOC format to other FETCH_* modes - would be very useful in contrib for non-pdo based drivers. But I'll leave that to a follow up.

Second thinking, I'm after the trait that will make all more organised.

Fixed the CS of the snake_case vs camelCase.

Added a trait to manage emulation of fetch modes, implemented in the StatementPrefetchIterator. Will be very useful for the mysqli driver that is not PDO based and therefore con only emulate PDO’s fetch modes.

Posted some small nits to the MR review.

@mondrake FetchModeTrait is yet more API - the new trait is another API surface for us to support. Ideally improvements in API are not mixed in with bug fixes. Can't this be part of a follow-up? It's not necessary to fix the bug and unlike the introduction of the other trait we can't even argue that this is a necessary refactor to make iteration of all db statements consistent.

OK, I tried. Let me revert that and put in a follow up.

Done #122.

Filed 📌 Introduce a FetchModeTrait to allow emulating PDO fetch modes Fixed .

I added one question to the MR otherwise I think we're finally there with this one :)

@mondrake thanks for the recent changes and opening of follow-ups and blockers.

@VladimirAus can you please explain what was the purpose of your push, and did you use push --force? Why?

I'm working on the test addition.

Added a test. There can be certainly more (for instance, testing that fetchAll* returns the remaining resultset in a partially iterated statement), but test hardening can be postponed to later on IMHO.

So, the test uncovered a bug.

Made fixes to the StatementWrapperIterator methods that are 'foreaching' the statement themselves - in a way that if the statement was already traversed fully, they can return an empty array straight ahead instead of erroring - in respect to the API.

For the 2 remarks on the MR.

All changes look good to me.
Back to RTBC.

Needs adjustment after commit of ✨ Allow the database query log to be dispatched as log events Fixed . On it.

The reroll looks good to me.
The testbots are all green.
Back to RTBC.

Committed c6532d8 and pushed to 10.1.x. Thanks!

mondrake → closed merge request !1842

A @todo that was left over from a previous version of the MR got committed inadvertently. Filed 🐛 Remove @todo leftover of #3265086 Fixed to remove it.

Automatically closed - issue fixed for 2 weeks with no activity.

Adjusting issue credit as credit is not provided for button-click rebases without additional contribution.