The docblock of db_query_temporary makes zero sense:

 * @param $tablename
 *   The name of the temporary table to select into. This name will not be
 *   prefixed as there is no risk of collision.

There is a risk of collision in at least three cases:

* two modules could want to use the same $tablename
* some db engines doesn't support temporary tables and will emulate them using real tables. In that case, two concurrent connections could want to create temporary tables.
* in case of connection pooling (for example pgpool on front of postgresql), PHP runners can share the same connection to the database, so temporary tables could persist and name collision can occur

I suggest we change db_query_temporary() so that $table_name is only a hint, and the real table name used be returned by that function. Thoughts?

CommentFileSizeAuthor
#14 349500-db-query-temporary-alias.patch9.84 KBdamien tournoud
#10 349500-db-query-temporary-alias.patch9.8 KBdamien tournoud
#4 349500-db-query-temporary-alias.patch5.69 KBdamien tournoud
#3 349500-db-query-temporary-alias.patch0 bytesdamien tournoud

Comments

Crell’s picture

Crell commented

This makes sense to me. It fits the way field and table aliases work, and safety is generally a good thing. Let's do. :-)

Crell’s picture

Crell commented

Sounds like a possible duplicate of #279041: Temporary Table Creation Should First Check if Table Exists.

damien tournoud’s picture

damien tournoud commented
Status: Active » Needs review
StatusFileSize
new 349500-db-query-temporary-alias.patch0 bytes

Here is a patch for discussion.

damien tournoud’s picture

damien tournoud commented
StatusFileSize
new 349500-db-query-temporary-alias.patch5.69 KB

Let's try again.

stewsnooze’s picture

stewsnooze
he / him
commented
Status: Needs review » Active

I had thought of something simple like this. In most cases the caller would actually get the tablename they requested.

Index: includes/database/database.inc
===================================================================
RCS file: /cvs/drupal/drupal/includes/database/database.inc,v
retrieving revision 1.35
diff -u -p -r1.35 database.inc
--- includes/database/database.inc	20 Dec 2008 18:24:33 -0000	1.35
+++ includes/database/database.inc	21 Dec 2008 23:49:11 -0000
@@ -1611,8 +1611,11 @@ function db_query_temporary($query, $arg
     array_shift($args);
   }
   list($query, $args, $options) = _db_query_process_args($query, $args, $options);
-
-  return Database::getActiveConnection($options['target'])->queryTemporary($query, $args, $tablename, $options);
+  while (db_table_exists($tablename)) {
+    $tablename .= '1';
+  }
+  $data = Database::getActiveConnection($options['target'])->queryTemporary($query, $args, $tablename, $options);
+  return $tablename;
damien tournoud’s picture

damien tournoud commented

Who cares about the table name? And oups, I probably forgot to update the tests.

damien tournoud’s picture

damien tournoud commented
Status: Active » Needs work
stewsnooze’s picture

stewsnooze
he / him
commented

I was working on this for a while today.

I think that the tests in testTemporaryQuery() would be flawed if we implemented this patch. Effectively we would need to make sure that creating two temporary tables will not clash.

I ended up actually doing the calls to db_query_temporary() in testTemporaryQuery() rather than in a drupalGet as I needed the temporary tables to stick around for a while to make sure they both existed. I think your patch has stronger legs than mine but we need to make sure the tests check properly for the new functionality

stewsnooze’s picture

stewsnooze
he / him
commented

Also I'm pretty new here!

damien tournoud’s picture

damien tournoud commented
StatusFileSize
new 349500-db-query-temporary-alias.patch9.8 KB

@stewsnooze: welcome, it's great to see new core contributors :)

Here is actually a working patch. I extended the tests to check that two temporary tables can be created in the same request.

damien tournoud’s picture

damien tournoud commented
Status: Needs work » Needs review

Let's see if the test bot wants to pick it up.

Crell’s picture

Crell commented

The rest of the DB layer uses keyword_$i, not keyword$i for auto-generated strings such as placeholders. Let's do that here, too. Also, should we perhaps use db_temporary_$i, since we've already reserved the db_ namespace?

Otherwise, this looks good to me on visual inspection. Forcing people to pass around the table name themselves is more loosely coupled than passing a name in in the first place, so +1 on just standardizing on that form. A quick reroll and we should be able to commit this.

Crell’s picture

Crell commented
Status: Needs review » Needs work
damien tournoud’s picture

damien tournoud commented
Status: Needs work » Needs review
StatusFileSize
new 349500-db-query-temporary-alias.patch9.84 KB

Here we are, reroll per Crell comments in #12.

Crell’s picture

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

I like it, and so does the bot. :-)

The next step should probably be to finish off #332353: [DBTNG + simpletest] missing db_select()->temporary() so that we have parity between static and dynamic queries.

dries’s picture

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

And so do I. Committed to CVS HEAD. Thanks DamZ. Onto #332353 now. :-)

Status: Fixed » Closed (fixed)

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