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

This is trying to change StatementWrapper from implementing \IteratorAggregate to implement \Iterator instead. This would avoid pre-fetching all records when iterating over the statement.

Working on a more round patch…

Deprecating StatementWrapper::getIterator and implementing methods and properties to track fetching status so that an \Iterator can be implemented.

@daffie besides a deprecation test for getIterator(), are there other tests needed?

TBH, I do not care. We have a StatementWrapper since 9.1 precisely to abstract away from any underlying statement object.

#18 AFAICS, Drupal\KernelTests\Core\Database\FetchTest is already testing traversing the executed statement via foreach loops.

I see the point in testing the memory consumption, but I am afraid such a test would be rather tricky/fragile.

Added a CR placeholder, to be filled in at later stage.

Added a deprecation test.

I am trying to understand this patch and why it does it the way it does. I need some help to understand it.

@daffie those methods are must haves for a class implementing \Iterator.

More about the iterator design pattern: https://refactoring.guru/design-patterns/iterator/php/example

I am still not convinced that we should do this issue.

If we do, than we need testing for the new methods: 'key', 'current', 'next', 'rewind' and 'valid'. Not if we need to add testing for the new class variables and protected helper methods.

Could we update the IS and CR to make more clear why we are doing this issue.

@daffie have a look at the StatementPrefetch class: this patch does something very similar. The difference is that StatementPrefetch loads ALL the rows in an array in memory to release the database for changes (IIRC SQLite wont' allow e.g. to INSERT or UPDATE if there's an open SELECT resultset, or something like that), here we do not do that, we just keep in synch the traversal of the StatementWrapper through foreach() or while() with fetching from the wrapped client statement.

In fact, currently in HEAD, if you do foreach($statement as $row) {...} you do something same as StatementPrefetch: PHP looks for the iterator logic, finds that it is an \IteratorAggregate class, gets the iterator via getIterator() that in the case at hand is a fetchAll() to the wrapped client statement. After the patch, PHP finds \Iterator so it knows it has to use the specific methods (current, key, etc), that allow us to fetch one record at a time instead.

rerolled

I have another thought about this and I see why we should do this.
No new tests are needed.
The CR needs an update.

Test fails on SQLite, because it does not implement StatementWrapper. I'm not keen on introducing yet another conditional to skip tests based on the driver. Postpone on getting a more stable solution in #3231950: Split Database tests in 'core' ones and 'driver specific' ones.

#3231950: Split Database tests in 'core' ones and 'driver specific' ones has landed.

Rebased to 10.1.x

2 unresolved threads on the MR.

Replied.

Ok, let keep it as it is.
All code changes look good to me.
I have updated the IS and CR.
For me it is RTBC.

How come #26 has not been addressed? Like if we can fix this without an API change that'd be nice.

Correct me if I am wrong, but if we rely on the underlying PDOStatement,

1. we re-couple the StatementWrapper to PDO, which we struggled to decouple from
2. we do not solve the issue at hand (the IteratorAggregate would have to fetch all resultset and pass it back in single yields)

I was trying to make it consistent with StatementPrefetch, which implements \Iterator and is used by SQLite.

The question of @alexpott has been addressed.
Back to RTBC.

Rebased

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?

Looks like the answer to #42 is that this is due to \PDO::MYSQL_ATTR_USE_BUFFERED_QUERY => TRUE in modules/mysql/src/Driver/Database/mysql/Connection.php - if I change this to FALSE then the execute() on all rows of a large table uses almost no memory and returns almost instantly.

#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.

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

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

Time: 00:00.671, Memory: 10.00 MB

There was 1 failure:

1) Drupal\Tests\migrate\Kernel\TrackChangesTest::testTrackChanges
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
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'status'
+'error'

/home/runner/work/d8-unit/d8-unit/vendor/phpunit/phpunit/src/Framework/Constraint/Equality/IsEqual.php:96
/home/runner/work/d8-unit/d8-unit/core/modules/migrate/tests/src/Kernel/MigrateTestBase.php:205
/home/runner/work/d8-unit/d8-unit/core/modules/migrate/src/MigrateExecutable.php:191
/home/runner/work/d8-unit/d8-unit/core/modules/migrate/tests/src/Kernel/MigrateTestBase.php:177
/home/runner/work/d8-unit/d8-unit/core/modules/migrate/tests/src/Kernel/TrackChangesTest.php:106
/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. The MR just does NOT 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 #3339373: Drupal\migrate\Plugin\migrate\source\SourcePluginBase::rewind() is rewinding database statements 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 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 #3339373: Drupal\migrate\Plugin\migrate\source\SourcePluginBase::rewind() is rewinding database statements. 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.

#3339373: Drupal\migrate\Plugin\migrate\source\SourcePluginBase::rewind() is rewinding database statements 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 behind.

EDIT: Second thinking, not a good idea (yet). It would make sense to do some cleanup of the supported fetch modes first, #3345938: Deprecate support for unused \PDO::FETCH_* modes. That might reshape some methods.

What's interesting about #97 and that test is that it works the same as PdoStatement and it behaves like a cursor. If you change

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

to

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

on MySQL and PostgreSQL the test still passes. It's quite a bit harder to get the client statement from SQLite...

So we're choosing to depart from PdoStatement behaviour - which is also fine but should be noted.

The problem that prevents us committing the MR though is highlighted by the test in this test

  public function testStatementForeachTwice(): void {
    $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);

    // Restart iterating through the same statement. The foreach loop will try
    // rewinding the statement which should fail, and the counter should not be
    // increased.
    $this->expectDeprecation('Rewinding a StatementInterface object when fetching has started is deprecated in drupal:10.1.0 and will throw an exception from drupal:11.0.0. Refactor your code to avoid rewinding statement objects. See https://www.drupal.org/node/3265938');
    foreach ($statement as $row) {
      $count++;
      $this->assertNotNull($row);
    }
    $this->assertSame(3, $count);
  }

This test will do something very different on HEAD or with \PdoStatement. The current behaviour in the MR makes it look like there are 3 rows whereas there are 4. Amusingly on HEAD this the final count will be 4 for Postgres and MySQL but it will be 5 for SQLite because of bugs in the StatementPrefetch class. Yes at least the MR is consistent :)

I think that this will be tricky to solve and perhaps rather and deprecating going straight to an exception is warranted because that's the behaviour we seem to actually want. If we want to pursue the deprecation then we need to make it behave as it has before we broke it in #3174662: Encapsulate \PDOStatement instead of extending from it - i.e. we need to make it behave like a \PdoStatement

Let's go for an exception in place of a deprecation then, if all agree. The current state after #3174662: Encapsulate \PDOStatement instead of extending from it has been in place for more than 2 years with noone reporting the issue; so trying to go back to the situation prior than that change seems overkill to me.

Discussed with @catch who suggested instead of doing an exception in Drupal 10 or a silenced deprecation we should do

trigger_error('..', E_USER_WARNING)

in Drupal 10 and then change to an exception in Drupal 11. I think this gives us the best of both worlds. Code that is broken now will have a warning to fix but we won't cause any sites to stop functioning.

OK, on that.

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 #3346898: Remove obsolete code from Drupal\sqlite\Driver\Database\sqlite\Statement.

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 everything 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 #3347497: Introduce a FetchModeTrait to allow emulating PDO fetch modes.

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 #3313355: Allow the database query log to be dispatched as log events. 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!

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

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