Bring back temporary tables (Connection::queryTemporary())

I get why your contrib module needs the temporary table functionality back. What you are now doing in the patch is making it a driver specific functionality. Only that is something that first to me needs framework managers approval. As do you want to have database driver specific functionality? Personally I do not like it very much.

Another solution is to create a contrib database driver for SQLite that extends the core one. That will be very easy to do, after #3256642: Introduce database driver extensions and autoload database drivers' dependencies has landed. That issue has been RTBC for 8 months and hopefully will go into D10.1. Only that will not fix the situation for D10.0. Maybe we could convince the core committers to have it land in D10.0.

Changing the priority to major as it is a contrib blocker.

There's related issue, so something should care about cleaning of this temporary tables

I think the idea of a contrib sqlite driver is bad. What would its namespace be? And does it automatically replace the core sqlite driver? Or do tests now need to be more specific.

Here's a comment from the CR:

We use it and kinda need it. We have some queries which require temporary tables otherwise they would never run within production constraints. Using a temp table means a result in a couple of seconds, without it we have a query which would take minutes. I don't mind things being deprecated and changed but completely dropping the functionality leaves people without a solution.

It seems useful regardless. I don’t understand how this blocked removing table prefixes. Search API with its database backend is broken without this in drivers.

Ok, lets do this. It is clear to me that we need this.

1. +++ b/core/modules/sqlite/src/Driver/Database/sqlite/Connection.php
   @@ -352,6 +359,22 @@ public function queryRange($query, $from, $count, array $args = [], array $optio
   +  public function queryTemporary($query, array $args = [], array $options = []) {
   

   Can we give this method a different name then the just removed method. Something like: sqliteCreateTemporaryTable.

2. +++ b/core/modules/sqlite/src/Driver/Database/sqlite/Connection.php
   @@ -478,4 +501,14 @@ public static function createUrlFromConnectionOptions(array $connection_options)
   +  protected function generateTemporaryTableName() {
   +    return "db_temporary_" . $this->temporaryNameIndex++;
   +  }
   

   Can we remove this method and move the code into the main function.

As I now the subsystem maintainer for the Database API and I am the one who has added the tag, I can remove the tag.

Ok, lets do this. It is clear to me that we need this. Only can we give this method a different name then the just removed method. Something like: sqliteCreateTemporaryTable.

Why is this only on SQLite? We need it for the other drivers as well.

Honestly this is nearly feasible without adding the method back to the drivers, if we could tell a statement to not process prefixes in $options.

I have the following sample code I am working on:

    static $temporary_name_index = 0;
    $db_type = $this->database->databaseType();
    $tablename = "db_temporary_" . $temporary_name_index++;
    if ($db_type === 'mysql') {
      $this->database->query('CREATE TEMPORARY TABLE {' . $tablename . '} Engine=MEMORY ' . $query, $args, $options);
    }
    elseif ($db_type === 'pgsql') {
      $this->database->query('CREATE TEMPORARY TABLE {' . $tablename . '} AS ' . $query, $args, $options);
    }
    elseif ($db_type === 'sqlite') {
      $this->database->query('CREATE TEMPORARY TABLE ' . $tablename . ' AS ' . $query, $args, $options);
    }
    else {
      throw new InvalidQueryException("Unable to create temporary table for database type $db_type");
    }
    return $tablename;

It is fine, expect for SQLite because I can't opt-out of the table being prefixed by statements.

Here is the final code I provided in #3313708: Provide replacement for queryTemporary.

SQLite always stores temp tables in the temp table. So it has nothing to do with Drupal's prefixing system. It just needs to be provided a fully qualified table name for temp tables, because it's in the temp database, but happens to pass through if not fully-qualified.

  private function queryTemporary($query, array $args = [], $options = []) {
    static $temporary_name_index = 0;
    $db_type = $this->database->databaseType();
    $tablename = "db_temporary_" . $temporary_name_index++;
    if ($db_type === 'mysql') {
      $this->database->query('CREATE TEMPORARY TABLE {' . $tablename . '} Engine=MEMORY ' . $query, $args, $options);
    }
    elseif ($db_type === 'pgsql') {
      $this->database->query('CREATE TEMPORARY TABLE {' . $tablename . '} AS ' . $query, $args, $options);
    }
    elseif ($db_type === 'sqlite') {
      $this->database->query('CREATE TEMPORARY TABLE ' . $tablename . ' AS ' . $query, $args, $options);
      // Temporary tables always live in the temp database, which means that
      // they cannot be fully qualified table names since they do not live
      // in the main SQLite database. We provide the fully-qualified name
      // ourselves to prevent Drupal from applying prefixes.
      // @see https://www.sqlite.org/lang_createtable.html
      $tablename = "temp.$tablename";
    }
    else {
      throw new InvalidQueryException("Unable to create temporary table for database type $db_type");
    }
    return $tablename;
  }

I think the 'for sqlite driver' is a mis-reading of a previous conversation, either way I don't think that's what's needed here - we'd just need to bring back the methods.

The original reason we deprecated the method was for two reasons:

1. No core usage and not much contrib usage (but this issue has demonstrated the contrib usage is significant enough to bring it back.

2. The sqlite implementation was doing weird things with db prefixes which were were also trying to remove.

This is what tied the original issue up with prefixes:

// SQLite requires that temporary tables to be non-qualified.
    $tablename = $this->generateTemporaryTableName();
    $prefixes = $this->prefixes;
    $prefixes[$tablename] = '';
    $this->setPrefix($prefixes);

It looks like @mglaman's code achieves the same thing without using the prefixes system, so in that case I think we could essentially revert #3211780: Deprecate Connection::queryTemporary() but with the different sqlite implementation to ensure the table is fully qualified.

@catch so roll an MR that is a revert of the removal, but modified SQLite code to always fully-qualify to temp.? I can do that!

edit: confirmed in Drupal Slack.

It was me removing this feature, and besides #11 my thinking was that the sheer concept of temp tables sounds strange in 2020 - we have other ways to store data temporarily and memory availability is orders of magnitude bigger now than when temp tables were invented to persist temp data on disk.

So for me (I admit I haven’t got in the details, please consider this), contrib should first think if the design using temp tables is still actual, or could be revisted. Can subselects be used instead? Or results be cached in arrays and accesssed in memory?

Search API tries to fallback on a subselect when doing work for facets. The autocomplete does not - not sure why.

I was going researching and it seems like a useful technique. Especially if it's not viable as a View, which we don't even support.

This is especially useful when the dataset is too large for in-memory storage and better CPU usage. The DBMS is more performant for a lot of these things than writing it in PHP.

If temporary tables are supported by the engines, why remove support? Especially if the only reasoning was the SQLite workaround for prefixes.

Just thinking aloud: core is likely not going to need this, right? So bringing it back adds maintenance burden and no focus since it's unused i core. And contrib may get later frustation if they need changes. Also, forces contrib db drivers to comply, and I for sure can say that Oracle is not so flexible here as currently core supported dbs, https://oracle-base.com/articles/misc/temporary-tables.

Core has a pluggable option here: backend overrideable services. Maybe contrib can implement such in their module and then manage their own stuff - more flexibility to contrib modules consuming temp tables, no core maintenance burden, no forcing of contrib drivers to comply to this API.

Just to be clear: I am not pushing back here, trying to find better solutions than just a revert.

It is clear to me that we need some solution for search_api module. Only I would rather not have to support the temporary tables functionality in core. I get a very hacky feeling about those "temporary tables".

I do very much like the solution of @mondrake to put the functionality in a contrib service module. I am willing to create and maintain the module.

@mglaman: Is this solution for you acceptable?

Just quickly: Temporary tables are a big boon in case you have some DB result set that you want to run multiple further queries on. Instead of using a (very slow) nested SELECT each time, you save the result into a temporary table once and then get much better performance for your further queries.
As such, just using a nested SELECT might be a valid replacement for a temporary table in general, but for larger sites it could easily mean the difference between usable and much too slow – or even between slow and crashing. So, I’d much rather have temporary table support in place.

Also, since this has been supported in Core for ages, I generally don’t agree with its removal and think it should stay supported in Core, especially if it’s not that hard. That being said, though, if you decide against keeping support for it in Core, I’m not sure whether I’d use a separate contrib module for adding support back and not just add the code for it directly to Search API. It really doesn’t seem complicated enough to warrant its own module, even if we might end up with multiple contrib modules with the same code adding back temporary tables support. (Furthermore, this would only be needed for some functionality, so a dependency would add a completely unused module for some sites.)

(I of course cannot speak for other contrib developers who might want/need this functionality.)

Copying my response from the same question in Slack:

I'd be extremely frustrated because other frameworks seem to adopt helpers like this, but Drupal core seems to push stuff out.

I already proofed out the code for Search API to contain this code. In the end I don't care and just want to unblock Search API for D10.

My reply to #19

It is clear to me that we need some solution for search_api module

Six months ago on the CR someone said they use it for production operations due to large datasets.

Only I would rather not have to support the temporary tables functionality in core. I get a very hacky feeling about those "temporary tables".

Why is it hacky? It's something supported by the systems and not going away. If you search the internet there are various articles on how it's used.

I do very much like the solution of @mondrake to put the functionality in a contrib service module. I am willing to create and maintain the module.

This doesn't feel like an answer. "Hey, we'll make you install yet another contributed module for 15 lines of code!"

Reviewing #17

+++ b/core/lib/Drupal/Core/Database/Connection.php
@@ -86,6 +86,13 @@ abstract class Connection {
+  /**
+   * An index used to generate unique temporary table names.
+   *
+   * @var int
+   */
+  protected $temporaryNameIndex = 0;

@@ -1518,6 +1525,43 @@ protected function doCommit() {
+    return "db_temporary_" . $this->temporaryNameIndex++;

Let's skip the property and just track it statically within the method.

+    static $temporary_name_index = 0;
$tablename = "db_temporary_" . $temporary_name_index++;

Moving the component to the database system not the sqlite driver. As said by others, losing temporary query functionality is a problem for ALL databases. And per comment #20, without this core functionality, DB search in Search API, used by nearly 25% of D8+ users, would be less performant in Drupal 10. Clearly, based off this issue, and unrelated comment in the CR, this has much wider-reaching consequences than the maintainers expected.

Ignorance of how sqlite works with temporary tables was not a good reason to remove unrelated code to the prefix issue. Further review of #3211780: Deprecate Connection::queryTemporary() should have occurred and had this come up during that time, I don't think this would have been removed.

As per the 'not used in core' argument -- there was a discussion some time ago about bringing Search API into core at one point, and the fact that core doesn't use these methods is a technicality, not a reality. With Dries (and other core maintainers) making the point that core will have fewer modules and be composable, it is not a good argument to remove key functionality simply because it is not used by core. Its use by one of the top contrib modules demonstrates a need to have strong core APIs to empower contrib.

1. +++ b/core/lib/Drupal/Core/Database/Connection.php
   @@ -1518,6 +1518,44 @@ protected function doCommit() {
   +    static $temporary_name_index = 0;
   +    return "db_temporary_" . $temporary_name_index++;
   

   Can we use the function uniqid() instead of using a static variable. If we do this than we can also remove this helper method.

2. +++ b/core/lib/Drupal/Core/Database/Connection.php
   @@ -1518,6 +1518,44 @@ protected function doCommit() {
   +  abstract public function queryTemporary($query, array $args = [], array $options = []);
   

   Can we move this method to an interface and have the 3 database drivers implement the interface. The abstract function forces all contrib database driver to implement this method.

Thanks, @daffie! We'll get a new patch rolled on Monday.

If we do this than we can also remove this helper method.

Using uniqid() makes sense, so each driver will have this code and then others can do whatever they want, or copy and paste what is in the core drivers.

Can we move this method to an interface and have the 3 database drivers implement the interface.

Makes sense! So a \Drupal\Core\Database\SupportsTemporaryTablesInterface with queryTemporary

+++ b/core/modules/system/tests/src/Functional/Database/TemporaryQueryTest.php
@@ -0,0 +1,63 @@
+  public function testTemporaryQuery() {

Can we move this test to Drupal\KernelTests\Core\Database and change it into a kernel test. Can we also add:

if (!in_array($coonection->databaseType(), ['mysql', 'pgsql', 'sqlite'], TRUE)) {
  // Run this test only for the by Core supported database drivers.
  $this->markTestSkipped();
}

In #3231950: Split Database tests in 'core' ones and 'driver specific' ones we will change the new test.

The patch looks good!

1. +++ b/core/tests/Drupal/KernelTests/Core/Database/TemporaryQueryTest.php
   @@ -0,0 +1,63 @@
   +    $http_kernel = $this->container->get('http_kernel');
   +    $request = Request::create('database_test/db_query_temporary');
   +    $response = $http_kernel->handle($request);
   +    $data = json_decode($response->getContent());
   +
   +    if ($data) {
   +      $this->assertEquals($this->countTableRows('test'), $data->row_count, 'The temporary table contains the correct amount of rows.');
   +      $this->assertFalse($connection->schema()->tableExists($data->table_name), 'The temporary table is, indeed, temporary.');
   +    }
   +    else {
   +      $this->fail('The creation of the temporary table failed.');
   +    }
   

   This part can be removed. The same for the changes to the database_test module.

2. +++ b/core/tests/Drupal/KernelTests/Core/Database/TemporaryQueryTest.php
   @@ -0,0 +1,63 @@
   +    $table_name_test = $connection->queryTemporary('SELECT [name] FROM {test}', []);
   +    $table_name_task = $connection->queryTemporary('SELECT [pid] FROM {test_task}', []);
   

   Could we add testing that the 2 tables are temporary tables and that they have the same columns as the tables from which they are created.

@daffie,
Addressed point 1 for comment #29.
Needs review.

fixed cs error. please review it

1. +++ b/core/modules/system/tests/modules/database_test/database_test.routing.yml
   @@ -1,3 +1,10 @@
   +database_test.db_query_temporary:
   +  path: '/database_test/db_query_temporary'
   +  defaults:
   +    _controller: '\Drupal\database_test\Controller\DatabaseTestController::dbQueryTemporary'
   +  requirements:
   +    _access: 'TRUE'
   +
   
   +++ b/core/modules/system/tests/modules/database_test/src/Controller/DatabaseTestController.php
   @@ -40,6 +40,23 @@ public static function create(ContainerInterface $container) {
   +  /**
   +   * Creates temporary table and outputs the table name and its number of rows.
   +   *
   +   * We need to test that the table created is temporary, so we run it here, in a
   +   * separate menu callback request; After this request is done, the temporary
   +   * table should automatically dropped.
   +   *
   +   * @return \Symfony\Component\HttpFoundation\JsonResponse
   +   */
   +  public function dbQueryTemporary() {
   +    $table_name = $this->connection->queryTemporary('SELECT [age] FROM {test}', []);
   +    return new JsonResponse([
   +      'table_name' => $table_name,
   +      'row_count' => $this->connection->select($table_name)->countQuery()->execute()->fetchField(),
   +    ]);
   +  }
   +
   

   The changes to these files from the database_tests module can be removed.

2. +++ b/core/tests/Drupal/KernelTests/Core/Database/TemporaryQueryTest.php
   @@ -0,0 +1,49 @@
   +class TemporaryQueryTest extends DatabaseTestBase {
   

   As #3231950: Split Database tests in 'core' ones and 'driver specific' ones has just landed, we need to make some changes. The test should now extend Drupal\KernelTests\Core\Database\DriverSpecificDatabaseTestBase and the class will need to become an abstract class. For each of the 3 database driver modules, a test should be added that extends TemporaryQueryTest.

+++ b/core/modules/mysql/tests/src/Kernel/mysql/TemporaryQueryTest.php
@@ -0,0 +1,37 @@
+    $connection = $this->getConnection();
+
+    // Now try to run two temporary queries in the same request.
+    $table_name_test = $connection->queryTemporary('SELECT [name] FROM {test}', []);
+    $table_name_task = $connection->queryTemporary('SELECT [pid] FROM {test_task}', []);
+
+    $this->assertEquals($this->countTableRows('test'), $this->countTableRows($table_name_test), 'A temporary table was created successfully in this request.');
+    $this->assertEquals($this->countTableRows('test_task'), $this->countTableRows($table_name_task), 'A second temporary table was created successfully in this request.');
+
+    // Check that leading whitespace and comments do not cause problems
+    // in the modified query.
+    $sql = "
+      -- Let's select some rows into a temporary table
+      SELECT [name] FROM {test}
+    ";
+    $table_name_test = $connection->queryTemporary($sql, []);
+    $this->assertEquals($this->countTableRows('test'), $this->countTableRows($table_name_test), 'Leading white space and comments do not interfere with temporary table creation.');

+++ b/core/modules/pgsql/tests/src/Kernel/pgsql/TemporaryQueryTest.php
@@ -0,0 +1,37 @@
+    $connection = $this->getConnection();
+
+    // Now try to run two temporary queries in the same request.
+    $table_name_test = $connection->queryTemporary('SELECT [name] FROM {test}', []);
+    $table_name_task = $connection->queryTemporary('SELECT [pid] FROM {test_task}', []);
+
+    $this->assertEquals($this->countTableRows('test'), $this->countTableRows($table_name_test), 'A temporary table was created successfully in this request.');
+    $this->assertEquals($this->countTableRows('test_task'), $this->countTableRows($table_name_task), 'A second temporary table was created successfully in this request.');
+
+    // Check that leading whitespace and comments do not cause problems
+    // in the modified query.
+    $sql = "
+      -- Let's select some rows into a temporary table
+      SELECT [name] FROM {test}
+    ";
+    $table_name_test = $connection->queryTemporary($sql, []);
+    $this->assertEquals($this->countTableRows('test'), $this->countTableRows($table_name_test), 'Leading white space and comments do not interfere with temporary table creation.');

+++ b/core/modules/sqlite/tests/src/Kernel/sqlite/TemporaryQueryTest.php
@@ -0,0 +1,37 @@
+    $connection = $this->getConnection();
+
+    // Now try to run two temporary queries in the same request.
+    $table_name_test = $connection->queryTemporary('SELECT [name] FROM {test}', []);
+    $table_name_task = $connection->queryTemporary('SELECT [pid] FROM {test_task}', []);
+
+    $this->assertEquals($this->countTableRows('test'), $this->countTableRows($table_name_test), 'A temporary table was created successfully in this request.');
+    $this->assertEquals($this->countTableRows('test_task'), $this->countTableRows($table_name_task), 'A second temporary table was created successfully in this request.');
+
+    // Check that leading whitespace and comments do not cause problems
+    // in the modified query.
+    $sql = "
+      -- Let's select some rows into a temporary table
+      SELECT [name] FROM {test}
+    ";
+    $table_name_test = $connection->queryTemporary($sql, []);
+    $this->assertEquals($this->countTableRows('test'), $this->countTableRows($table_name_test), 'Leading white space and comments do not interfere with temporary table creation.');

All this code can be moved to the class core/tests/Drupal/KernelTests/Core/Database/TemporaryQueryTestBase

The patch looks great, only my point #29.2 still needs to be addressed.

I think we could test it by using \Drupal\Core\Database\Database::closeConnection, correct? Temporary tables are cleaned up after a connection is closed. So a kernel test could close the connection and then re-open it to verify the table has been removed.

1. The test that I am looking for is something like:

   foreach (array of table fields as field_name) {
    for(each row) {
       $this->assertSame(normal_table->field_name, temporary_table->field_name);
     }
   }
   

2. The other thing I would like to assert that our temporary table is in the database a temporary table. Use the information_schema data from the database.

All code changes look good to me.
Tests are added.
I have updated the IS and created a CR.
For me it is RTBC.

The change is for D10.0 as it is contrib module blocker.

Committed/pushed to 10.1.x and cherry-picked to 10.0.x, thanks!

I've updated https://www.drupal.org/node/3211781 to indicate that it's outdated and added a release note here.

I think we need 9.5 and 9.4 patches here to remove the deprecation from queryTemporary().

+++ b/core/modules/system/tests/src/Functional/Database/TemporaryQueryTest.php
@@ -33,9 +33,6 @@ public function countTableRows($table_name) {
-    $this->expectDeprecation('Connection::generateTemporaryTableName() is deprecated in drupal:9.3.0 and is removed from drupal:10.0.0. There is no replacement. See https://www.drupal.org/node/3211781');

The method generateTomporaryTableName() will stay deprecated. Therefor this line needs to stay.

Addressed Comment #50.

Looks good to me.

Committed/pushed to 9.5.x and 9.4.x, thanks!