I noted that StatementWrapperIterator implements fetchAllAssoc() and fetchAllKeyed() incorrectly since they run a foreach loop on the statement itself - which in case a statement is already partially fetched requires a rewind that is not allowed in our implementation. This is not a case for StatementPrefetchIterartor that keeps fetching records while there are available.
IMHO we should fix it to align all fetchAll* methods to the standard PDO fetchAll() behaviour which is
PDOStatement::fetchAll()returns an array containing all of the remaining rows in the result set.
See https://www.php.net/manual/en/pdostatement.fetchall.php#refsect1-pdostat...
| Comment | File | Size | Author |
|---|---|---|---|
| #18 | 3350375-10.0.x-regression-proof.patch | 2.83 KB | alexpott |
Show commandsStart within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
changes, plain diff MR !3729
Comments
Comment #2
mondrakeA test-only patch to demonstrate the issue.
Comment #5
mondrakeFix would be avoiding to use
foreach.Comment #6
smustgrave commentedRan the tests without the fix locally and they passed unfortunately.
Comment #7
mondrake@smustgrave on which db type did you test?
Comment #8
smustgrave commentedTested on fresh installs of mariaDb, mysql, and postgres and the tests pass each time without the fix.
Won't move to NW though because clearly they are failing here.
Comment #10
mondrakeJust rerun tests in #2, and mysql and pgsql fail as expected - sqlite is not failing, expected as well, since that driver is not using StatementWrapperIterator.
Will try on github, in the meantime NR by anyone else that can reproduce @smustgrave results and figure out why.
Comment #11
mondrakeI setup a Github Action test workflow - https://github.com/mondrake/d8-unit/blob/test-3350375/.github/workflows/...
and I get the test failures - https://github.com/mondrake/d8-unit/actions/runs/4630185826
So I cannot replicate #6 and #8
Comment #12
smustgrave commentedI won't be the hold up as my local could be different and change isn't so large I must get a failure. Will let the committers decide on this one but don't mind marking as #2 shows the tests do fail (reran earlier).
Comment #13
larowlanWhilst this seems logical to me, I have a concern that this may disrupt someone who has written existing code that relies on the current behaviour.
Is this something we can make opt-in (e.g. via settings.php flag) that we default to on for new installs?
Would like a release manager to have a look at this and assess the possible risk of disruption, so tagging as such.
Comment #14
larowlanCan we get a change record here too, will make it easier for a release manager to review as well.
Comment #15
catchIt seems unlikely that someone would start fetching individual records and then call fetchAll() half way through... but also you never know.
Is there some way we can detect that iteration has already begun? But then I guess the new code allows for this, just with different results, so it's not really a deprecation as such, just a behaviour change.
Overall I feel like this is probably OK in a minor release with a change record and release note, although it might be better to commit early in 10.2 to give the maximum time for anyone to run into issues (which won't be long now before 10.2 is open really).
Comment #16
alexpott@mondrake what was the behaviour prior to #3265086: Implement statement classes using \Iterator to fix memory usage regression and prevent rewinding?
Answering my own question. This is a regression introduced by #3265086: Implement statement classes using \Iterator to fix memory usage regression and prevent rewinding and should be fixed prior to 10.1.x being released.
I reverted
c6532d8c5cthe commit hash for 3265086 and added @mondrake's new tests. They pass - whereas on head they fail. This is for both mysql and sqlite (so both flavours of statement class we have).@mondrake can you confirm my findings? If this is the case I don't think we need a change record or a release manager review.
Comment #17
mondrakeThis is an edge case, I do not think anyone was actually starting to fetch single records to then fetchAll* the remaining part. I am not sure what the situation was before #3265086: Implement statement classes using \Iterator to fix memory usage regression and prevent rewinding; I will try a test-only patch run on 10.0 and 9.5 to discover (I tried running the one in #2 but that does not apply).
I also think we should consider this a completion of #3265086: Implement statement classes using \Iterator to fix memory usage regression and prevent rewinding.
Comment #18
alexpottHere's proof that this is a regression on 10.1.x - here's the test-only patch but for 10.0.x.
Comment #19
alexpottThe postgres fails on 10.0.x are due to a regression caused by #3191391: Schema::changeField() has bug when changing regular serial field to big serial field that's been fixed due to a revert. As #18 shows us this fixes a regression caused by #3265086: Implement statement classes using \Iterator to fix memory usage regression and prevent rewinding therefore this does not need a CR or release manager review as we're fixing an accidental behaviour change in 10.1.x and the original RTBC in #12can stand.
Committed 1057a39 and pushed to 10.1.x. Thanks!