db_query_temporary() fails to create a table when the SQL has leading whitespace

The cause was fairly obvious, so I've rolled a patch for 8.x-dev.

I note that there is also the potential issue of leading comments in the SQL, which would also break this functionality, so ideally would also be accounted for. I don't know if there's existing code in Drupal for removing comments from raw SQL, though? I didn't spot anything obvious.

Perhaps it's reasonable to assume that we do want to retain anything prior to the first SELECT we find, and copy it verbatim into the modified query? That would account for both comments and whitespace.

Something like:

preg_replace('/^.*?SELECT/is', 'CREATE TEMPORARY TABLE {' . $tablename . '} AS \0', $query)

Here a patch for the second approach. This approach is working nicely with PostgreSQL 9.2beta2 (using the equivalent D7 patch).

And here's #3 with a test included. I don't have a D8 dev site at present, so I'll see what the test bot thinks of it.

Actually, that's still going to break if there's a leading comment containing the string "select". It's an improvement on the existing behaviour, though.

Edit: Come to think of it, the whole search & replace approach might be flawed?

The original code made the assumption that the query would start with a 'SELECT' statement. If that's a valid assumption, then presumably we can just use the replacement text up to (but not including) 'SELECT' as a prefix, concatenate the original SQL to that, and not bother with preg_replace() at all?

We'd still be making the same essential assumption about the SQL that the original code made, but without the additional constraints that the search+replace was imposing.

Edit 2: Ah, in fact the presence of the word "select" in a leading comment doesn't break the version in #4 as I initially thought (as the remainder of the comment and the real SELECT will still appear), but that exact behaviour definitely isn't the intent of the code, and it emphasises that we really just want to be concatenating the strings.

I've just looked back through the Drupal 5 commit history to see if there was any documented reason for the way this was done.

The code hasn't really changed much since it was introduced in commit 909d6928acb47cb9b50740e589b5e7447353e19c, but I believe we can see the source of the issue there, as initially Steven had thought that the Postgres syntax for CREATE TEMPORARY TABLE omitted the SELECT keyword, and so in the original version the preg_replace() for the postgres case was removing that keyword.

That turned out to be a mistake (see #36255: Postgres problem with db_query_temporary()), and the SELECT keyword for postgres was restored, which seems to render the preg_replace() approach redundant (for all databases).

It was left like that, though, and it's been that way ever since.

Updated issue summary.

Dan: I'm not sure anyone else is looking at this, unfortunately.

I spent enough time investigating this at the time, that I'm pretty happy with the patch and the reasoning behind it.

If you've actually reviewed the code (it's not clear to me from your "simple patch" comment), you could please go ahead and update the issue status to RTBC?

It's an annoying bug, and the cause isn't at all obvious when it occurs, so it would be nice to get some extra eyes on it, and get this fix committed!

6: drupal-8.x-db_query_temporary_whitespace-1868972-6.patch queued for re-testing.

Ugh, annoying. That's actually a test PASS (exactly as it used to do). The failure was:

59,755 pass(es), 1 fail(s), and 0 exception(s)

Non-pass

Test name Pass Fail Exception
Drupal\system\Tests\Theme\ThemeTest 61 1 0

Message Group Filename Line Function Status
The test did not complete due to a fatal error. Completion check ThemeTest.php 263 Drupal\system\Tests\Theme\ThemeTest->testFindThemeTemplates()

(which is utterly unrelated to this patch).

Thank you Dan, that's excellent. Hi chx :)

Wow... 12 hours for that to get through the test queue.

The functional changes are all good -- and it'll be a similarly trivial backport to D6 -- but I've edited the test for consistency, so that in D7 we're querying {system} (as per the other temporary table tests), rather than the {test} table used in the D8 tests (the testbot didn't complain about it, but it just looked wrong).

Hopefully the test queue is smaller by now :)

I can see now that querying {test} is 100% fine in D7 as well, so I'm just setting this back to RTBC. #18 is good to commit.

#18 is still ready and awaiting a 7.x commit.

If it helps to hurry this along, can I just reiterate that the essentials of the patch are identical for all of 8.x, 7.x, and 6.x (because this code is essentially unchanged since 5.x), so this (#18) really is good to go.

I don't know whether to keep bumping this in the hopes that a Drupal 7 maintainer notices it, or if it's going to get processed in due course and I should just leave it be. It's impossible for me to tell given the lack of response, so I really don't know what to do about it -- I don't want it to fade from view entirely.

It's doubly frustrating because the change has already been committed to 8.x and the exact same change is reviewed and tested in 7.x, so I feel that this is an absolute no-brainer.

18: drupal7x-db_query_temporary_whitespace-1868972-18.patch queued for re-testing.

Re-submitted the correct RTBC patch for 7.x to the test bot, because bzrudi71 re-submitted the already-committed 8.x patch after we'd switched to 7.x, which left a test failure message in the more recent comments.

Thank you David (and for that very useful info on the D7 patching process!).

D6 version.