Fix dropping of columns in a composite primary key on MariaDB >=10.2.8 and make it consistent across database drivers

What happens on other dbs to the primary key? Reading https://mariadb.com/kb/en/library/alter-table/#drop-column leaves we to suspect that we need to take care of re-creating the primary key without the column.

Would be nice to do #2881522: Add a Schema::findPrimaryKeyColumns method to remove database specific logic from test first so to get a common method that introspects the primary keys across different db platforms.

mysql> CREATE TABLE a (
    ->   a int,
    ->   b int,
    ->   primary key (a,b)
    -> );
Query OK, 0 rows affected (0.03 sec)

mysql> desc a;
+-------+---------+------+-----+---------+-------+
| Field | Type    | Null | Key | Default | Extra |
+-------+---------+------+-----+---------+-------+
| a     | int(11) | NO   | PRI | NULL    |       |
| b     | int(11) | NO   | PRI | NULL    |       |
+-------+---------+------+-----+---------+-------+
2 rows in set (0.02 sec)

mysql> ALTER TABLE a DROP COLUMN a;
Query OK, 0 rows affected (0.04 sec)
Records: 0  Duplicates: 0  Warnings: 0

mysql> desc a;
+-------+---------+------+-----+---------+-------+
| Field | Type    | Null | Key | Default | Extra |
+-------+---------+------+-----+---------+-------+
| b     | int(11) | NO   | PRI | NULL    |       |
+-------+---------+------+-----+---------+-------+
1 row in set (0.00 sec)

Testing on MySQL Server version: 5.7.19 Homebrew

I have reviewed you for humans patch from comment #15. The patch looks good. I do have some remarks:

1. +++ a/core/lib/Drupal/Core/Database/Driver/mysql/Schema.php
   @@ -460,7 +460,24 @@ public function dropField($table, $field) {
   +    $primary_key = array_keys($this->connection->query('SHOW INDEX FROM {' . $table . '} WHERE key_name = ' . $this->connection->quote('PRIMARY'))->fetchAllAssoc('Column_name'));
   

   Can we use the method Schema-> findPrimaryKeyColumns() here?

2. +++ a/core/lib/Drupal/Core/Database/Driver/pgsql/Schema.php
   @@ -249,7 +249,7 @@ protected function createTableSql($name, $table) {
   -    if (isset($table['primary key']) && is_array($table['primary key'])) {
   +    if (!empty($table['primary key']) && is_array($table['primary key'])) {
   

   I do not see why this change is necessary. Please explain. :)

3. +++ a/core/lib/Drupal/Core/Database/Driver/pgsql/Schema.php
   @@ -606,7 +606,20 @@ public function dropField($table, $field) {
   +    if ((count($primary_key) > 1) && in_array($field, $primary_key, TRUE)) {
   +      $this->addPrimaryKey($table, array_diff($primary_key, [$field]));
   +    }
   

   Is it maybe better to calculate from the saved primary key what the new primary should be and test that to the actual primary key. Just an idea.

4. +++ a/core/tests/Drupal/KernelTests/Core/Database/SchemaTest.php
   @@ -699,6 +699,116 @@ protected function assertFieldCharacteristics($table_name, $field_name, $field_s
   +  public function testSchemaCreateTablePrimaryKey(array $primary_key) {
   ...
   +    $expected = array_values(array_diff($primary_key, ['test_field']));
   

   Can we add an extra parameter to the test method. It should be the expected primary key. The test method should not calculate it.

5. +++ a/core/tests/Drupal/KernelTests/Core/Database/SchemaTest.php
   @@ -699,6 +699,116 @@ protected function assertFieldCharacteristics($table_name, $field_name, $field_s
   +   * Tests adding and dropping fields that are part of the primary key.
   ...
   +  public function testSchemaAddFieldPrimaryKey($initial_primary_key, $new_primary_key) {
   

   We are not testing the dropping of fields in this method.

6. +++ a/core/tests/Drupal/KernelTests/Core/Database/SchemaTest.php
   @@ -699,6 +699,116 @@ protected function assertFieldCharacteristics($table_name, $field_name, $field_s
   +    $field_spec = ['type' => 'int'];
   ...
   +        'other_test_field' => $field_spec,
   ...
   +    $schema->addField($table_name, 'test_field', $field_spec, ['primary key' => $new_primary_key]);
   

   I think that the readability improves if we remove the variable $field_spec and just put ['type' => 'int'] in its place.

7. +++ a/core/tests/Drupal/KernelTests/Core/Database/SchemaTest.php
   @@ -699,6 +699,116 @@ protected function assertFieldCharacteristics($table_name, $field_name, $field_s
   +    $connection = Database::getConnection();
   +    $schema = $connection->schema();
   ...
   +    $connection = Database::getConnection();
   +    $schema = $connection->schema();
   

   Can we replace these two lines with:

       /** @var \Drupal\Core\Database\Schema $schema */
       $schema = Database::getConnection()->schema();
   

   .
   The variable $connection is never used.

8. +++ a/core/tests/Drupal/KernelTests/Core/Database/SchemaTest.php
   @@ -699,6 +699,116 @@ protected function assertFieldCharacteristics($table_name, $field_name, $field_s
   +  public function testSchemaCreateTablePrimaryKey(array $primary_key) {
   ...
   +  public function testSchemaAddFieldPrimaryKey($initial_primary_key, $new_primary_key) {
   

   Can we add: @covers ::findPrimaryKeyColumns

9. I am missing testing with the method Schema->changeField().

@tstoeckler:

For #21.2 Can you add a comment to the patch.

For #21.3 My idea was that it would maybe better use the saved primary key and use it to calculate what the new paimary key should be.
Then after the column drop test if the primary key is the same as the expected primary key. If not then update the primary key.

I think the more correct behavior would actually be to drop the primary key entirely instead of recreating a possibly broken primary key.

The big question for me is what other developers expect when they use the functionality. My idea is that they will not expect that the primary key will be dropped. I think that we should be as helpful to them as we can be, therefor update the primary key.

We do not have a subsystem maintainer any more since @crell left.

The requested code example:

  $primary_key = $this->findPrimaryKeyColumns($table);

  $this->connection->query('ALTER TABLE {' . $table . '} DROP COLUMN "' . $field . '"');

  $expected_primary_key = array_values(array_diff($primary_key, [$field]));

  if ($expected_primary_key != $this->findPrimaryKeyColumns($table)) {
    $this->addPrimaryKey($table, $expected_primary_key);
  }

I am not saying that my solution is better. It is just an idea.

For #21.2: After some thinking I come to the conclusion that if we want to solve correctly then we also need testing for it. And therefor it needs its own issue. The problem is that it is very subtle change. I know that I would easily mix it up. Sorry :(

The patch looks great to me! I love the fact that we're normalizing this behavior across all db drivers :)

1. +++ a/core/lib/Drupal/Core/Database/Driver/mysql/Schema.php
   @@ -460,7 +460,24 @@ public function dropField($table, $field) {
   +    // this recreation for compatibility. See
   +    // https://mariadb.com/kb/en/library/alter-table for more information.
   
   +++ a/core/lib/Drupal/Core/Database/Driver/pgsql/Schema.php
   @@ -606,7 +606,20 @@ public function dropField($table, $field) {
   +    // case. See
   +    // https://www.postgresql.org/docs/current/static/sql-altertable.html for
   +    // more information.
   

   I would change these to only @see URL on the next comment line, without the "see ... for more information." part.

2. +++ a/core/tests/Drupal/KernelTests/Core/Database/SchemaTest.php
   @@ -700,9 +700,107 @@ protected function assertFieldCharacteristics($table_name, $field_name, $field_s
   +   * @covers ::findPrimaryKeyColumns
   

   We should mention that we're also testing ::addField, ::changeField and ::dropField here.

My 2 cents... sorry I did not go through the entire issue, comment based on the IS.

IMHO when you are dropping a field that is part of the primary key, most likely you are in a transient situation where you'll probably be doing more changes to the table (adding new fields for example) before you'll re-create the primary key itself. However, I do not think we can assume that in the atomic dropField method. Dropping and recreating the primary key at that point is more or less an assumption; the calling code should be in charge of identifying if a field is part of the pk, drop it, drop the field, do more stuff and eventually create the new pk. I believe the right thing to do in dropField would be to throw an exception if you are trying to drop a field that is part of the PK - and avoid the risk of ending up with a wrong pk. Do not know how badly this will break things, though :(

I just read through the documentation linked in the patch and my opinion is that PostgreSQL's and MariaDB's >= 10.2.8 behavior makes sense: after dropping a field that is part of a composite primary key, we have no idea if the resulting primary key without that field is still correct for the table or not.

A few examples in core also support this conclusion. For example, dropping the langcode field from an entity revision data table makes the resulting primary key (id + revision_id) unusable. Same for all the other entity tables with composite primary keys.

So I think it's fair to leave it up to the "application"/caller to decide and act upon whether they want to re-instate the primary key with the remaining columns or not.

Great to see that @mondrake has the same opinion :)

As for the choice between dropping the primary key automatically in dropField() (PostgreSQL's behavior) and raising an exception (MariaDB' >= 10.2.8 behavior), I'm afraid we don't have much choice because of the BC implications, so we can realistically only choose the first option and drop the PK automatically.

The patch looks good to me.

I have one remark left. There is no testing for an empty primary key. In the issue summary we state that we will allow the creation of tables without a primary key. After which the patch is RTBC for me.

For the dropping of a combined primary key, the argument of:

As for the choice between dropping the primary key automatically in dropField() (PostgreSQL's behavior) and raising an exception (MariaDB' >= 10.2.8 behavior), I'm afraid we don't have much choice because of the BC implications, so we can realistically only choose the first option and drop the PK automatically.

I for me a great argument to change my position on the issue.

I am not is a position to manual test this patch with MariaDB's >= 10.2.8. It would be great if somebody else could do that.

Thanks @tstoeckler: great work from you.

Now we have to wait on #2881522: Add a Schema::findPrimaryKeyColumns method to remove database specific logic from test.

Also we have to wait for somebody who can manual test this patch with MariaDB's >= 10.2.8.

For the rest is the patch RTBC for me.

#2881522: Add a Schema::findPrimaryKeyColumns method to remove database specific logic from test has landed. Started the testbot for the for review patch.

Looks good to me.
The test only patch show that there is a bug and the full patch fixes that bug.
The code for this patch comes from is parent issue: #2881522: Add a Schema::findPrimaryKeyColumns method to remove database specific logic from test
For me there is enough testing for the problem.

What is mis is for somebody to manual test this patch with MariaDB's >= 10.2.8.

For the rest is the patch RTBC for me.

1. +++ b/core/tests/Drupal/KernelTests/Core/Database/SchemaTest.php
   @@ -704,9 +704,119 @@ protected function assertFieldCharacteristics($table_name, $field_name, $field_s
   +    $method = new \ReflectionMethod(get_class($schema), 'findPrimaryKeyColumns');
   

   Let's use a more descriptive name than $method as that will make the test more readable.

2. +++ b/core/lib/Drupal/Core/Database/Driver/sqlite/Schema.php
   @@ -633,8 +635,8 @@ protected function mapKeyDefinition(array $key_definition, array $mapping) {
   -      if (isset($mapping[$field])) {
   -        $field = $mapping[$field];
   +      if (in_array($field, $mapping, TRUE)) {
   +        $field = array_search($field, $mapping);
          }
   

   There's no need to do in_array() followed by array_search(). We can do something like:

         $mapped_field = array_search($field, $mapping, TRUE);
         if ($mapped_field !== FALSE) {
           $field = $mapped_field;
         }
   

   Also it would be great is there was documentation about what the mapping is. As far as I can see its a list that maps [new] => [old]. Where the keys are the new field names and the values are the old field names.

   What's really strange is the code that creates the mapping..

       // Map the old field to the new field.
       if ($field != $field_new) {
         $mapping[$field_new] = $field;
       }
       else {
         $mapping = [];
       }
   

   Er... no... you've mapped the new field to the old field!

   In fact it is almost tempting to fix the caller here to do what it says and do:

       // Map the old field to the new field.
       if ($field != $field_new) {
         $mapping[$field] = $field_new;
       }
       else {
         $mapping = [];
       }
   

   I.e. swap $field and $field_new.

The framework manager's review has been addressed, so removing that tag. The patch has also been manually tested by @tstoeckler on MariaDB >= 10.2.8.

Can't find anything else to point out, not even nitpicks :)

I was asked by @tstoeckler to execute the test \Drupal\KernelTests\Core\Database\SchemaTest::testSchemaChangePrimaryKey() on my system, which runs with MariaDB 10.3.7. I confirm that the test passes. Great work! +1 RTBC.

Crediting reviewers whose reviews affected the patch.

Committed 8faf0fa and pushed to 8.6.x. Thanks!

We can only do this in 8.6.x because #2881522: Add a Schema::findPrimaryKeyColumns method to remove database specific logic from test was an 8.6.x only change.

This introduced intermittent test failure on PostgreSQL 9.1
https://www.drupal.org/pift-ci-job/986641
https://www.drupal.org/pift-ci-job/991466

Follow-up: #2982755: Random failure in SchemaTest::testSchemaChangePrimaryKey with order of composite primary key