Postgres insert queries that fail in a transaction break the entire transaction

The existing code looks the same in Drupal 8, so I think this needs to be fixed there too (then backported to Drupal 7)?

I've patched this for Drupal 8.

This approach makes sense, but I'm not sure how to manually reproduce the issue in order to test. I'm guessing automated tests won't be able to reproduce this issue either?

+++ b/core/lib/Drupal/Core/Database/Driver/pgsql/Insert.php
@@ -101,12 +101,23 @@ public function execute() {
+    // Transaction commits here where $transaction looses scope.

Spelling and maybe a re-word.

"The transaction will commit after $transaction loses scope here."

I'm okay with explanatory comments. :-)

+++ b/core/lib/Drupal/Core/Database/Driver/pgsql/Insert.php
@@ -101,12 +101,23 @@ public function execute() {
+      catch (Exception $e) {
...
+        // Rethrow the exception for the calling code.
+        throw $e;

Should this catch \PDOException, and then rethrow DatabaseExceptionWrapper in Drupal 8?

I just got those kind of errors in a Postgres with a master/slave architecture, when loging/deloging a user (through a CAS Server).
We are not using PbBounce.

The patch above solves the issue.

I'm wondering if we are solving an issue with this patch, or if we are implementing a workaround for a bug in PG ?

This is also causing problems for auto creation of tables like cache. See #2664466: url_alias table is only used by a core service but it depends on system install. Basically postgres behaves differently from both sqlite and mysql in the once a transaction has an error it can not continue using that transaction. All of the database actions create a savepoint to protect us against this but insert does not.

I've added a test and ensure that this works and now the patch works the same way as all of the other postgres query types with the exception of the NativeUpsert which needs fixing too. No interdiff to #2 because the patch was developed without knowledge of this issue.

re-titling to explain the actual bug and adding info to the summary.

As well as NativeUpsert queries it looks like Merge queries might have the issue too.

So postgres merge queries don't have the issue because they are just combinations of select, insert and update queries. So the only one that will fail is the NativeUpsert - but I can't test that because I don't have postgres 9.5 installed... However I've added tests for all the query types so at least we have the test in place.

Here's a patch for the NativeUpsert - it'd be great if someone can run it against a postgres db on 9.5 or above... the test to run is Drupal\system\Tests\Database\TransactionTest::testQueryFailureInTransaction

Oops forgot to release the savepoint in the NativeUpsert - I've asked @jsmith to test this.

Now for the even more complex situation with the query method...

I don't have a vm running 9.5 handy at the moment, but probably can get that up and running to test some time this week.

I have tested this on a Fedora 24 (rawhide) installation with PostgreSQL 9.5.0 and PHP 5.6.18.

With the test-only patch from comment 11, the test on Drupal\system\Tests\Database\TransactionTest fails with one exception:

Drupal\Core\Database\DatabaseExceptionWrapper: SQLSTATE[25P02]: In failed sql transaction: 7 ERROR: current transaction is aborted, commands ignored until end of transaction block: SELECT 1 FROM pg_tables WHERE schemaname = :schema AND tablename = :table; Array ( [:schema] => public [:table] => simpletest566550test ) in Drupal\simpletest\KernelTestBase->installSchema() (line 438 of /var/www/html/drupal8/core/modules/simpletest/src/KernelTestBase.php).

Drupal\Core\Database\Connection->handleQueryException(Object, 'SELECT 1 FROM pg_tables WHERE schemaname = :schema AND tablename = :table', Array, Array)
Drupal\Core\Database\Connection->query('SELECT 1 FROM pg_tables WHERE schemaname = :schema AND tablename = :table', Array, Array)
Drupal\Core\Database\Driver\pgsql\Connection->query('SELECT 1 FROM pg_tables WHERE schemaname = :schema AND tablename = :table', Array)
Drupal\Core\Database\Driver\pgsql\Schema->tableExists('test')
Drupal\Core\Database\Schema->createTable('test', Array)
Drupal\simpletest\KernelTestBase->installSchema('database_test', Array)
Drupal\system\Tests\Database\TransactionTest->testQueryFailureInTransaction()
Drupal\simpletest\TestBase->run()
_simpletest_batch_operation(Array, '8', Array)
call_user_func_array('_simpletest_batch_operation', Array)
_batch_process()
_batch_do()
_batch_page(Object)
Drupal\system\Controller\BatchController->batchPage(Object)
call_user_func_array(Array, Array)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}()
Drupal\Core\Render\Renderer->executeInRenderContext(Object, Object)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->wrapControllerExecutionInRenderContext(Array, Array)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}()
call_user_func_array(Object, Array)
Symfony\Component\HttpKernel\HttpKernel->handleRaw(Object, 1)
Symfony\Component\HttpKernel\HttpKernel->handle(Object, 1, 1)
Drupal\Core\StackMiddleware\Session->handle(Object, 1, 1)
Drupal\Core\StackMiddleware\KernelPreHandle->handle(Object, 1, 1)
Drupal\page_cache\StackMiddleware\PageCache->pass(Object, 1, 1)
Drupal\page_cache\StackMiddleware\PageCache->handle(Object, 1, 1)
Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle(Object, 1, 1)
Drupal\Core\StackMiddleware\NegotiationMiddleware->handle(Object, 1, 1)
Stack\StackedHttpKernel->handle(Object, 1, 1)
Drupal\Core\DrupalKernel->handle(Object)

With the patch from comment 12, the tests pass with no exceptions.

I have also tested the patch in comment 14, and it passes with no errors or exceptions.

I'm not entirely sure about the changes in #14. We need to identify savepoint related queries in order to make postrgres work the same way as the other databases. If we don't identify the savepoint queries we end up in a infinite loop :( But adding the option feels weird and unreliable. For example Mysql connection contains:

        try {
          $this->query('RELEASE SAVEPOINT ' . $name);
        }

So in theory we'd have to add the option to this as well to be consistent....

Setting back to "needs review" to do some more thinking about this.

I think this is a cleaner solution - there is no point wrapping savepoint queries in savepoints :) but also postgres implementation details should not bleed into the main database code.

Actually we can improve this since we only need to worry about this if we're in a transaction.

Also there is #2572283: Neither REPEATABLE READ nor READ COMMITTED transaction isolation levels are always appropriate, in which the latest patch makes changes to the implicit commit handling and makes it more flexible for all database drivers. Unfortunately nobody has reviewed @david_garcia's work yet for all database drivers.

@mradcliffe that issue is addressed something different afaics. This issue is addressing a difference between postgres and the other database drivers.

One thing I don't understand about this patch. For insert/merge etc. queries we now create two savepoints, all the time. Is this actually needed or would it be enough to create just one
around the entire insert (for example) and skip the query ones?

On top of that I'm also wondering how costly it is to set savepoints for potentially many queries, when you are in a transaction? I guess this would be basically for free?

+++ b/core/lib/Drupal/Core/Database/Driver/pgsql/Connection.php
@@ -138,7 +138,35 @@ public function query($query, array $args = array(), $options = array()) {
+        stripos($query, 'ROLLBACK TO SAVEPOINT ') === 0 ||
+        stripos($query, 'RELEASE SAVEPOINT ') === 0 ||
+        stripos($query, 'SAVEPOINT ') === 0

is there a reason we use case insensitive comparison here?

We only create savepoints if we're in a transaction - ie. where it is necessary.

  public function addSavepoint($savepoint_name = 'mimic_implicit_commit') {
    if ($this->inTransaction()) {
      $this->pushTransaction($savepoint_name);
    }
  }

And release and rollback only work if the savepoint exists

But @dawehner you are on to something - we shouldn't be creating a 'query_mimic_implicit_commit' savepoint if a 'mimic_implicit_commit' already exists.

+++ b/core/lib/Drupal/Core/Database/Driver/pgsql/Connection.php
@@ -138,9 +138,12 @@ public function query($query, array $args = array(), $options = array()) {
+    // We need to wrap queries with a savepoint if :
+    // - Currently in a transaction.
+    // - A 'mimic_implicit_commit' does not exist already.
+    // - The query is not a savepoint query.
     $wrap_with_savepoint = $this->inTransaction() &&
+      !isset($this->transactionLayers['mimic_implicit_commit']) &&

I'm not sure, I would have passed along an additional option to stop adding the savepoint instead.

@dawehner see #14 and the comments up to the new patch. I didn't like the way that that was going.

Basically that would mean if you add your own savepoint you'd need to indicate that it is a savepoint query.

Basically that would mean if you add your own savepoint you'd need to indicate that it is a savepoint query.

I'm not sure that's such a bad requirement?

I'm not sure that's such a bad requirement?

Well, the question is whether this would be so easy to do. This basically makes it impossible to call out to other code, as that code cannot know whether you have added your own savepoint or not.

To me adding an option to say this is a save point query is breaking the idea of database encapsulation - people writing cross database code have to know stuff to make Postgres work - with the current patch they don't.

Alex made some convincing arguments.

Yes that's fair enough. Committed/pushed to 8.1.x and cherry-picked to 8.0.x. Thanks!

Moving to 7.x for backport.

I have created a D7 issue for backporting this: #3384672: [D7] PostgreSQL queries that fail in a transaction break the entire transaction

Closing as Fixed for D8, so that credits get assigned.