In a contrib module (Search by Page) I am running into SQL errors when I run the tests on a Drupal installation running PostgreSQL 8.4. It appears to me that the Postgre query builder is incorrectly using the same placeholders for two different parts of the query.

I will see if I can devise both a test and a fix for this, based on the query my contrib module is doing. It's (obviously) a rather complex query...

Here's the error:

SQLSTATE[22P02]: Invalid text representation: 7 ERROR: invalid input syntax for integer: "en" LINE 10: ...sbpa_sq.objtype = '2') AND (sbpa_sq.environment = 'en') AND ... ^:

Here's the query the query builder created. Note that db_condition_placeholder_1 through 3 are used twice (e.g. placeholder 1 is for the environment value the first time, an integer, and for the language the second time, a string 'en'):

SELECT SUM(i.score * t.count) AS calculated_score FROM {search_index} i INNER JOIN {sbp_path} sp ON i.sid = sp.pid LEFT OUTER JOIN (SELECT DISTINCT sbpa_sq.sbpaid AS sbpaid FROM {sbpa_attachments} sbpa_sq LEFT OUTER JOIN {node} sbpa_n ON sbpa_sq.objid = sbpa_n.nid INNER JOIN {node_access} na ON na.nid = sbpa_n.nid WHERE (sbpa_sq.objtype = :db_condition_placeholder_0) AND (sbpa_sq.environment = :db_condition_placeholder_1) AND (sbpa_n.status = :db_condition_placeholder_2) AND (sbpa_sq.display = :db_condition_placeholder_3) AND(( (na.gid = :db_condition_placeholder_4) AND (na.realm = :db_condition_placeholder_5) )OR( (na.gid = :db_condition_placeholder_6) AND (na.realm = :db_condition_placeholder_7) ))AND (na.grant_view >= :db_condition_placeholder_8) ) sbpa_a ON sbpa_a.sbpaid = sp.modid INNER JOIN {search_total} t ON i.word = t.word WHERE (sp.environment = :db_condition_placeholder_0) AND (sp.language = :db_condition_placeholder_1) AND( (0=1) OR( (sp.from_module = :db_condition_placeholder_2) ))AND( (i.word = :db_condition_placeholder_3) )AND (i.type = :db_condition_placeholder_4) GROUP BY i.type, i.sid HAVING (COUNT(*) >= :matches) ORDER BY calculated_score DESC LIMIT 1 OFFSET 0; Array ( [:db_condition_placeholder_0] => 2 [:db_condition_placeholder_1] => en [:db_condition_placeholder_2] => sbp_attach [:db_condition_placeholder_3] => flowers [:db_condition_placeholder_4] => search_by_page [:matches] => 1 [:db_condition_placeholder_5] => node [:db_condition_placeholder_6] => environment [:db_condition_placeholder_7] => 1 [:db_condition_placeholder_8] => 1 )

CommentFileSizeAuthor
#12 886970-placeholders.patch4.55 KBdamien tournoud
#9 886970-placeholders.patch4.31 KBCrell
#7 886970-7.patch3.71 KBjhodgdon
#3 886970-fail-test.patch2.71 KBjhodgdon

Comments

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented

May be related to #564852: Subselect with placeholders causes invalid/duplicate placeholders

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Title: PostreSQL putting wrong db placeholders in complex queries » DB API putting wrong db placeholders in complex queries
Component: postgresql database » database system

Apparently this is not specific to Postgres. Investigating further... it may be specific to the search extender...

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Status: Active » Needs review
StatusFileSize
new 886970-fail-test.patch2.71 KB

OK.

It seems to be related to adding a node_access tag to a rather complex query. I am still not sure why/where in the code it is happening, but I have devised a test for it.

If you run the test interactively you can see the query string that is generated. Note: I'm not sure the query will execute, and I know the query makes no sense, but the point is that the db placeholders are repeated and they shouldn't be.

damien tournoud’s picture

damien tournoud commented

The problem is that the preExecute() of the subquery is called *after* (during __toString()) the call to getArguments() which compiles everything.

Because of this, if a alter hook (which happens during preExecute()) changes the conditions of the subquery, that condition has to be rebuilt by __toString(), which fails because it uses the wrong placeholder source.

The fix should be simple: move the call to preExecute() of *all* the subqueries in the preExecute() of the parent query, right after the parent query has been altered.

Status: Needs review » Needs work

The last submitted patch, 886970-fail-test.patch, failed testing.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Assigned: Unassigned » jhodgdon

Damien: really? Here's SelectQuery::execute():

 public function execute() {
    // If validation fails, simply return NULL.
    // Note that validation routines in preExecute() may throw exceptions instead.
    if (!$this->preExecute()) {
      return NULL;
    }

    $args = $this->getArguments();
    return $this->connection->query((string) $this, $args, $this->queryOptions);
  }

Besides if you look at the test case I created, it does preExecute() then getArguments() then a string cast (same order as execute() here).

Ahhhh.

The sub-query's preExecute() is not called until the toString though. This is from SelectQuery::_toString():

     // If the table is a subquery, compile it and integrate it into this query.
      if ($table['table'] instanceof SelectQueryInterface) {
        // Run preparation steps on this sub-query before converting to string.
        $subquery = $table['table'];
        $subquery->preExecute();
        $table_string = '(' . (string) $subquery . ')';
      }

So I think what we need to do is to iterate over tables and do the preExecute on all of them during the query's preExecute(), and we'll be fine. Probably needs to do that for unions too?

I'll take a go at a patch...

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Status: Needs work » Needs review
StatusFileSize
new 886970-7.patch3.71 KB

Here's a patch. It passes the included test on my test box. I'm going to run the Search by Page tests now and verify it fixes the original problem I noticed as well.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented

Update: With the patch in #7 and this other issue also patched
#886752: SQL error on PostgreSQL in comment.module
all of my contrib module's tests pass for both PostgreSQL and SQLite, as well as MySQL. So I'd appreciate it if we could get this one in.

Crell’s picture

Crell commented
StatusFileSize
new 886970-placeholders.patch4.31 KB

We discussed this at length at the Dev Summit and here's what we found/concluded. I'm documenting it here for posterity.

1) This is the correct fix here.
2) There is also a bug with WHERE subqueries having the same issue. However, it doesn't show up in practice because we are compiling conditions in getArguments(), where we are able to pass in a placeholder tracking object, before we compile them in __toString(), where we cannot. We get around the issue only because DatabaseCondition::compile() is a no-op the second time it's called. So while we were trying to make it so that you could call __toString() at any time and call getArguments() at any time, in practice that's not the case for SelectQueries (and only SelectQueries). That also cannot be fixed without moving away from __toString() as a compilation mechanism.
3) Moving away from __toString() as a compilation mechanism is too big of a change to make in Drupal 7, especially since the issue in #2 is an edge case that we are very unlikely to actually hit.

I have therefore added a comment to __toString() describing the above problem. We can try to address that in Drupal 8. Also I fixed an issue with the previous patch that was returning $this instead of $this->prepared.

This should be good to go, but I'll let Damien mark it. :-)

damien tournoud’s picture

damien tournoud commented
Status: Needs review » Reviewed & tested by the community

Looks good to me. The test bot can fail it if I missed something.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 886970-placeholders.patch, failed testing.

damien tournoud’s picture

damien tournoud commented
Status: Needs work » Reviewed & tested by the community
StatusFileSize
new 886970-placeholders.patch4.55 KB

And now as a proper patch :)

Crell’s picture

Crell commented

D'Oh! Stupid gid patches. Thanks, Damien!

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Priority: Normal » Critical

Just re-encountered this again.

Every query that uses node_access in combination with the default count query that uses a subselect is totally broken because of this. For example, views :)

Raising to critical, this really needs to be fixed and the patch is already there, approved and RTBC :)

BenK’s picture

BenK commented

Just need to keep track of this...

dries’s picture

dries commented
Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD. Thanks.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.