Random failure in SchemaTest::testSchemaChangePrimaryKey with order of composite primary key

Cannot reproduce this on Postgres 10.4, which I have locally.

I ran the test 100 hundred times and verified that it passed each time. For anyone following along at home, this is what I did:

for i in `seq 1 100`; do ../vendor/bin/phpunit --testdox-text output-$i.txt tests/Drupal/KernelTests/Core/Database/SchemaTest.php --filter testSchemaChangePrimaryKey &> /dev/null; done
cat output-*.txt | grep '\[x\]' | wc -l # This is the number of passes (300)
cat output-*.txt | grep '\[ \]' | wc -l # This is the number of fails (0)

Will now try to run it with 9.2 as that's the oldest version that's on Docker Hub.

Hmm... same result with 9.2.23 :-/

Hehe, searching for "Postgres 9.1" on Docker Hub, I found this little fellow: drupalci/pgsql-9.1. So I ran the above again in the same environment that Drupal.org runs its tests and I again got the same results. So not really sure what to do here.

One thing I thought that may be happening is that due to the concurrency of the tests that there is a race between the different data sets composite_primary_key and composite_primary_key_different_order in ::testSchemaChangePrimaryKey()? I.e. while the first data set is being executed the second is being run as well and then the assertion on the first one fails because they are operating on the same table? That would explain the different order between expected and actual results.

When running the tests locally, a different database prefix is used for each data set, so I can't reproduce this locally, but I don't really know much about how the tests are run on Drupal.org, so...

postponed is not the correct status here. We have a confirmed random fail here.

Trying to have a patch that always exposes the problem.

So we can reproduce it seemingly. I have a hunch that this is to do with the fact this query $result = $this->connection->query("SELECT a.attname, i.indkey FROM pg_index i JOIN pg_attribute a ON a.attrelid = i.indrelid AND a.attnum = ANY(i.indkey) WHERE i.indrelid = '{" . $table . "}'::regclass AND i.indisprimary")->fetchAllKeyed(); is not quite working the way we expect it too. Seeing if the table name has anything to do with it.

So the fix looks effective. Not sure how to explain it though. Also not entirely sure that that matters.

#9 included an unrelated fix for cache clearing on menu save and that code in #9 can be ignored.

oh that's good #9 still failed eventually for the same reason so #11 doesn't work. Hmmm.

What also could be true is that the PostgreSQL table "pg_index" is not updated as fast as we are changing the table structure and testing the changes. It would explain why the patch from comment #10 would do the trick.

@daffie we drop all the tables in the tests ::tearDown() method. One thought is that since I've not seen this locally and I've run this test on Postgres test a lot recently is that one difference between my local and Drupal is concurrency. Maybe that is having an impact.

Also the fix proposed in #11 turned out not to work.

Is it maybe an idea to add some sleep methods, so that the PostgreSQL database has some time to update the pg_index table.

@daffie Postgres is a transactional database I'm pretty sure pg_index is consistent. If not this would bbe a major bug in postgres's ACID compliance.

@alexpott: You are sure that every Schema class call is its own transaction?

@daffie it feels super weird if a schema change is not transactional. That said I'm not sure about how Postgres handles indexes that change as a result of schema changes. So maybe a sleep is worth trying.

@alexpott: We can also split up the test in smaller pieces, so that every test only does one index change. Every test should then be its own transaction.

@daffie that doesn't help us get to the root cause - it'd be great to understand why this is happening. ATM it doesn't make much sense.

The problem still exists.

https://www.drupal.org/pift-ci-job/1411152
https://www.drupal.org/pift-ci-job/1365057
https://www.drupal.org/pift-ci-job/1362262
https://www.drupal.org/pift-ci-job/1359942

After renaming one of the columns, the \Drupal\Core\Database\Driver\pgsql\DatabaseSchema::findPrimaryKeyColumns() method returns the correct primary keys but in the wrong order.

alexpott confirmed in #9 that using different table names does not solve this problem.

I'm testing the impact of the PostgreSQL version and the concurrency parameter. I limit other factors. I'm only testing the field name change.

public function testSchemaChangePrimaryKey(array $initial_primary_key, array $renamed_primary_key) {
  $find_primary_key_columns = new \ReflectionMethod(get_class($this->schema), 'findPrimaryKeyColumns');
  $find_primary_key_columns->setAccessible(TRUE);

  // Test making the field the primary key of the table upon creation.
  $table_name = 'test_table';
  $table_spec = [
    'fields' => [
      'test_field' => ['type' => 'int', 'not null' => TRUE],
      'other_test_field' => ['type' => 'int', 'not null' => TRUE],
    ],
    'primary key' => $initial_primary_key,
  ];
  $this->assertEquals($renamed_primary_key, $find_primary_key_columns->invoke($this->schema, $table_name));
  $this->schema->createTable($table_name, $table_spec);
  $this->assertTrue($this->schema->fieldExists($table_name, 'test_field'));
  $this->assertEquals($initial_primary_key, $find_primary_key_columns->invoke($this->schema, $table_name));

  // Rename the field and make sure the primary key was updated.
  $this->schema->changeField($table_name, 'test_field', 'test_field_renamed', ['type' => 'int', 'not null' => TRUE]);
  $this->assertTrue($this->schema->fieldExists($table_name, 'test_field_renamed'));
  $this->assertEquals($renamed_primary_key, $find_primary_key_columns->invoke($this->schema, $table_name));
}

See #26. The problem does not apply to the new version of PostgreSQL. Tests were successful in version 9.5. The number of tests run simultaneously matters (concurrency: 1).

Everything confirms alexpott's suggestion that the reason is in the SELECT query.

protected function findPrimaryKeyColumns($table) {
  if (!$this->tableExists($table)) {
    return FALSE;
  }

  // Fetch the 'indkey' column from 'pg_index' to figure out the order of the
  // primary key.
  // @todo Use 'array_position()' to be able to perform the ordering in SQL
  //   directly when 9.5 is the minimum  PostgreSQL version.
  $result = $this->connection->query("SELECT a.attname, i.indkey FROM pg_index i JOIN pg_attribute a ON a.attrelid = i.indrelid AND a.attnum = ANY(i.indkey) WHERE i.indrelid = '{" . $table . "}'::regclass AND i.indisprimary")->fetchAllKeyed();
  if (!$result) {
    return [];
  }

  $order = explode(' ', reset($result));
  $columns = array_combine($order, array_keys($result));
  ksort($columns);
  return array_values($columns);
}

Maybe the solution is to add 'ORDER BY' to the SELECT query? Otherwise, the solution is unlikely to be trivial. I suggest that you first get rid of the troublesome random test failure. This is critical and applies only to tests run simultaneously when concurrency > 1.

I add a follow-up to solve this problem in another task. #3096608: On PostgreSQL 9.1 after renaming one of the fields, the DatabaseSchema::findPrimaryKeyColumns method returns the correct keys but in the wrong order.

This might be related to orphaned sequences because of #3028706: New serial columns do not own sequences. Not specifically the addPrimaryKey assertion, but it being in the same test case as SchemaTest and due to concurrency, length of the key, and databases (re)used in test bots?

@Krzysztof Domański why do we need another task #3096608: On PostgreSQL 9.1 after renaming one of the fields, the DatabaseSchema::findPrimaryKeyColumns method returns the correct keys but in the wrong order - isn't that what this issue is going to solve?

I close #3096608 then.

Ignore

It also fails elsewhere.

Ignore

Stupid mistake. Ignore.

Looks like technical debt, but it works. One frequent random test failure less.

See #26 and #27. Primary key sorting works on PostgreSQL 9.1 in the real world. The problem occurs only in tests, because we use many machines, concurrency > 1.

Proposed solution:

// If PostgreSQL less than 9.5, then arrays of primary key are sorted before comparison.
$canonicalize = FALSE;
if ($this->connection->driver() == 'pgsql' && version_compare($this->connection->clientVersion(), '9.5', '<')) {
  $canonicalize = TRUE;
}
$this->assertEquals($renamed_primary_key, $find_primary_key_columns->invoke($this->schema, $table_name), '', 0, 10, $canonicalize);

@Krzysztof Domański: Great work, thanks.

The problem with the solution from comment #36 is that if you run the test with a concurrency is 1 and with PostgreSQL < 9.5, the test will fail and it should pass. My suggestion would be to skip the tests for PostgreSQL versions < 9.5.

Let's see what happens. Each test repeated 100 times.

Correction of a damaged previous patch.

The problem with the solution from comment #36 is that if you run the test with a concurrency is 1 and with PostgreSQL < 9.5, the test will fail and it should pass. My suggestion would be to skip the tests for PostgreSQL versions < 9.5.

All pass in PostgreSQL 9.5. Also pass when you run the test with a concurrency is 1 and with PostgreSQL 9.1.

We do not need to exclude tests for older versions. Sorting arrays before comparison will solve the problem.

+++ b/core/tests/Drupal/KernelTests/Core/Database/SchemaTest.php
@@ -816,6 +816,14 @@ public function testSchemaChangePrimaryKey(array $initial_primary_key, array $re
+      $canonicalize = TRUE;

I think the whole point of the test is to test the order of the columns so doing this makes the test invalid.

I think we need to understand why concurrency is impacting this test.

Back to needs work.

If we do end up wanting to sort the arrays, a note that the way to do that now in phpunit is ::assertEqualsCanonicalizing.

But given we're testing the sorting of the primary key, agreed that's not a viable fix anyway.

BTW, I posted #3119170: Optimize pgsql/Schema::findPrimaryKeyColumns() for Postgres >= 9.6 which is probably of interest to people here. Not yet sure if it will actually impact this, but it's certainly related.

1. Tests repeated 100 times confirmed that the problem only affects the old version (PostgreSQL 9.1). See #39.

2. PostgreSQL version requirements raised to 10.0 so this issue is no longer critical.
It may still appear in older versions of drupal (8.8 and 8.9), which is why I leave the issue open.

Affected Drupal 8 with PostgreSQL 9.1.

Confirmation that it is no longer occur in Drupal 9.

No longer occur in Drupal 9 because PostgreSQL version requirements raised to 10.0.. Let's close this issue.