Improve the Upsert query implementation for PostgreSQL

It would probably be reasonably straightforward to have 2 Postgres Upsert classes, and override the upsert() method on the Connection to load one or the other depending on the version. Then we can leave the current safe version in place, and have a fancy-faster version for 9.4+ users.

@isntall and I are fairly certain that the current Upsert is causing testbot havoc as implemented by #2336627: Deadlock on cache_config (DatabaseBackend::setMultiple()). And @amateescu has been testing revert patches as well)

I'm going to set this to active, and bump it up to Major at least.

What I think is happening on testbot, and it's happening on my local testbot, is that PostgreSQL is running out of shared memory creating the temporary table in the upsert class.

This might be because cache tables are large and the temporary table persists until the end of the transaction. Maybe we could try a patch that commits the transaction after the upsert is done. I don't know if any other queries are run in that connection. I don't think we would want to commit the temporary table before doing any of the other operations, right?

I'm not sure if this is explicitly causing the test to exceed Docker's timeout.

Here is a sample exception from my latest test run which was concurrency 1. Also attached sqlite database with simpletest results. For the curious, sqlite3 2015-08-18-test.sqlite "SELECT * FROM simpletest WHERE status = 'exception'" | less

Drupal\Core\Database\DatabaseExceptionWrapper: SQLSTATE[53200]: Out of memory: 7 ERROR:  out of shared memory
HINT:  You might need to increase max_locks_per_transaction.: CREATE TEMPORARY TABLE {db_temporary_1} AS SELECT * FROM {cache_
config} LIMIT 0; Array
(
)
 in Drupal\Core\Config\CachedStorage->write() (line 133 of /var/www/html/core/lib/Drupal/Core/Config/CachedStorage.php). 
Drupal\Core\Database\Connection->handleQueryException(Object, 'CREATE TEMPORARY TABLE {db_temporary_1} AS SELECT * FROM {cache_config} LIMIT 0', Array, Array)
Drupal\Core\Database\Connection->query('CREATE TEMPORARY TABLE {db_temporary_1} AS SELECT * FROM {cache_config} LIMIT 0', Array, Array)
Drupal\Core\Database\Driver\pgsql\Connection->query('CREATE TEMPORARY TABLE {db_temporary_1} AS SELECT * FROM {cache_config} LIMIT 0', Array, Array)
Drupal\Core\Database\Driver\pgsql\Connection->queryTemporary('SELECT * FROM {cache_config} LIMIT 0', Array, Array)
Drupal\Core\Database\Driver\pgsql\Upsert->execute()
Drupal\Core\Cache\DatabaseBackend->doSetMultiple(Array)
Drupal\Core\Cache\DatabaseBackend->setMultiple(Array)
Drupal\Core\Cache\DatabaseBackend->set('system.performance', Array)
Drupal\Core\Config\CachedStorage->write('system.performance', Array)
Drupal\config\Tests\Storage\CachedStorageTest->setUp()
Drupal\simpletest\TestBase->run(Array)
simpletest_script_run_one_test('221', 'Drupal\config\Tests\Storage\CachedStorageTest')

I'm going to assume that this patch is not a good approach, but I want to send something to the test bot before I go to bed.

new_values is populated from a select? Maybe the data type that is returned from the select is not the same as the base table's type?

The use of CTE is still going to create a temporary table, right?

I was also looking at temporary table creation.

* If going back to the original upsert implementation, queryTemporary() could be changed to inherit constraints probably best implemented using LIKE.
* No indexes are created so with lots of rows, a select on the temporary table may be much slower.
* Should the driver always drop the temporary table on commit? queryTemporary() would to be modified to append ON COMMIT DROP. I think this would also prevent the need to manually vacuum afterward.
* Currently drupalci postgresql container has temp_buffers set to default 8MB. This could be increased for performance for the testbot. Maybe 1GB?

When using Common Table Expressions, the type can be defined for the first row of values, but PostgreSQL will infer the type for the rest of the values.

That's a round trip to table information again. :(

Maybe it can be stored as protected on the Upsert object?

It is unclear to me what the concurrency requirements of our UPSERT should be.

One approach would be to do a INSERT followed by an UPDATE of the rows that couldn't be inserted.

Supposing an UPSERT in table "cache_render", with key "cid":

BEGIN;

// Try to insert the rows, I think this will try to insert all the values, and will not stop at the first duplicate key.
INSERT INTO cache_render (...) VALUES(...) RETURNING cid;

// Try to update the rows that were not returned by the previous query.
// We have to do one query per row, but we can batch that into multi-query statements for performance.
UPDATE cache_render SET .... WHERE cid = ... RETURNING cid; [ x rows not returned ]

// In the face of concurrent deletions, we might have missed some rows in the update, in that case, we could either
// (1) retry the insert here, but that can possibly fail too in the face of concurrent inserts (or upserts)
// or (2) abort the transaction and retry

COMMIT;

@amateescu: I don't think you need a stored function at all here. All this logic can be safely handled in the application, and if we batch the UPDATE patch into multi-statement, the latency is going to be just fine.

Ran ab -n 10000 -c 10 http://drupal8.dev/ and drush cr (repeatedly) on CentOS 7, Apache 2.4.6, PostgreSQL 9.2.13 on a bare Drupal installation.

It's slow, but it didn't run into any Exceptions.

Results:

ab -n 10000 -c 10 http://drupal8.dev/
This is ApacheBench, Version 2.3 <$Revision: 1430300 $>
Copyright 1996 Adam Twiss, Zeus Technology Ltd, http://www.zeustech.net/
Licensed to The Apache Software Foundation, http://www.apache.org/

Benchmarking drupal8.dev (be patient)
Completed 1000 requests
Completed 2000 requests
Completed 3000 requests
Completed 4000 requests
Completed 5000 requests
Completed 6000 requests
Completed 7000 requests
Completed 8000 requests
Completed 9000 requests
Completed 10000 requests
Finished 10000 requests


Server Software:        Apache/2.4.6
Server Hostname:        drupal8.dev
Server Port:            80

Document Path:          /
Document Length:        9548 bytes

Concurrency Level:      10
Time taken for tests:   257.084 seconds
Complete requests:      10000
Failed requests:        4715
   (Connect: 0, Receive: 0, Length: 4715, Exceptions: 0)
Write errors:           0
Non-2xx responses:      40
Total transferred:      107653045 bytes
HTML transferred:       95105365 bytes
Requests per second:    38.90 [#/sec] (mean)
Time per request:       257.084 [ms] (mean)
Time per request:       25.708 [ms] (mean, across all concurrent requests)
Transfer rate:          408.93 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0    0   0.0      0       2
Processing:    23  257 2257.5     65   30050
Waiting:       23  257 2257.5     65   30050
Total:         23  257 2257.5     65   30050

Percentage of the requests served within a certain time (ms)
  50%     65
  66%     74
  75%     81
  80%     85
  90%     97
  95%    109
  98%    124
  99%    146
 100%  30050 (longest request)

1. +++ b/core/lib/Drupal/Core/Database/Driver/pgsql/Upsert.php
   @@ -29,68 +29,38 @@ public function execute() {
   +      $this->connection->query('LOCK TABLE {' . $table . '} IN SHARE ROW EXCLUSIVE MODE', [], $this->queryOptions);
   

   SHARE ROW EXCLUSIVE MODE looks like it would mean concurrency issues, but it does lock the table for exclusive data changes.

2. +++ b/core/lib/Drupal/Core/Database/Driver/pgsql/Upsert.php
   @@ -29,68 +29,38 @@ public function execute() {
   +      foreach (array_chunk($delete_ids, 1000) as $delete_ids_chunk) {
   +        $this->connection->delete($this->table, $this->queryOptions)
   +          ->condition($this->key, $delete_ids_chunk, 'IN')
   +          ->execute();
   

   I have done this in Drupal 7.

   If we think about the tables that currently use upsert, it's cache tables (cache_bootstrap and cache_config). The config cache could get quite large, which probably accounts for the slow cache-rebuild in the benchmark tests.

   However the request isn't timing out so that seems okay.

3. +++ b/core/lib/Drupal/Core/Database/Driver/pgsql/Upsert.php
   @@ -29,68 +29,38 @@ public function execute() {
   +      $insert->execute();
   

   These two large writes will probably incur an AUTO VACUUM within 3 minutes on the database server.

   That would also account for a performance hit, especially on the testbot. I suggested turning off auto vacuum on the testbot, which we need to test, but should be Ok with the amount of memory the test bot has.

   On a standard production server, postgresql should vacuum within 3 minutes of the cache rebuild with this many writes.

I think this is RTBC if the test bot run passes, and someone can confirm that my AB results are respectable for a cache rebuild. 30 second request after a cache clear seems bad, but it is not deadlocking, and after cache rebuilt the requests go back down to being fast with the page cache.

The pgsql-9.1-web-5.5 test bot was running tests in about 28 minutes before.

The job above is running more than twice as long.

Job Comparison:

• #112: https://dispatcher.drupalci.org/job/php5.5_postgres9.1/112/
• #8097: https://dispatcher.drupalci.org/job/default/8097/

I updated the issue summary with details from the comments about the various things @amateescu or I tried.

Specified which patch is the latest and hid #20.

I can't speak to the Postgres code specifically, but the DBTNG architectural bits look good to me. +1 on commit from that angle.

Let's get this in because then at least we have testbot results again. Committed 8dc9763 and pushed to 8.0.x. Thanks!

Fix typo in summary.