Fix errors when using unbuffered mysql queries

On the Umami front page with a cold cache, this takes 2mb of the peak memory usage for ::execute() and more like 4mb off the incl (not peak) memory usage.

I'm not seeing meaningful change in the headline figures for the request though.

Will also try on a page that's loading a lot more rows from the db.

Well, quite a lot breaks. Looks like it's mostly in migrate.

More xhprof screenshots - in this case from a custom JSON:API endpoint that loads 2000 entities * 3 in chunks of 100.

I'm also seeing 10-15mb off the total memory usage (both peak and non-peak) in xhprof, and can't see another explanation for it being lower apart from this change, e.g. from about 105mb for this endpoint down to 90mb.

Slowly working through the test failures.

Migrate's SQL idmap was explicitly setting a statement as a property then iterating over it when other queries are run - this needed a fetchAll().

Entity loading was doing some cache gets while iterating over a result set, this is already dealt with by #3576129: Reduce calls to FieldDefinition::getColumns() so cherry-picked across here.

Node search also passed a statement around while executing other queries - another ::fetchAll().

There are some test failures in migrate_drupal that I think are different to idMap, but they look fairly plugin-specific. All that code will go in #3572280: Remove Migrate Drupal so ignoring that for now, these appear to be nearly 100% of the remaining kernel test failures.

I've added some bullets to the issue summary on how I think we can approach this - essentially get core compliant, then enable it for tests, then the same for contrib, then eventually (and optionally) flip the default for new installs.

#2347867: Race conditions with lock/cache, session storage - add a non-transactional database connection would 'fix' a lot of the remaining test failures here - because what's commonly happening is foreaching over a query, then checking some kind of entity metadata (like whether the entity type has an integer or string ID key), and this then loads the entity definitions, which does a cache get, which issues a database query. If the cache backends used a different database connection, then MySQL wouldn't mind this at all.

This isn't fully green, but that's not really the point of this issue as such:

- all functional tests are green
- remaining kernel test failures entirely (or at least mostly) in migrate_drupal which is about to be removed
- the change that causes the test failures will be removed from the MR prior to commit, it's there to flush out the test failures.

I suspect that the benefits might be very situation dependent and not generic enough to warrant switching the default behaviour.

That might be true but right now it's not possible to use unbuffered queries with core at all, so I think we could do 1-3 in the issue summary, and by the time we've done all that we might have some data about whether we want to go ahead with #4.

can releases locks, table resources, and the result cursor immediately after sending all rows

For most select queries in core we do very little with the results except foreach over them and collect them into an array anyway.

But this change will still result from queries have longer read locks against a table right - so anything wanting to write to the table will have to wait longer - no?

No because InnoDb doesn't do read locking at all, only when you issue SELECT .. FOR UPDATE to explicitly acquire a lock (and FOR SHARE I think but never used or even encountered that in the wild). Are you thinking of MyISAM or am I missing something? https://dev.mysql.com/doc/refman/8.4/en/innodb-locking-reads.html

According to https://www.php.net/manual/en/mysqlinfo.concepts.buffering.php the thing that doesn't get freed until the entire set is fetched is only the mysql connection (and the ability to make other queries on the same connection, which is what the MR has fixes for) - but we keep the mysql connection until the end of the request anyway so that bit is not really relevant for us at all.

I wouldn't trust what Claude says about anything...

MR is green now: https://git.drupalcode.org/project/drupal/-/pipelines/758574

Just to be clear, this issue will not switch from buffered to unbuffered queries, all I want to do is make core compliant in this issue, then an issue to add test coverage (not sure how yet, might need a test-only driver that's otherwise identical to the mysql driver?).

Then we can have a proper policy issue about whether to switch the default and how after all that. The commit to change the default in this issue is only to get useful test runs.

Reverted that initial commit for now, so the MR contains only the changes actually proposed here (and the entirety of #3576129: Reduce calls to FieldDefinition::getColumns()).

Need to remember to unrevert that query to validate any further changes in this MR though.

We are increasing the scope for non-repeatable reads with using the recommended READ COMMITTED transaction level. Because the window for data to change while we are reading the table is increasing.

Not sure this window is 'longer' except in very specific situations.

For a buffered query, MySQL executes the query, and all of the results get loaded into memory before PHP gets a statement back.

For an unbuffered query, PHP immediately gets a statement back, and then MysQL streams the results to PHP as they come in.

When there is a single row returned, there should be no measurable difference between the two as far as I can tell - you get one row back and process it and you're done.

For multiple rows, it should mean that in the unbuffered case, we start processing results earlier than the unbuffered case, then whether we finish processing those rows earlier or later would depend on how much work is being done in PHP. It could well be that the full result set is processed in the same time, because we are essentially interleaving i/o wait with CPU, instead of a big i/o wait followed by CPU.

If you are doing a CPU intensive task like writing out CSV files or processing images or something for each row, then sure it could make a difference, but I don't see it increasing the window much or at all for Drupal.

Where it will make a bigger difference is the relative load between PHP and MySQL.

Rebased.

Rebased.

Rebased again.

Do we still need to have iterable StatementInterface objects after this? It looks like when a foreach is involved, now the approach is to iterate over a fetchAll result.

At the moment, the StatementInterface being iterable doesn't save any memory because the queries are buffered.

After this MR, there is a memory saving if you iterate over StatementInterface, but you can only do so if you don't execute other queries while iterating - because to do so causes a MySQL error.

The changes in the MR are to remove the places in core that do that, so that it'll possible to run core with unbuffered queries. But I don't think it means dropping StatementInterface.

Kicked off pipelines on other database combinations.

Rebased - the node search plugin moved. Manually re-applied the same changes to the new location.

Missed one spot in the rebase.