Problem/Motivation

- DBTNG does not support adding a serial field to an existing table, even if all the supported DB engines can do that just fine
- DBTNG does not handle primary keys properly when adding a new field to an existing table. \Drupal\Core\Database\Schema::changeField() documents that primary keys have to be handled manually by the calling code, but \Drupal\Core\Database\Schema::addField() documents that it handles primary keys by itself.

Proposed resolution

Fix DBTNG to support adding a serial field to an existing table.

Fix implementations of \Drupal\Core\Database\Schema::addField() to handle primary key changes automatically.

Remaining tasks

Review.

User interface changes

Nope.

API changes

Nope.

Data model changes

Nope.

CommentFileSizeAuthor
#83 interdiff_82-83.txt1.3 KBpoker10
#83 2615496-83.patch7.71 KBpoker10
#82 2615496-82.patch7.33 KBpoker10
#68 interdiff.txt1.41 KBDavid_Rothstein
#68 D7-2615496-68.patch7.36 KBDavid_Rothstein
#65 interdiff_63_to_65.txt585 bytesjoseph.olstad
#65 D7-2615496-65.patch7.38 KBjoseph.olstad
#63 D7-2615496-63.patch7.75 KBjoseph.olstad
#63 interdiff_61_to_63.txt620 bytesjoseph.olstad
#62 interdiff_61_to_62.txt620 bytesjoseph.olstad
#62 D7-2615496-62.patch7.75 KBjoseph.olstad
#61 D7-2615496-schema_inc-61.patch7.84 KBjoseph.olstad
#61 interdiff_51_to_61.txt585 bytesjoseph.olstad
#54 D7-baseline_postgresql_test.patch387 bytesjoseph.olstad
#51 interdiff_49_to_51.txt1.58 KBjoseph.olstad
#51 D7-2615496-51.patch7.46 KBjoseph.olstad
#49 interdiff_46_to_49.txt2.31 KBjoseph.olstad
#49 D7-2615496-49.patch7.28 KBjoseph.olstad
#46 D7-2615496-46.patch7.24 KBjoseph.olstad
#35 interdiff.txt2.06 KBamateescu
#35 2615496-35.patch7.38 KBamateescu
#31 interdiff.txt3.33 KBamateescu
#31 2615496-31.patch7.63 KBamateescu
#27 interdiff.txt3.71 KBamateescu
#27 2615496-27.patch7.35 KBamateescu
#23 2615496-23.patch4.81 KBamateescu
#23 2615496-23-test-only.patch1.68 KBamateescu
#14 d8-2615496-mysql_schema_add_field_serial_type-13.patch622 bytesjoseph.olstad
#14 d7-2615496-mysql_schema_add_field_serial_type-13.patch527 bytesjoseph.olstad
#11 d7-2615496-mysql_schema_add_field_serial_type-11.patch527 bytesjoseph.olstad
#9 d8-2615496-mysql_schema_add_field_serial_type.patch622 bytesjoseph.olstad
#2 d7-mysql_schema_add_field_serial_type.patch527 bytesamcgowanca
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dustise created an issue. See original summary.

amcgowanca’s picture

I ran into the same issue when updating the Backup & Migrate module to the latest version.

djdevin’s picture

This is an issue with MySQL 5.7, trying to add a new serial field will result in:

SQLSTATE[42000]: Syntax error or access violation: 1171 All parts of a PRIMARY KEY must be NOT NULL; if you need NULL in a key, use UNIQUE instead

Does not happen with MySQL <= 5.6

Using an int field works, but then you have to manually make it auto-increment.

Applying the patch made it work.

Similar to #2665362: D7 upgrades fail with mysql 5.7:

Example of an update that fails on 5.7: http://cgit.drupalcode.org/quiz/tree/quiz.install#n877

greggles’s picture

Title: Can't add a serial field with db_add_field » Can't add a serial field with db_add_field on MySQL 5.7
Status: Active » Needs review

I ran into this issue as well related to the login_history module.

The patch in #2 fixes it for me.

Thanks, @amcgowanca!

greggles’s picture

Status: Needs review » Reviewed & tested by the community

This actually seems RTBC to me. I'm about to have a contrib release that depends on it and would rather have a hook_requirements that looks for an up to date core than instructing people to install a patch.

joseph.olstad’s picture

+1 by @rudiedirkx

#2428371: Upgrade from 1.3 to newer (1.4, 1.5, 3.x) with drush fails.

see comments

MySQL 5.7 seems to have gotten stricter, and now Drupal must too.

Fabianx’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs check if D8 is affected

Looks good to me, is D8 affected, too?

+++ b/includes/database/mysql/schema.inc
@@ -329,7 +329,7 @@ class DatabaseSchema_mysql extends DatabaseSchema {
+    if ('serial' != $spec['type'] && !empty($spec['not null']) && !isset($spec['default'])) {

In Drupal coding standards we use:

$spec['type'] != 'serial'

--

Also this needs a comment to explain why we want to check this.

Else someone else might be tempted to remove this in the future.

Setting to CNW for the missing comment.

Fabianx’s picture

Priority: Normal » Major
Issue tags: +Drupal bugfix target
joseph.olstad’s picture

Based on suggestion by Fabianx, here is the D8 version of this patch

To follow is the updated D7 version.

joseph.olstad’s picture

Status: Needs work » Needs review
joseph.olstad’s picture

joseph.olstad’s picture

Issue tags: -Needs check if D8 is affected

Status: Needs review » Needs work

The last submitted patch, 11: d7-2615496-mysql_schema_add_field_serial_type-11.patch, failed testing.

The last submitted patch, 14: d7-2615496-mysql_schema_add_field_serial_type-13.patch, failed testing.

joseph.olstad’s picture

kick the testbot until this passes.

D8 patch passed all tests, I expect both patch 13 (d7/d8) to pass as well and if they don't keep kicking them until they do.

I prefer Patches 13 , I think (based on my interpretation of the code) should be better performance than the previous ones.

Fabianx’s picture

Version: 7.x-dev » 8.2.x-dev
Issue tags: +Needs D7 backport

RTBC for #9 then in Drupal 8.

joseph.olstad’s picture

Suggest #13 (14) as preferred. Although I have not profiled performance it is likely more efficient than #9. Looks prettier because 13/14 is in original pre-patch order

Thx

Fabianx’s picture

RTBC for #14 is okay with me, too.

Order should not matter at all as there is no function calls, but I am fine either way.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

djdevin’s picture

Version: 8.3.x-dev » 8.4.x-dev
Issue tags: +Triaged for D8 major current state

Still an issue and patch applies cleanly to 8.4.x-dev.

7.x patch applies cleanly too.

Unfortunately testbot doesn't have 5.7, but it does fail and the patch does fix it.

alexpott’s picture

Discussed with @catch, @cilefen, @cottser, @laurii, and @xjm. We agreed that this is a major issue as there is no work around and this is part of another major bug - #2841291: Fix NOT NULL handling in the entity storage and 'primary key' changes when updating the storage definition of an identifier field

amateescu’s picture

The problem is not only about serial fields, but all added fields that will be part of the table's primary key. And it's not limited to the MySQL driver, Postgres has the same problem.

Here's the relevant parts from the patch in #2841291: Fix NOT NULL handling in the entity storage and 'primary key' changes when updating the storage definition of an identifier field.

The last submitted patch, 23: 2615496-23-test-only.patch, failed testing.

jibran’s picture

Status: Needs review » Reviewed & tested by the community
alexpott’s picture

Status: Reviewed & tested by the community » Needs review
+++ b/core/tests/Drupal/KernelTests/Core/Database/SchemaTest.php
@@ -143,6 +143,24 @@ public function testSchema() {
+    db_add_field('test_table', 'test_serial', ['type' => 'serial', 'not null' => TRUE], ['primary key' => ['test_serial']]);
...
+    // Test adding a new column and form a composite primary key with it.
+    db_add_field('test_table', 'test_composite_primary_key', ['type' => 'int', 'not null' => TRUE, 'default' => 0], ['primary key' => ['test_serial', 'test_composite_primary_key']]);

Shouldn't we have some assertion that the primary key is as expected?

timmillwood’s picture

Status: Needs review » Reviewed & tested by the community

#27 looks good.

timmillwood’s picture

Status: Reviewed & tested by the community » Needs work

ah, just saw the test, not so good got postgres and sqlite.

tstoeckler’s picture

+++ b/core/lib/Drupal/Core/Database/Driver/mysql/Schema.php
@@ -395,14 +395,24 @@ public function addField($table, $field, $spec, $keys_new = []) {
+    // Fields that are part of a PRIMARY KEY can not be added as NOT NULL.

+++ b/core/lib/Drupal/Core/Database/Driver/pgsql/Schema.php
@@ -543,8 +543,11 @@ public function addField($table, $field, $spec, $new_keys = []) {
+    // Fields that are part of a PRIMARY KEY can not be added as NOT NULL.

This comment is reversed, right? I.e. fields that are part of a primary key *must* be NOT NULL, i.e. cannot be added as not NOT NULL ?!

amateescu’s picture

lokapujya’s picture

Status: Needs review » Reviewed & tested by the community

Ran a test by recreating a table and using the API to add a primary key.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
  1. It is great to get stuff like this fixed
  2. +++ b/core/lib/Drupal/Core/Database/Driver/sqlite/Schema.php
    @@ -353,7 +353,7 @@ public function addField($table, $field, $specification, $keys_new = []) {
    -      $new_schema += $keys_new;
    +      $new_schema = array_merge($new_schema, $keys_new);
    

    Sometimes I feel as though we should ban array addition. So many bugs.

  3. +++ b/core/lib/Drupal/Core/Database/Driver/sqlite/Schema.php
    @@ -430,16 +430,16 @@ protected function alterTable($table, $old_schema, $new_schema, array $mapping =
    -  protected function introspectSchema($table) {
    +  public function introspectSchema($table) {
    
    +++ b/core/tests/Drupal/KernelTests/Core/Database/SchemaTest.php
    @@ -804,4 +826,38 @@ public function testFindTables() {
    +      case 'sqlite':
    +        $table_info = Database::getConnection()->schema()->introspectSchema($table_name);
    +        $this->assertSame($primary_key, $table_info['primary key']);
    

    Hmmm this is interesting. We're expanding the scope here to increase the public API of the SQLite driver. Rather than changing this here can we not just do the query on the database like we do for MYSQL and PSQL?

amateescu’s picture

Status: Needs work » Reviewed & tested by the community

@alexpott, the reason why I chose to make that method public is that we can't run this query 'PRAGMA ' . $info['schema'] . '.table_info(' . $info['table'] . ')' from the outside. Specifically, we don't have access to the table prefixes which are internal to the SQLite schema driver (i.e. \Drupal\Core\Database\Schema::getPrefixInfo() is protected).

Also, table_info() is not a real table, so we can't use \Drupal\Core\Database\Driver\sqlite\Connection::getFullQualifiedTableName() (which is already a public method) on it.

Maybe your concern could be addressed by marking it @internal?

amateescu’s picture

As discussed with @alexpott on IRC, let's use reflection instead of making that method public.

jibran’s picture

Even better. RTBC +1.

The last submitted patch, 11: d7-2615496-mysql_schema_add_field_serial_type-11.patch, failed testing.

alexpott’s picture

Bugs fixed in all 3 core supported database drivers and new test coverage. Nice work!

Updating issue credit.

alexpott’s picture

Status: Reviewed & tested by the community » Patch (to be ported)

Committed and pushed 45d8336 to 8.4.x and de15524 to 8.3.x. Thanks!

I've backported this to 8.3.x because it is a major self-contained bug with no API changes. Setting to patch to be ported so the D7 issue can be created.

  • alexpott committed 45d8336 on 8.4.x
    Issue #2615496 by amateescu, joseph.olstad, amcgowanca, alexpott,...

  • alexpott committed de15524 on 8.3.x
    Issue #2615496 by amateescu, joseph.olstad, amcgowanca, alexpott,...
joseph.olstad’s picture

Version: 8.4.x-dev » 7.x-dev
joseph.olstad’s picture

Status: Patch (to be ported) » Reviewed & tested by the community
Issue tags: -Needs D7 backport, -Triaged core major

RTBC +1
Tested this with mysql 5.7.x , we require this core patch for upgrading workbench_moderation from 1.3.x to 3.x.

Without patch :
Syntax error or access violation: 1171 All parts of a PRIMARY KEY must be NOT NULL; if you need NULL in a key, use UNIQUE instead

With patch:
Works without errors.

I recommend the D7 patch be targetted for next release.

adjusting tags, it's already been committed to D8, and the D7 patch has been ready for a long time so it doesn't need a backport.

Commit for the win.

amateescu’s picture

Status: Reviewed & tested by the community » Needs work

The patch that was committed to 8.x (#35) is *very* different than the latest 7.x patch (#14), so someone needs to actually write a 7.x version of the patch in #35 before this can be RTBC.

joseph.olstad’s picture

@amateescu , ok, new D7 patch to follow shortly

joseph.olstad’s picture

Status: Needs work » Needs review
FileSize
7.24 KB

See what the testbot says.

Status: Needs review » Needs work

The last submitted patch, 46: D7-2615496-46.patch, failed testing.

joseph.olstad’s picture

Status: Needs work » Needs review

syntax issue
need to rework array syntax for php 5.3 compatibility

New patch to follow shortly

joseph.olstad’s picture

Status: Needs review » Needs work

The last submitted patch, 49: D7-2615496-49.patch, failed testing.

joseph.olstad’s picture

Changing from assertSame to assertEqual fixed it on my local environment, I expect patch 51 to pass the mysql tests, not sure about the others like Postgres or sqlite because I don't have a test environment for those.

joseph.olstad’s picture

hmm, not sure why the postgresql test is failing, perhaps head is also failing.

joseph.olstad’s picture

joseph.olstad’s picture

Status: Needs review » Reviewed & tested by the community

patch 51 is a valid D7 backport of what was committed to D8.

Test results on Postgres and sqlite are tainted by what appears to be jenkins configuration issues (and or head issues) with the test postgres/sqlite environments and db abstraction layers.

joseph.olstad’s picture

Suggest we commit the MySql part of this patch which we are 100% confident with and postpone the sqlite and postgresql portion until head passes for those respective db systems.

Easily whip up another patch .

Recommend that we move forward quickly on this.

tstoeckler’s picture

Status: Reviewed & tested by the community » Needs review

@joseph.olstad Please do not mark your own patch as RTBC. Even if it is just a backport, someone should verify that and also we need to review that it properly fixes the problem in D7.

joseph.olstad’s picture

@tstoeclker , please provide a reason why this is NOT RTBC.

joseph.olstad’s picture

joseph.olstad’s picture

FileSize
7.75 KB
620 bytes

removed commented out line that had no place in the patch. For test results, refer to patch 61. See interdiff for details.

amateescu’s picture

+++ b/includes/database/sqlite/schema.inc
@@ -407,7 +407,7 @@ class DatabaseSchema_sqlite extends DatabaseSchema {
-  protected function introspectSchema($table) {
+  public function introspectSchema($table) {

This change is not correct. @alexpott argued for the exact opposite thing, that we should not make this protected method public.

joseph.olstad’s picture

FileSize
7.38 KB
585 bytes

@amateescu

Yes definately, here's a new patch.

joseph.olstad’s picture

IMHO rtbc this backport. As for postgresql and sqlite, there seems to be a problem with the Testbot /docker config or Jenkins for those. Test results often inconsistent, does not reflect upon the quality of this backport. At very least, fast track the MySql commit, it is needed for MySql 5.7.x and higher. Currently Testbot is only 5.5.x.

mlncn’s picture

Status: Needs review » Reviewed & tested by the community

Tested D7-2615496-65.patch, albeit MySQL only. Good to go.

David_Rothstein’s picture

There were some minor issues in the tests (for PHP 5.2 and Drupal 7 compatibility) which I've fixed in the attached. Will run this against all 3 databases and we can compare it to https://www.drupal.org/node/1970534#comment-12115998

Overall, the patch looks good, but:

  1.      if ($keys_sql = $this->createKeysSql($keys_new)) {
    +      // Make sure to drop the existing primary key before adding a new one.
    +      // This is only needed when adding a field because this method, unlike
    +      // changeField(), is supposed to handle primary keys automatically.
    +      if (isset($keys_new['primary key']) && $this->indexExists($table, 'PRIMARY')) {
    +        $query .= ', DROP PRIMARY KEY';
    +      }
    

    I am not entirely sure why we want to make this change (vs. simply documenting that you have to explicitly call db_drop_primary_key() first, which in practice has always been the case).

    It seems unrelated to the original bugfix here. And I am not totally convinced that it is good to have this kind of magic. db_add_field() is about adding fields... so why should it magically delete primary keys rather than simply failing (like it currently does) if you try to add an illegal field? If a developer adds a new field with a primary key under the mistaken belief that the table doesn't have a primary key yet, shouldn't they be notified of that rather that just having the primary key silently switched?

  2. --- a/includes/database/sqlite/schema.inc
    +++ b/includes/database/sqlite/schema.inc
    @@ -324,7 +324,7 @@ class DatabaseSchema_sqlite extends DatabaseSchema {
           }
     
           // Add the new indexes.
    -      $new_schema += $keys_new;
    +      $new_schema = array_merge($new_schema, $keys_new);
    

    The old behavior was clearly broken, but I'm not convinced array_merge() is correct here either. Suppose a table has multiple 'indexes'. array_merge() will remove all the old indexes and only use the ones passed in via db_add_field(). That means if you are adding a new field and adding an index for it, you also have to remember to pass in every existing index (for other fields in the same table) or SQLite will delete them? That is not consistent with how it works in MySQL, for example, which only requires that you pass in any indexes that you're changing (and leaves other existing ones alone).

    So I suspect the correct code is more like $new_schema['indexes'] = array_merge($new_schema['indexes'], $keys_new['indexes']) and similar for the other types of keys?

    At least this is worth a followup issue, for Drupal 8 also. I'm not sure if it's worth skipping the above change in Drupal 7 until this is addressed (depends on which behavior we think is more broken).

Overall, I think this should be held until Drupal 7.60 since the fixes are a bit risky and it could use more review.

Limiting the patch to just be about the broken NULL handling for primary keys (on MySQL and PostgreSQL) could also help get it in faster.

David_Rothstein’s picture

Issue tags: -Drupal bugfix target +Drupal 7.60 target

Status: Needs review » Needs work

The last submitted patch, 68: D7-2615496-68.patch, failed testing.

amateescu’s picture

Status: Needs work » Needs review

I am not entirely sure why we want to make this change (vs. simply documenting that you have to explicitly call db_drop_primary_key() first, which in practice has always been the case).

That's because of composite primary keys :) When you have a table that contains a serial field (which is by default part of the primary key), you can not drop the existing primary key of that table before adding the new field. This exact scenario is now tested so it's quite easy to check for yourself locally, just comment out that code in addField() and try to run SchemaTestCase on MySQL.

So I suspect the correct code is more like $new_schema['indexes'] = array_merge($new_schema['indexes'], $keys_new['indexes']) and similar for the other types of keys?

I think you're right, but wouldn't it be simpler to just do a nested merge instead?

amateescu’s picture

Status: Needs review » Needs work
David_Rothstein’s picture

Not sure how much we can learn from those test failures overall, but one thing is that some of the SQLite failures are real and caused by this patch. That's because:

+        $table_info = $reflection->invoke($schema_object, $table_name);
+        $this->assertEqual($primary_key, array_keys($result), 'sqlite assert expected key column for a tables primary key.');

There is no $result - it should probably be something like $table_info['primary key'] instead.

Regarding #71:

  1. When you have a table that contains a serial field (which is by default part of the primary key), you can not drop the existing primary key of that table before adding the new field.

    You're right, it's not always as simple as just calling db_drop_primary_key(), but I think it still can be done. To remove the primary key from a serial field before adding a new one, I can still get the tests to pass (at least on MySQL) by adding this:

     db_add_unique_key('test_table', 'test_serial', array('test_serial'));
     db_drop_primary_key('test_table');
    

    (and then presumably a db_drop_unique_key() afterwards once the new primary key is created)

    So I guess it is a bit more annoying than I thought in that particular case, but it's no different than what you would have to do with db_change_field() (I think?). So my overall point still stands.

  2. wouldn't it be simpler to just do a nested merge instead?

    I think that would be wrong in the case of some particular array elements. And in fact, the suggestion I had above is actually wrong for the same reasons...

    For example if $new_schema['primary key'] = array('column1') and $keys_new['primary key'] = array('column2') we actually want array('column2') as the result. But either my suggestion or yours would give array('column1', 'column2') in that case.

    So the code for this would actually need to be a bit more careful.

MustangGB’s picture

So what "needs work" here?
Is it the SQLite/PostgreSQL failures, in which case should/can they be handled in a follow-up issue?

joseph.olstad’s picture

Issue tags: -Drupal 7.60 target +Drupal 7.61 target

Bumping to 7.61. This didn't make it into 7.60.

joseph.olstad’s picture

joseph.olstad’s picture

MustangGB’s picture

MustangGB’s picture

Issue tags: -Drupal 7.68 target +Drupal 7.69 target
izmeez’s picture

Patch in #68 applies cleanly to the drupal 7.69 release and appears essential for backup_migrate #3023556: Performed update: backup_migrate_update_7303 is causing SQLSTATE[42000]. It is RTBC despite concerns that it may break functionality of other databases.

MustangGB’s picture

Issue tags: -Drupal 7.69 target +Drupal 7.70 target
poker10’s picture

The patch from #68 does not apply anymore, so adding reroll. Just to see if the test failures are still present (there was a lot of changes in PostgreSQL recently).

poker10’s picture

Well, PostgreSQL seems good. Attaching updated patch with fixed typo with undefined $result. There is one last test failure in SQLite and that is caused by what @David_Rothstein mentioned earlier:

The old behavior was clearly broken, but I'm not convinced array_merge() is correct here either. Suppose a table has multiple 'indexes'. array_merge() will remove all the old indexes and only use the ones passed in via db_add_field(). That means if you are adding a new field and adding an index for it, you also have to remember to pass in every existing index (for other fields in the same table) or SQLite will delete them?

It is interesting, that the same behavior is currently in D8/9. And if we do not have added this recently in D7 (or if it was written as db_add_field() + db_add_index()):

function user_update_7020() {
  ....
    db_add_field('users', 'changed', $spec, $keys);
    // Set the initial value for existing users.
    db_update('users')
      ->expression('changed', 'created')
      ->execute();
  }
}

Then the SQLite tests will be green despite of this problem.

I tried to workaround the array_merge() problem affecting the update of keys with additional logic which will try to keep the current keys and only add new ones (except primary key, which should be overwritten). What do you think?

poker10’s picture

Status: Needs work » Needs review