Add iterators for non pdo drivers. #2718

jenkoian · 2017-05-08T09:54:35Z

Allows result sets to be iterated over without running out of memory on
large data sets.

--

Before: Iterating (e.g. foreaching) over a large data set would often run out of memory.
After: Iterating over a large data set uses iterators using the cursor so that memory usage remains low.

That's the aim anyway, tested (manually) the SQLSrv driver and it has the desired effect, could do with some help testing the other drivers if anyone is able? I would have liked to add some unit tests but none of the existing stuff is unit tested and I'm not really sure where I would even start, again any help here appreciated also.

Small update: I have reverted the changes for the mysqli driver as I don't quite understand that driver enough, it seems to behave unlike any of the others, again any help here much appreciated. Also came across the DataAccessTest which should cover most of this, it certainly brought out the issues with the mysqli driver, so that's good.

Another update: I have removed all the stuff with iterators, favouring generators. This makes things a lot easier as we can just re-use the existing fetch functionality. Generators are not without their issues though as they cannot be re-used, see the discussion below for more info, in particular this comment.

Update (20170724): Due to concerns over consistency both PDO and non PDO can now be iterated but not rewindable. A rewindable generator has been included if users wish to use this (possible this will be removed though?).

Update (20170914): StatementIterator class created which is now used by all non PDO statements. Extracting to own class makes it easier to test.

jvasseur · 2017-05-17T09:06:37Z

You could use a generator instead of creating classes for the iterators, it would simplify the code needed for the feature.

jenkoian · 2017-05-17T09:12:22Z

@jvasseur agreed that would be easier...as getIterator needs to return a Traversable though not sure it would work? Did consider an anonymous class here and indeed started with that but (can't remember why now) couldn't quite get it to work, although admittedly I didn't spend very long looking into why.

jvasseur · 2017-05-17T09:28:10Z

The Generator class implements the Iterator interface so the return type of getIterator is not a problem.

jenkoian · 2017-05-17T09:31:57Z

@jvasseur wow, I did not know that! Thanks, will update PR, this makes it way easier.

jenkoian · 2017-05-17T09:41:40Z

@jvasseur updated PR, thanks! Will likely give it some time for feedback before rebasing so's we can all forget that whole iterator thing ever happened 😆 Have manually tested the SQL server implementation and it works.

deeky666 · 2017-05-17T15:30:19Z

While the idea is good, using generators is a BC break when doing the following:

$result = $statement->getIterator();

foreach ($result as $row) {
}

// will error because generators cannot be reused
foreach ($result as $row) {
}

Not sure if we should care though. /cc @Ocramius

Ocramius · 2017-05-17T21:33:21Z

We could provide a rewindable iterator somehow: tricky but feasible

…

On 17 May 2017 5:30 p.m., "Steve Müller" ***@***.***> wrote: While the idea is good, using generators is a BC break when doing the following: $result = $statement->getIterator();foreach ($result as $row) {}// will error because generators cannot be reusedforeach ($result as $row) {} Not sure if we should care though. /cc @Ocramius <https://github.com/ocramius> — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2718 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAJakIgNx32rdEVeOh3V6FTDZid3Ns2tks5r6xKMgaJpZM4NTuoD> .

morozov · 2017-05-18T00:10:04Z

@Ocramius could you elaborate a bit more, besides the technical possibility and BC, what could it be needed for?

Generators do not allow to be rewound on purpose, and I think the same is applicable to the statements: as a developer, I'd better be informed that I'm re-fetching data from the same statement twice and redesign my logic instead of just fetching the same data again.

Ocramius · 2017-05-18T07:33:14Z

@morozov we could accumulate the data that was already iterated upon

deeky666 · 2017-05-18T09:08:13Z

@morozov I tend to agree with your argumentation but TBH I still find it confusing that generators implement \Iterator although rewind() is definitely something that generators do not support. It might be a matter of taste but when I want an iterator from a statement and it tells me I get \Iterator, I expect it to be an object that implements the interface correctly. There is no word in the documentation about allowing objects not to be rewindable. IMO PHP has brought a lot of confusion with that to the userland and personally I am not a friend of pushing it further.

@Ocramius do you mean "caching" fetched data in the object while iterating? What is wrong with simply executing the statement again on rewind?

Ocramius · 2017-05-18T09:09:47Z

What is wrong with simply executing the statement again on rewind?

Side effects

deeky666 · 2017-05-18T09:15:02Z

@Ocramius can you give an example? Funfact: I am actually working on 3.0 version of the driver component since two weeks or so and have come up with this (draft, mysqli).

Ocramius · 2017-05-18T09:20:16Z

@deeky666 for example, a query that selects values from a sequence will allocate new sequence values, instead of retrieving the same ones over multiple calls.

deeky666 · 2017-05-18T09:26:17Z

@Ocramius good point. That's reason enough for me. So then iterators should probably really be forward only here. I don't see how we could make them rewindable then without storing fetched rows in memory.

jenkoian · 2017-05-19T07:42:33Z

I've added a failing test case for the re-usable iterator problem.

jenkoian · 2017-05-19T16:08:39Z

I added an example of a RewindableGenerator to the mysqli driver after I noticed Symfony used a RewindableGenerator, feedback welcome. If this works I will look to roll out to the other drivers.

jvasseur · 2017-05-19T16:09:56Z

lib/Doctrine/DBAL/Driver/Mysqli/MysqliStatement.php

-        $data = $this->fetchAll();
-
-        return new \ArrayIterator($data);
+        $that = $this;


You can use $this inside the closure instead of doing that.

Been working on a legacy codebase for too long...thanks

jvasseur · 2017-05-19T16:10:23Z

lib/Doctrine/DBAL/RewindableGenerator.php

+        return $g();
+    }
+
+    public function count()


I don't think we need to implement the countable interface here.

Yep, you're right, will remove 👍

Ocramius · 2017-05-20T12:59:56Z

lib/Doctrine/DBAL/RewindableGenerator.php

+
+    public function getIterator()
+    {
+        $g = $this->generator;


return ($this->generator)();

This would simply call the generator callable at each foreach () {} (because each foreach will cause a ->getIterator() call).

That's most likely what we do NOT want, as a query will be repeated.

The test case is as following:

$result = someQuery(); foreach ($result as $value) { } foreach ($result as $value) { } self::assertCount(1, $executedQueries);

Ocramius · 2017-05-20T13:02:56Z

tests/Doctrine/Tests/DBAL/Functional/DataAccessTest.php

+            $j++;
+        }
+
+        $this->assertEquals($i, $j);


The amount of executed queries is to be checked here - you can use something like SELECT CURRENT_TIME_IN_MICROSECONDS() (or equivalent - I made this SQL up) to verify that the result doesn't change over multiple calls.

I've added a way of counting the number of executed queries using DebugStack take a look, if that doesn't quite get what we need I will do the microseconds thing.

Argh, I just checked: the logger is only used outside statements, not inside them :-(

This means that the logger will only log the creation of the statement and its first execution.

A stateful query would most likely be better. Sorry for not noticing this before!

Thanks for your reply and sorry for my delay. I've pushed something which I'm pretty sure is what you mean. However, I don't think this works. I can't seem to get microseconds. Only thing I can do to get around this is to add a sleep call 😞 . Any ideas?

I think this is likely because of the MySQL version. 5.6 and up seems to support it. Travis is on a pre 5.6 version of MySQL IIRC although I think the new trusty boxes may be newer. So I guess we could update travis to build against the newer versions if we want (if this is indeed the issue!).

We can do that, yes (using trusty on travis-ci). Ideally, we'd test this feature with SQLite in-memory though, unless we want something mysql-specific

afaict, travis is using mariadb which I think supports microseconds, so think it should be fine?

On another note, I noticed the build is failing and it's because pdo doesn't actually support rewinding the iterator (well not by default is my understanding, may be able to with unbuffered queries) so made me questions if we even needed to support this for non pdo drivers, or if we should, should we wrap the pdo statements so they can be rewound too?

Ocramius

Requires testing against stateful queries

Ocramius · 2017-06-23T07:39:11Z

On another note, I noticed the build is failing and it's because pdo doesn't actually support rewinding the iterator (well not by default is my understanding, may be able to with unbuffered queries) so made me questions if we even needed to support this for non pdo drivers, or if we should, should we wrap the pdo statements so they can be rewound too?

If an iterator cannot be rewinded, we'll just need to build an accumulator and rewind ourselves (at the cost of memory): providing inconsistent features is worse than providing features at all :-)

jenkoian · 2017-06-23T08:35:44Z

@Ocramius to clarify, if we're going to provider rewindable iterators for non pdo drivers we should add something to do the same for pdo drivers? If so, I can take a look at doing that.

Ocramius · 2017-06-23T08:44:43Z

we should add something to do the same for pdo drivers?

At least looking at what is feasible first.

Rewindable iterator with emulation means memory usage.
Rewindable iterator should NOT perform a query a second time.

jenkoian · 2017-06-24T15:41:57Z

@Ocramius I've committed an example of a PDO iterator where we store a cache and can be rewound. Just wanted to remind ourselves of what the PR intentions were and where we're at now.

Initial: Non PDO drivers were running out of memory when iterating over large data sets due to fetchAll() being used and essentially just shoving all results into memory. A generator was added to MysqliStatement (with a view to roll out to the other non PDO drivers too) to solve this issue.

Iterator couldn't be reiterated or couldn't be rewound. So a RewindableGenerator was added so that the iterator could be rewound and thus iterated over multiple times.

PDO couldn't be reiterated noticed after adding an iterator that could be reiterated (rewound) for non PDO drivers that PDO drivers could not actually be reiterated, not sure if this is a bug or by design.

Latest I've added an example of a PDOStatementIterator which basically wraps PDOStatement and makes it reiterable. This is done via a static cache (basically memoization) which means that if large data sets are iterated over they will likely fill up memory.

So...we're basically gone from non PDO drivers running out of memory when iterating over large data to PDO drivers now running out of memory when iterating over large data sets 😆

Anyway, let me know how you wish to proceed 😄

Ocramius · 2017-06-24T20:15:15Z

@jenkoian the alternative is that we make them ALL non-rewindable, and that's it. That means that the user is left the decision to call iterator_to_array() or equivalent, and potentially having the application run out of memory.

Thoughts?

jenkoian · 2017-06-25T20:15:46Z

Options as I can see it are:

a) Consistent, BC Break
Make both PDO and NPDO (non PDO) rewindable, this is consistent but breaks BC for PDO as previously large data sets iterated over wouldn't suffer OOM.

b) Consistent, doesn't break BC
Make NPDO iterable without risk of OOM but not rewindable. Leave PDO as non rewindable.

c) Inconsistent, doesn't break BC
Make NPDO iterable without risk of OOM and rewindable. Leave PDO as non rewindable.

If you're looking for consistency I would say option b would be the best choice. IMO I think option c is the best and inconsistency here can be tolerated due to shortcoming of PDO.

Ocramius · 2017-07-07T12:20:30Z

b) Consistent, doesn't break BC
Make NPDO iterable without risk of OOM but not rewindable. Leave PDO as non rewindable.

Fairly sure we need this scenario then: can't afford OOM

…ons. Only added to mysqli for now, can add to other drivers (assuming they support reseting the pointer in some way) after feedback.

statements in a rewindable iterator. Skipped the test for iterating multiple times with a message regarding that iterators are non rewindable.

morozov

Only a few minor issues. Looks good overall.

morozov · 2017-09-15T00:28:50Z

tests/Doctrine/Tests/DBAL/Driver/StatementIteratorTest.php

+    public function testGettingIteratorDoesNotCallFetch()
+    {
+        $stmt = $this->createMock(Statement::class);
+        $stmt->expects($this->never())->method('fetch');


I'd also add all other fetch* methods here (fetchAll(), fetchColumn()).

morozov · 2017-09-15T00:29:38Z

lib/Doctrine/DBAL/Driver/OCI8/OCI8Statement.php

@@ -20,6 +20,8 @@
 namespace Doctrine\DBAL\Driver\OCI8;

 use Doctrine\DBAL\Driver\Statement;
+use Doctrine\DBAL\Driver\StatementIterator;
+use \IteratorAggregate;


The leading namespace separator is redundant.

morozov · 2017-09-15T00:31:41Z

tests/Doctrine/Tests/DBAL/Driver/StatementIteratorTest.php

+
+    public function testIterationCallsFetchOncePerStep()
+    {
+        $values = ['foo', '', 'bar', '0', 'baz', 0, 'qux', null, 'quz', false];


We need to add one more value after false to prove that fetching false terminates iteration, while other empty values don't (see original example).

jenkoian · 2017-09-15T07:23:39Z

Thanks @morozov - minor issues resolved. Anything else I need to do, or can we get this in? (I will rebase to a single commit once happy this is done) @Ocramius

Ocramius

Excellent work! It took a while, but we moved from a complex implementation to a really simple, clean and maintainable one. Thanks @jenkoian and @morozov!

Ocramius · 2017-09-15T17:20:15Z

🚢

The methods has more limitations and caveats than provides real use: 1. It fetches all data in memory which is often inefficient (see doctrine#2718). 2. It fetches rows in memory one by one instead of using `fetchAll()`. 4. It doesn't allow to specify the statement fetch mode since it's instantiated internally. 5. It can be easily replaced with: ```php foreach ($conn->executeQuery($query, $params, $types) as $value) { yield $function($value); } ```

jvasseur reviewed May 19, 2017

View reviewed changes

jenkoian force-pushed the efficient-iterating branch from 57522c0 to 790a7ec Compare May 19, 2017 16:20

Ocramius suggested changes May 20, 2017

View reviewed changes

Ocramius suggested changes Jun 1, 2017

View reviewed changes

jenkoian force-pushed the efficient-iterating branch from b078876 to 926c94c Compare June 23, 2017 08:00

Ocramius mentioned this pull request Jun 25, 2017

Batch Processing ,out of memory doctrine/orm#6525

Closed

Ocramius added the Improvement label Jun 25, 2017

Ian Jenkins added 7 commits September 14, 2017 08:32

Add rewindable generator in order fix the issue with mutliple iterati…

689200e

…ons. Only added to mysqli for now, can add to other drivers (assuming they support reseting the pointer in some way) after feedback.

Check query is only run once during iteration.

abbedec

Compare two timestamps to ensure only one query is run when iterating.

0ebafdb

An example of a PDOIterator which can be rewound .

3505a5e

Removed rewindable non pdo driver support and the wrapping of pdo

7cd962e

statements in a rewindable iterator. Skipped the test for iterating multiple times with a message regarding that iterators are non rewindable.

Use a spy to check fetch is called appropriate number of times

385cbb7

Refactoring tests

22b2b6b

jenkoian force-pushed the efficient-iterating branch 2 times, most recently from 21e1740 to 581511e Compare September 14, 2017 07:39

morozov reviewed Sep 15, 2017

View reviewed changes

Extract statement iterator into own class

078d32e

jenkoian force-pushed the efficient-iterating branch from 581511e to 078d32e Compare September 15, 2017 07:22

morozov approved these changes Sep 15, 2017

View reviewed changes

Ocramius self-assigned this Sep 15, 2017

Ocramius approved these changes Sep 15, 2017

View reviewed changes

Ocramius added Performance Prepared Statements labels Sep 15, 2017

Ocramius merged commit a0cc581 into doctrine:master Sep 15, 2017

jenkoian deleted the efficient-iterating branch September 16, 2017 10:26

morozov mentioned this pull request Apr 20, 2018

Portability Statement still fetches all data in iterator mode #3114

Closed

morozov mentioned this pull request Apr 29, 2018

SQLSrv (and other non PDO drivers) iterator inefficient #2705

Closed

morozov mentioned this pull request Nov 15, 2019

add sql type parameter to connection project function #3703

Closed

morozov mentioned this pull request Jan 16, 2020

Removed Connection::project() #3823

Merged

morozov mentioned this pull request Jun 7, 2020

Changing the position of the emptied variable #3997

Closed

github-actions bot locked as resolved and limited conversation to collaborators Aug 14, 2022

Add iterators for non pdo drivers. #2718

Add iterators for non pdo drivers. #2718

Conversation

jenkoian commented May 8, 2017 • edited Loading

jvasseur commented May 17, 2017

jenkoian commented May 17, 2017

jvasseur commented May 17, 2017

jenkoian commented May 17, 2017

jenkoian commented May 17, 2017

deeky666 commented May 17, 2017

Ocramius commented May 17, 2017 via email

morozov commented May 18, 2017

Ocramius commented May 18, 2017

deeky666 commented May 18, 2017

Ocramius commented May 18, 2017

deeky666 commented May 18, 2017

Ocramius commented May 18, 2017

deeky666 commented May 18, 2017

jenkoian commented May 19, 2017

jenkoian commented May 19, 2017

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ocramius left a comment

Choose a reason for hiding this comment

Ocramius commented Jun 23, 2017

jenkoian commented Jun 23, 2017

Ocramius commented Jun 23, 2017

jenkoian commented Jun 24, 2017

Ocramius commented Jun 24, 2017

jenkoian commented Jun 25, 2017

Ocramius commented Jul 7, 2017

morozov left a 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

jenkoian commented Sep 15, 2017 • edited Loading

Ocramius left a comment

Choose a reason for hiding this comment

Ocramius commented Sep 15, 2017

jenkoian commented May 8, 2017 •

edited

Loading

jenkoian commented Sep 15, 2017 •

edited

Loading