Incorrect statement class caused by race condition when closing and opening the connection

I'm wondering if we could make the $connection variable an explicit reference of a private variable that holds the PDO connection object?

Theoretical patch attached. Probably breaks horribly.

Flip back to needs work.

@benjifisher:

Although I can't recall the specific thing I did to notice that, what I had been doing was to add some debug code to check what class/interface the connection class's statement was using. I think I needed to add the debug code because I was running the one test directly on the command line through run-tests script. Perhaps I put that code in Connection::destroy right before that condition, but I'm not sure exactly why I said that about key_value table query. I

I'm actually surprised the patch I posted in #4 was as successful as it was in the Postgresql run.

Let's move my logic into the pgsql driver instead and then check NULL in Database::openConnection for whether a new connection should be opened as well as isset condition.

+++ b/core/lib/Drupal/Core/Database/Driver/pgsql/Connection.php
@@ -157,6 +174,21 @@ public static function open(array &$connection_options = array()) {
+      $this->actualConnection->setAttribute(\PDO::ATTR_STATEMENT_CLASS, array('PDOStatement', array()));

Maybe also setting $this->connection = &$this->actualConnection; would be safer as well even if there are no explicit test failures.

Maybe the problem is not the PostgreSQL database, but the different implementations of the method tableExists. This patch make the implementation of PostgreSQL version the same as that of the MySQL version. Lets see if that makes any difference.

The priority may be higher: https://www.drupal.org/core/issue-priority

Seriously I'm just on a phone and saw "race condition".

Oh, wow, nice find @daffie. I didn't know there was a difference.

Updated issue summary.

+++ b/core/lib/Drupal/Core/Database/Driver/pgsql/Schema.php
@@ -466,9 +466,13 @@ protected function createPrimaryKeySql($fields) {
+      $this->connection->queryRange("SELECT 1 FROM {" . $table . "}", 0, 1);

This needs to be wrapped in a transaction (probably using the connection::addSavepoint() method iirc) so that a failure does not stop any subsequent writes.

I think a somewhat faster query than pg_tables could be to join pg_catalog.pg_namespace and pg_catalog.pg_class. But this may not be as fast as a transaction-wrapped select on the table itself.

SELECT 1 FROM pg_catalog.pg_class c INNER JOIN pg_catalog.pg_namespace n ON (n.oid = c.relnamespace) WHERE n.nspname='public' AND c.relkind = 'r' AND relname = :tablename

There are 4 different versions of the method Schema::tableExists(). One version for all databases and three other that all override the base version. This patch adds the code for each version in a try-catch statement. I have added two patch: one with the fix only and one with the fix and the patch from #2664322-111: key_value table is only used by a core service but it depends on system install. Both patches should pass the testbot.

@mradcliffe: Making the code faster is a great idea, but lets do that in another issue.

I just spent the entire day yesterday digging into the performance issues on postgres on the testbots, because we dont really want postgres to be that much slower in testing, and therefore cost us more resources.

So, what I discovered is that huge majority of the time spend in queries in postgres are to the information_schema tables. In #postgresql yesterday I was made aware that the queries to the information_schema table are *very* slow, :

[02/02/2017 -:- 02:45:23 PM]  <RhodiumToad>	ok. the information_schema views are crippled by the SQL spec and generate _really bad_ query plans
[02/02/2017 -:- 02:45:54 PM]  <RhodiumToad>	so, if you need to test whether tables exist or whatever, and you care about performance, you use the system catalogs instead

Which tells me that we should avoid hitting information_schema at all. pg_class and pg_type are the two system catalogs that end up getting hammered with disk writes.

During a mysql test, there is 7.8 GB of total disk writes, and during a pgsql test, there is about 180 GB of disk writing, and almost all of it is to the two files that comrprise pg_class and pg_type.

Changed the PostgreSQL query for Schema::tableExists() so that it will be faster. Using the query from @mradcliffe in comment #17.

Removing the "Needs tests" tag, because the test ConnectionFailureTest does the testing we need for this problem.

Forgot to add the transaction savepoints.

Only adding transaction savepoints to the PostgreSQL method Schema::tableExists().

I updated the issue summary based on comments and added the tests for a spin on 8.7.x. I'm sorry for taking so long.

I looked at the latest patch in #24 and that makes sense. I think we're covering all the bases here by wrapping in our savepoint transaction.

Needs a re-roll.

I'm intending to pick this up tomorrow as it and https://www.drupal.org/project/drupal/issues/2664322 are causing me issues with a custom profile install. Assigning to self for that reason.

Updating to 8.7-dev at the time of writing (88ad0137b5d0cd9267160e29).

Rerolled the patch and added testing.

Just quickly skimmed the patch and it looks sqlite could be affected by related

+++ b/core/tests/Drupal/Tests/Core/Database/SchemaTest.php
@@ -0,0 +1,120 @@
+    $connection = $this->getMockBuilder('Drupal\Core\Database\Connection')
...
+    $condition = $this->getMockBuilder('Drupal\Core\Database\Query\Condition')
...
+    $schema = $this->getMockBuilder('Drupal\Core\Database\Schema')
...
+    $connection = $this->getMockBuilder('Drupal\Core\Database\Connection')
...
+    $schema = $this->getMockBuilder('Drupal\Core\Database\Driver\pgsql\Schema')
...
+    $connection = $this->getMockBuilder('Drupal\Core\Database\Connection')
...
+    $schema = $this->getMockBuilder('Drupal\Core\Database\Driver\sqlite\Schema')

Better to use ::class notation, it is shorter and better navigable in IDEs

Order matters. When uploading multiple patches, one of which is expected to fail, add a patch that is expected to pass last.

Did not know that, thought the ordering was random. @benjifisher: Thanks, I learned something new.

Addressed the remark made by @andypost in comment #39.

1. +++ b/core/tests/Drupal/Tests/Core/Database/SchemaTest.php
   @@ -0,0 +1,125 @@
   +/**
   + * @group Database
   + */
   

   Shouldn't class doc block have a full sentence before @group?

2. +++ b/core/tests/Drupal/Tests/Core/Database/SchemaTest.php
   @@ -0,0 +1,125 @@
   +  /**
   +   * @covers \Drupal\Core\Database\Schema::tableExists
   ...
   +  /**
   +   * @covers \Drupal\Core\Database\Driver\pgsql\Schema::tableExists
   ...
   +  /**
   +   * @covers \Drupal\Core\Database\Driver\sqlite\Schema::tableExists
   

   Shouldn't there be a full sentence before @covers or any other annotation?

3. +++ b/core/tests/Drupal/Tests/Core/Database/SchemaTest.php
   @@ -0,0 +1,125 @@
   +    $this->assertFalse($schema->tableExists('table_does_not_extst'));
   ...
   +      ->will($this->returnValue(['schema' => 'main', 'table' => 'table_does_not_extst']));
   ...
   +    $this->assertFalse($schema->tableExists('table_does_not_extst'));
   ...
   +      ->will($this->returnValue(['schema' => 'main', 'table' => 'table_does_not_extst']));
   ...
   +    $this->assertFalse($schema->tableExists('table_does_not_extst'));
   

   I was a little confused by the "table_does_not_extst" as my mind wants to look at it as a typo "table_does_not_exist". It doesn't really matter, but it might be confusing to someone else.

Thank you for your review @mradcliffe.

1. Fixed.
2. I am not sure if there needs to be a full sentence before @covers. I followed the example in https://www.drupal.org/node/2116043#s-examples--3. The example is about @coversDefaultClass and applied the same idea to @covers. If you think that adding a sentence is an improvement then I will add one.
3. Fixed. Had to read your comment twice, before I saw my own typo.

Works for me.

+++ b/core/lib/Drupal/Core/Database/Driver/pgsql/Schema.php
@@ -489,7 +489,19 @@ protected function createPrimaryKeySql($fields) {
+    catch (\Exception $e) {

+++ b/core/lib/Drupal/Core/Database/Driver/sqlite/Schema.php
@@ -27,10 +27,15 @@ class Schema extends DatabaseSchema {
+    catch (\Exception $e) {

+++ b/core/lib/Drupal/Core/Database/Schema.php
@@ -167,14 +167,19 @@ protected function buildTableNameCondition($table_name, $operator = '=', $add_pr
+    catch (\Exception $e) {

Is there a more specific exception to catch here - ie should we only catching \PDOException exceptions? Catching \Exception is very generic.

I see no problem in changing the use of \Exception to \PDOException.

Since we should fix this bug in D8 too, I'm filing it against 8.8.x (which is the current bugfix support branch). Patches can be tested against other branches as needed when they're uploaded.

The issue will be automatically updated to 8.9.x after the last 8.8.x bugfix release. Thanks!

The Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.