subselect in WHERE condition with multiple placeholders cause invalid/duplicate placeholders
hass - August 31, 2009 - 23:03
| Project: | Drupal |
| Version: | 7.x-dev |
| Component: | database system |
| Category: | bug report |
| Priority: | critical |
| Assigned: | mcarbone |
| Status: | closed |
| Issue tags: | Release blocker |
Description
If I run the below code a wrong SQL statement is build having two times the same placeholder executed:
<?php
$subquery = db_select('linkchecker_link', 'll');
$subquery->fields('ll', array('lid'));
$subquery->condition('ll.urlhash', array_map('md5', $links), 'IN');
$query = db_delete('linkchecker_comment');
$query->condition('cid', $cid);
$query->condition('lid', $subquery, 'NOT IN');
$query->execute();
?>Same placeholders used twice (devel output):
DELETE FROM linkchecker_comment WHERE (cid = :db_condition_placeholder_0) AND (lid NOT IN (SELECT ll.lid AS lid FROM linkchecker_link ll WHERE (ll.urlhash IN (:db_condition_placeholder_0)) )) Debug on MySQL level (second placeholder have a wrong value):
DELETE FROM linkchecker_comment WHERE (cid = '1') AND (lid NOT IN (SELECT ll.lid AS lid FROM linkchecker_link ll WHERE (ll.urlhash IN ('1')) ))For easier reading the D6 version:
<?php
db_query"DELETE FROM {linkchecker_comments} WHERE cid = %d AND lid NOT IN (SELECT lid FROM {linkchecker_links} WHERE token IN (" . db_placeholders($links, 'varchar') . "))", array_merge(array($cid), array_map(md5, $links)));
?>
#1
Only as a note. I've tested with MySQL 5.x
#2
I suspect this is a side effect of #496500: Remove global placeholder counter. Can you check to see if the outer delete query is getting passed to the subselect getArguments() method properly?
#3
You are asking for something I do not fully overlook... Where do I need to add a krumo() line? :-)
#4
@Crell: Are you there :-) ??? I'd like to develop modules for D7... this bug hardly blocks development and I do not understand how to debug/solve :-(
Critical for release as data get's lost and DB API is broken.
#5
hass, can you provide a unit test that fails from this bug? The provided unit tests include a fair amount of sample data you can work from. If you can replicate it from that, we should be able to track it down and squish it.
#6
Crell: how about this test? seems to show the bug for me. (testing this because i want subselects to be robust when porting the chatroom to D7.)
#7
The last submitted patch failed testing.
#8
Subscribing, I have also run into this "subselect" issue when porting the biblio module to D7.
#9
Here is a patch that addresses this problem and passes justinrandell's test. I decided to use this issue as a way for me to learn about the DBTNG backend and PDO in general, so apologies for neglecting the Assigned field, as I wasn't sure if I could take it until I got knee-deep.
The only way I could think of doing this was to add getter and setter functions for the placeholder variable, as we need to provide the select subquery with an updated placeholder count, and then pass it back to the master query. Technically, this could also make the nextPlaceholder function irrelevant, but I think it's still useful as a lean function. Also, I'm not sure if I'm following standards with the names of the new abstract functions.
#10
Hm. What confuses me is that select and update queries were fixed a long time ago by passing a placeholder generating object along the compile chain. Delete queries are doing the same, so I don't get why it's breaking here. I don't think the extra methods added by the patch in #9 should be necessary. Unfortunately my debugger is being cranky and not letting me debug properly. :-( Still investigating.
#11
I found the code you are referring to, but it's actually addressing another issue -- using a subquery as a table ("INNER JOIN [subquery] AS s"), rather than using a subquery in a condition ("WHERE [subquery] > :val"). As far as I can tell, without the above patch, the latter kind of subquery is broken on selects, updates, and deletes - and has been since the removal of global placeholders.
#12
And in fact, it appears as if delete and update queries don't allow joins at all, which means that the getArguments solution isn't even needed here. I'm assuming there was a previous community decision to eschew joins in delete and update queries? It seems like more than a year ago it was determined that it didn't need to be supported since since SQLite doesn't support them: http://drupal.org/node/225450#comment-781151 -- Not sure if that was the end of the discussion.
#13
Joins in UPDATE and DELETE are MySQL-specific extensions, so no we don't support them. However, subqueries in WHERE clauses *should* work cross-DB so those are supposed to work properly. Hence this issue being a legit bug. :-)
But we do have unit tests for WHERE subselects on Select queries already, don't we? Do those work? If not, then this is a bigger bug than just DELETE. :-)
#14
This is indeed a bigger bug than just DELETE, as the problem is in DatabaseCondition. There is a test (testConditionSubquerySelect) that's supposed to cover this for SELECT queries, but it only uses one placeholder so it didn't run into this problem.
#15
So like any good tricky bug, the actual fix is one line of code that took several hours to track down. :-)
The problem is that in order to keep placeholders consistent within dependent queries, we pass the outer-most query object through the entire compilation chain, no matter how many layers deep it recurses. That should work for all queries.
However! Long story short, in the specific case of a select query in a DatabaseCondition object (aka WHERE or HAVING clauses) the object wasn't getting passed through. Thus, the subquery thought it was the top-most object and used itself as the placeholder object, and when it then passed itself to its DatabaseCondition dependent objects (its own WHERE clause), the placeholder counter was different, and so we got 0 twice. Fail!
The solution is a single extra line to pre-compile the subselect object before we cast it to a string, into which we are then able to pass the placeholder object. And then the bug goes away.
While at it, though, I discovered the line right above it had a coding style error. It was doing an instanceof check on a class rather than an interface. So I fixed that, too. I also added a comment to an earlier code block a few lines away because I spent 10 minutes wondering if that's where the bug was because I forgot that no, it's processed elsewhere. :-)
Whew! I think we got this one. Thanks all. bot, what do you think?
#16
looks good to me. tiny nitpick, an extra blank line is introduced in the patch, but can probably just be taken out when its being committed. RTBC.
#17
Thank you very much for spending time on this issue! I will do my tests later. Only as a note:
+++ modules/simpletest/tests/database_test.test 2009-11-23 06:43:13 +0000@@ -945,6 +945,27 @@ class DatabaseDeleteTruncateTestCase ext
+ $num_records_before = db_query('SELECT COUNT(*) FROM {test_task}')->fetchField();
I thought we should only use
COUNT(1)as it's expensive under InnoDB, but maybe not a rule for tests...#18
I thought I removed some blank lines in this patch... :-)
@hass: Yeah, I'm not really worried about SQL micro-optimization in tests. For production code it may matter, but not for tests.
#19
Simplicity wins -- very nice.
#20
It was a joy to read up on this issue. Great team work. Committed to CVS HEAD.
#21
Automatically closed -- issue fixed for 2 weeks with no activity.