Prevent a query from aborting the entire transaction in pgsql

Seems this effects currently broken HEAD, so adding as related:
#2254903: PostgreSQL install broken again in HEAD by toolbar module

Testing this patch locally now...

Preparing a local environment:

~/# git clone --branch master http://git.drupal.org/project/drupalci_testbot.git
~/# cd drupalci_testbot

~/drupalci_testbot# ./scripts/build_all.sh update pgsql-9.1

Running tests with patch:

~/drupalci_testbot# sudo DCI_PATCH="https://www.drupal.org/files/issues/drupal-2181291-pgsql-aborted-transaction-3.patch,." DCI_DBTYPE='pgsql' DCI_DBVER='9.1' DCI_TESTGROUPS='--all' DCI_DRUPALBRANCH='8.0.x' DCI_VERBOSE="true" DCI_CONCURRENCY="8" ./containers/web/run.sh

Will paste the results in the end

Attaching results

They seem to fail on:

Test run duration: 14 min 26 sec

Detailed test results
---------------------

Fatal error: Uncaught exception 'PDOException' with message 'SQLSTATE[HY000]: General error: 1 too many SQL variables' in /var/www/core/lib/Drupal/Core/Database/Connection.php:336
Stack trace:
#0 /var/www/core/lib/Drupal/Core/Database/Connection.php(336): PDO->prepare('SELECT * FROM s...')
#1 /var/www/core/lib/Drupal/Core/Database/Connection.php(536): Drupal\Core\Database\Connection->prepareQuery('SELECT * FROM {...')
#2 /var/www/core/scripts/run-tests.sh(978): Drupal\Core\Database\Connection->query('SELECT * FROM {...', Array)
#3 /var/www/core/scripts/run-tests.sh(89): simpletest_script_reporter_display_results()
#4 {main}

Next exception 'Drupal\Core\Database\DatabaseExceptionWrapper' with message 'SQLSTATE[HY000]: General error: 1 too many SQL variables: SELECT * FROM {simpletest} WHERE test_id IN (:test_ids_0, :test_ids_1, :test_ids_2, :test_ids_3, :test_ids_4, :test_ids_5, :test_ids_6, :test_ids_7, :test_ids_8, :test_ids_9, :test_ids_10, :test_ids_11, :test_ids_12, :test_ids_13, :test_ids_14, :test_ids_15, :test_ids_ in /var/www/core/lib/Drupal/Core/Database/Connection.php on line 569

For me it installs with the last patch applied (on PostgreSQL 9.3.5 Ubuntu 14.04.1 LTS 64 bit).

Just startet run on PG testbot with latest patch applied (like in #12). Test is still running, but it seems we are back on track as most/all errors are gone and just leaving the ones we already know about. Big thanks @mradcliffe! Let's get this in after line by line review...

@ricardoamaro can you please crosscheck with my results? Thanks :-)

PostgreSQL 9.3.4
Patch #15

Tested #15 and it came back green and it fixes the broken installer. Some warnings though, not sure if they are related to this or some of the other issues related to PostgreSQL problem.

ERROR:  relation "cache_config" does not exist at character 102
STATEMENT:  SELECT cid, data, created, expire, serialized, tags, checksum_invalidations, checksum_deletions FROM cache_config WHERE cid IN ('system.filter')
ERROR:  relation "config" does not exist at character 24
STATEMENT:  SELECT name, data FROM config WHERE collection = '' AND name IN ('system.filter')
ERROR:  relation "cache_config" does not exist at character 13
STATEMENT:  DELETE FROM cache_config
  WHERE  (cid IN  ('system.filter'))
ERROR:  relation "cache_discovery" does not exist at character 13
STATEMENT:  DELETE FROM cache_discovery
  WHERE  (cid IN  ('typed_config_definitions'))
ERROR:  relation "config" does not exist at character 24
STATEMENT:  SELECT name, data FROM config WHERE collection = '' AND name IN ('core.extension')
ERROR:  relation "config" does not exist at character 18
STATEMENT:  SELECT name FROM config WHERE collection = '' AND name::text LIKE '%'
ERROR:  relation "cache_bootstrap" does not exist at character 102
STATEMENT:  SELECT cid, data, created, expire, serialized, tags, checksum_invalidations, checksum_deletions FROM cache_bootstrap WHERE cid IN ('module_implements')
ERROR:  relation "cache_bootstrap" does not exist at character 102
STATEMENT:  SELECT cid, data, created, expire, serialized, tags, checksum_invalidations, checksum_deletions FROM cache_bootstrap WHERE cid IN ('hook_info')
ERROR:  relation "cache_bootstrap" does not exist at character 30
STATEMENT:  SELECT 1 AS expression
  FROM
  cache_bootstrap cache_bootstrap
  WHERE ( (cid = 'hook_info') )
ERROR:  relation "config" does not exist at character 30
STATEMENT:  SELECT 1 AS expression
  FROM
  config config
  WHERE ( (collection = '') AND (name = 'core.extension') )
ERROR:  relation "cache_default" does not exist at character 30
STATEMENT:  SELECT 1 AS expression
  FROM
  cache_default cache_default
  WHERE ( (cid = 'schema') )
ERROR:  relation "cache_entity" does not exist at character 13
STATEMENT:  DELETE FROM cache_entity
  WHERE  (cid IN  ('values:user:0'))
ERROR:  relation "menu_tree" does not exist at character 56
STATEMENT:  SELECT DISTINCT menu_tree.menu_name AS menu_name
  FROM
  menu_tree menu_tree
ERROR:  relation "cache_menu" does not exist at character 8
STATEMENT:  UPDATE cache_menu SET expire='1409820431'
ERROR:  relation "cache_menu" does not exist at character 8
STATEMENT:  UPDATE cache_menu SET expire='1409820608'
ERROR:  relation "cache_menu" does not exist at character 13
STATEMENT:  DELETE FROM cache_menu
  WHERE  (expire <> '-1') AND (expire < '1409820609')
ERROR:  relation "cache_render" does not exist at character 13
STATEMENT:  DELETE FROM cache_render
  WHERE  (expire <> '-1') AND (expire < '1409820609')
ERROR:  relation "cache_data" does not exist at character 13
STATEMENT:  DELETE FROM cache_data
  WHERE  (expire <> '-1') AND (expire < '1409820609')
ERROR:  relation "config_snapshot" does not exist at character 13
STATEMENT:  DELETE FROM config_snapshot
  WHERE  (name::text ILIKE '%') AND (collection = '')
ERROR:  relation "config_snapshot" does not exist at character 33
STATEMENT:  SELECT DISTINCT collection FROM config_snapshot WHERE collection <> '' ORDER by collection
ERROR:  relation "config_snapshot" does not exist at character 30
STATEMENT:  SELECT 1 AS expression
  FROM
  config_snapshot config_snapshot
  WHERE ( (collection = '') AND (name = 'core.date_format.fallback') )
ERROR:  relation "cache_render" does not exist at character 102
STATEMENT:  SELECT cid, data, created, expire, serialized, tags, checksum_invalidations, checksum_deletions FROM cache_render WHERE cid IN ('entity_view:user:1:full:stark:r.authenticated:Europe/Berlin')
ERROR:  relation "cache_render" does not exist at character 30
STATEMENT:  SELECT 1 AS expression
  FROM
  cache_render cache_render
  WHERE ( (cid = 'entity_view:user:1:full:stark:r.authenticated:Europe/Berlin') )
ERROR:  relation "cache_menu" does not exist at character 102
STATEMENT:  SELECT cid, data, created, expire, serialized, tags, checksum_invalidations, checksum_deletions FROM cache_menu WHERE cid IN ('current-route-parameters:main:route:entity.user.canonical:route_parameters:a:1:{s:4:"user";s:1:"1";}')
ERROR:  relation "cache_menu" does not exist at character 30
STATEMENT:  SELECT 1 AS expression
  FROM
  cache_menu cache_menu
  WHERE ( (cid = 'current-route-parameters:main:route:entity.user.canonical:route_parameters:a:1:{s:4:"user";s:1:"1";}') )

Then I'm RTBC

Per request i'm running:

~/drupalci_testbot# ./scripts/build_all.sh refresh pgsql-9.1

~/drupalci_testbot# sudo \
DCI_PATCH="https://www.drupal.org/files/issues/drupal-2181291-pgsql-aborted-transaction-15.patch,." \
DCI_DBTYPE='pgsql' DCI_DBVER='9.1'  \
DCI_TESTGROUPS='--all' \
DCI_DRUPALBRANCH='8.0.x' \
DCI_VERBOSE="true" \
DCI_CONCURRENCY="8" 
./containers/web/run.sh

Here is the test stdout:

https://www.drupal.org/files/issues/results_drupal-2181291-pgsql-aborted...

ok, let me try and run this again.

My bad...
I did the tests again against patch 15 and here are the results:

https://www.drupal.org/files/issues/drupal-2181291-pgsql-aborted-transac...

Tell me if you need anything further ;)

@mradcliffe would it help if i would tell the testbot to use drush installer?

Like this:

~/drupalci_testbot# ./scripts/build_all.sh refresh pgsql-9.1
~/drupalci_testbot# sudo \
DCI_PATCH="https://www.drupal.org/files/issues/drupal-2181291-pgsql-aborted-transaction-15.patch,." \
DCI_DBTYPE='pgsql' \
DCI_DBVER='9.1'  \
DCI_TESTGROUPS='--all' \
DCI_DRUPALBRANCH='8.0.x' \
DCI_VERBOSE="true" \
DCI_CONCURRENCY="8" \
DCI_INSTALLER="drush" \
./containers/web/run.sh

Let me start a test with these and post the results

@mradcliffe here are the results for a run using drush install:

https://www.drupal.org/files/issues/drupal-2181291-pgsql-aborted-transac...

Rerolled. I'm able to run standard install with this patch.:
Mac OS X 10.9.4
PHP 5.5.17
Postgres 9.35

Oops. patch is now attached.

I've tested the patch and it works as advertised. I'm now able to go a clean installation on PostgreSQL from the 8.0.x branch. I've also read through the code of the patch, and agree that the code looks great.

My only concern, however, is that that we seem to be papering over a more fundamental problem in the installer code -- namely, that the installer tries to SELECT (and even worse, DELETE) from tables before they've been created. I don't know if this is really an expected thing (in which case, let's get this patch committed quickly), or unexpected (in which case, we should really fix the installation code).

Either way, I'd like to get this fixed sooner rather than later, and I'm willing to put some time into it here at DrupalCon Amsterdam.

Alternative to doing savepoints in code is by using ON_ERROR_ROLLBACK.

I also have to agree with #34, the installer shouldn't do SELECT or DELETE on non-existent tables.

Performance wise, I found this old post saying average slowdown of 12%. I wasn't able to find benchmarks for version 9 of postgres.

Just did a quick grep and it doesn't seem that Drupal makes heavily use of transactions. There are some transactions in the Entity System and mostly in storage context. So I guess we are save here in regards of performance...

+++ b/core/lib/Drupal/Core/Database/Driver/pgsql/Delete.php
@@ -9,4 +9,23 @@
-class Delete extends QueryDelete { }
+class Delete extends QueryDelete {

Um. That's crazy that this wasn't implemented before?!

Anyway, this only touches the pgsql lib so looks safe to commit. GREAT work on this effort, folks! Onward to 100% passing tests!

Committed and pushed to 8.x. Thanks! I'm pretty sure I got everyone in the commit message. :)

Just did a bit of reading to confirm that the uniqueness of the save point name. "If multiple savepoints have the same name, only the one that was most recently defined is released."

CommentTestBook test fails because of:
Drupal\Core\Entity\EntityStorageException: mimic_implicit_commit is already in use.

Please see #2356967: PostgreSQL: Fix tests in comment test group.
Follow-up?

Still think it simpler to use ON_ERROR_ROLLBACK. Have that option turned on when establishing the connection. Bonus of being a lot less code, be one extra line of code when connection is established instead of all this code to wrap queries with creating explicit savepoints.

What pros of doing it with code? what are the cons with using ON_ERROR_ROLLBACK option?

@grom358 the ON_ERROR_ROLLBACK option is not a feature of the PostgreSQL server itself. It's a feature of the PostgreSQL client(s) and as far as I can see not supported by PDO. Anyway, it does exactly the same as this patch does (adding SAVEPOINTS), so there will be no profit in terms of performance ;-)

@bzrudi71 correct you are. I just assumed the driver would support it. I was not suggesting it for performance reasons but in reducing the code needed to be maintained.

Besides the installer (which I notice has been marked as fixed) where else in Drupal does it make use of implict savepoints?

This patch means have the same behaviour as MySQL. But wether that is a good thing here is the question. Personally I think can be problematic if we committing a transaction that happen to contain SQL statements that errored.