Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
When used with a forward-only cursor (the only type of cursor implemented by PDO/MySQL), PDOStatement can never be rewinded. So:
$result = db_query("SELECT * FROM {user}");
foreach($result as $user) {
break;
}
foreach ($result as $user) {
print_r($user);
}
Will output all the users, except the first one.
This is not consistent with what we are doing in DatabaseStatementPrefetch, and that currently breaks the SQLite implementation.
Comment | File | Size | Author |
---|---|---|---|
#26 | 342693-fix-prefetch-cursor.patch | 9.82 KB | Damien Tournoud |
#20 | 342693-fix-prefetch-cursor_0.patch | 9.61 KB | webchick |
#7 | 342693-norewinditerator-approach.patch | 438 bytes | sdboyer |
#3 | 342693-fix-prefetch-cursor.patch | 9.61 KB | Damien Tournoud |
#1 | 342693-fix-prefetch-cursor.patch | 9.52 KB | Damien Tournoud |
Comments
Comment #1
Damien Tournoud CreditAttribution: Damien Tournoud commentedHere is a patch.
Comment #2
John Morahan CreditAttribution: John Morahan commentedI get fails in the sqlite search and user tests with this (plus #342366: SQLite badly replaces unamed placeholders without which sqlite currently doesn't even install properly) - they all seem to be in ordering tests, so I guess they are probably related to this patch rather than the other one?
Comment #3
Damien Tournoud CreditAttribution: Damien Tournoud commentedIn a nutshell, I tried a micro-optimization, and the first row was moved last in the result set... That should solve it.
Comment #4
Damien Tournoud CreditAttribution: Damien Tournoud commentedComment #5
John Morahan CreditAttribution: John Morahan commentedI don't understand this well enough to comment on whether it makes sense, but I can at least confirm that with this patch SQLite works again and all tests pass.
Comment #6
Damien Tournoud CreditAttribution: Damien Tournoud commentedRan all patches.. 100% pass.
Comment #7
sdboyer CreditAttribution: sdboyer commentedForgive me for saying the obvious, but this is why there's a NoRewindIterator, and I wonder if using it (vs. userland) wouldn't be faster. I kinda assume you guys have already looked and and dismissed the possibility, but I figured I'd give it a crack just in case.
I looked through, though, and I don't grok the db layer well enough (and sqlite hangs when I try to load anything other than the front page on my system, so stepping through to learn it isn't an option) to know where to tuck one in intelligently. The only thought I had, is a generic approach - just do it in the constructor for DatabaseStatementPrefetch:
When I try it, the frontpage still loads like a champ, at least! Given that I dunno how the statement objects get passed around and used, though, I really don't know if this takes care of your problem or not. Patch attached just in case, though, so that the testbot can have a crack at it.
Comment #8
Damien Tournoud CreditAttribution: Damien Tournoud commented@sdboyer: since when can you return something from a __construct? I think your code simply does nothing.
What you could do is implement IteratorAggregate and in getIterator() return a new NoRewindIterator($this);... but wrapping an Iterator around an Iterator adds several level of indirection.
Comment #9
sdboyer CreditAttribution: sdboyer commentedYeah...since never, it seems. lol@me. It felt like it was kinda suspect when writing it, but I felt like I'd seen it done somewhere before. Evidently not :P
Yeah, that'd be the way to do it. I think the extra layer of indirection might be just fine though, primarily because the SPL iterator system was specifically designed to layer iterators within iterators in just this sort of way. In fact, that's why the
NoRewindIterator
extendsIteratorIterator
, not just implementingIterator
- it's an iterator for iterators. From SPL's perspective, using NoRewindIterator would probably be the 'correct' way of doing it, although that's a pretty fuzzy line when we're effectively designing our ownforeach
statements. I dunno if a performance benchmark would have anything substantive to say, but since I can't run it on my system properly, I can't say anything more substantive.Comment #10
John Morahan CreditAttribution: John Morahan commented@sdboyer: can you still not get past the front page even if you apply #3 and reinstall? I can.
Comment #11
sdboyer CreditAttribution: sdboyer commented@John Morahan: Nope, I can't seem to get past the frontpage on a clean install even without Damien's patch is applied.
Comment #12
John Morahan CreditAttribution: John Morahan commented@sdboyer: right, neither can I. But how about with Damien's patch applied?
Comment #13
sdboyer CreditAttribution: sdboyer commentedWow. That's two idiotic things in a row. I'll try to not go for three here.
Yes, with the patch applied, it works.
Comment #14
John Morahan CreditAttribution: John Morahan commentedLOL! Changing the title and priority to make it more obvious that SQLite is currently totally broken and this patch fixes it.
Comment #15
Damien Tournoud CreditAttribution: Damien Tournoud commented@sdboyer: I may not have been clear enough. The patch in #3 is the way to go, because it allows us to free the memory used from the result set as soon as possible. There is nothing in SPL that would allow us to do that. Plus its simple, clear and well contained.
So please, one of you two mark this as RTBC, so that we can all move on to something else.
Comment #16
John Morahan CreditAttribution: John Morahan commentedSure. RTBC on #3. It looks sane as far as I understand it, and it's definitely better than what's there now :)
Comment #17
sdboyer CreditAttribution: sdboyer commentedAhh yeah. I wasn't thinking about memory consumption at all. I guess that counts as stupid thing #3 - so triple apologies.
+1
Comment #18
chx CreditAttribution: chx commentedYeah #3 is fine.
Comment #19
Dave ReidYay this fixed the problem with sqlite on my install.
Comment #20
webchickRe-posting patch in #3 in the hopes that testing bot will come back with greens.
Comment #21
Dave ReidResults from testing bot are all pass - testing bot is not reporting back here on testing results
http://testing.drupal.org/pifr/file/1/2217
Comment #22
Crell CreditAttribution: Crell commentedFrom visual inspection of #20.
I don't really grok what that line is doing... We're merging an array with itself non-destructively? How is that not a no-op?
For the rewind() method, should we perhaps return NULL explicitly? I'm OK with not bothering with an exception here, but it would be more self-documenting I think to explicitly return NULL rather than implicitly.
Grammar nazi: The colon should be a semi-colon, and the past tense of rewind is "rewound". (Although I could see the point debated here as it is the past tense of a method name, not the past tense of the English verb that method represents. I can live with it either way, but my brain parse errored on "rewinded".)
I don't quiet get the "take the first row out, then put it back in, then take it back out again" dance. If that's necessary OK, but I am not clear on why we are doing so.
Comment #23
Dries CreditAttribution: Dries commentedLet's address Crell's review.
Comment #24
John Morahan CreditAttribution: John Morahan commentedI can answer the easy ones :p
"We're merging an array with itself non-destructively? How is that not a no-op?" - array_values() produces an array indexed by numeric keys, which is then merged with the original associative array to satisfy PDO::FETCH_BOTH. Do you think this needs a comment? Or perhaps it would be more obvious if the PDO::FETCH_NUM case was moved above it?
I initially thought "rewinded" was OK as the past tense of the method name, but on reflection, method names don't really have a past tense. So yes this should change.
Comment #25
Crell CreditAttribution: Crell commentedAh! That's what it's doing! OK, that makes perfect sense now that I understand it, which is a sure-fire sign that it needs a comment, IMO. :-)
Incidentally I think there will still be a slight difference here, because the PHP FETCH_BOTH structure alternates between key and numeric values, I believe, while this code would put all keyed entries first and then all indexed values. That means if you try to iterate the record you'll get a different order. I don't think that's a problem, as if you're iterating a FETCH_BOTH record you're doing something very stupid anyway and I've frankly never even understood what use FETCH_BOTH has, and Drupal doesn't use it anywhere, but I am just mentioning it for posterity as a known non-issue. :-)
Comment #26
Damien Tournoud CreditAttribution: Damien Tournoud commentedGosh. I can't believe I rerolled that patch for this. I'm really too kind.
Next time, please read the manual.
PDO::FETCH_BOTH is described as "returns an array indexed by both column name and 0-indexed column number as returned in your result set". The contract doesn't mandate any particular order. So if there is a bug, it's in the consumer code, not here.
Comment #27
Dave ReidPatch and comments all looked good. This made all the db tests running on sqlite happy!
Comment #28
Dries CreditAttribution: Dries commentedCommitted to CVS HEAD. Thanks.