Change the uid field from integer to serial by leveraging NO_AUTO_VALUE_ON_ZERO on MySQL

We were doing this , this one was one of the few core use cases of the new db_next_id until it got another column...

I'm not against the original proposal to set the MySQL property by default, but I don't quite get why we also need to be changing the schema. Can someone explain that?

Crell, noone can explain this. Ask the MySQL authors, perhaps?

mysql> SET sql_mode='ANSI,TRADITIONAL,NO_AUTO_VALUE_ON_ZERO';
mysql> insert into simpletest_test_id (test_id) values (default);
Query OK, 1 row affected (0.00 sec)
mysql> select * from simpletest_test_id;
+---------+-------------+
| test_id | last_prefix |
+---------+-------------+
|       0 |             |
+---------+-------------+
1 row in set (0.00 sec)

Edit: in short if you use default, it uses the default 0 as dictated by NO_AUTO_VALUE_ON_ZERO. If you do insert into sequences () values (); then life is good... Let's unfurl the "I hate databases" flag.

#3: 838992.patch queued for re-testing.

Hm. I don't really see a compelling reason for fixing this in D7? Am I missing something? It sounds like a nice to have, since we were able to get update tests working without it.

If we do choose to go ahead with it though, it needs an upgrade path now.

A very compelling to commit this patch is the last operation performed in http://api.drupal.org/api/function/system_status/7

More insightful details may be read in #204411: Anonymous user id breaks on MySQL export/import (xID of Zero get's auto-incremented on external xSQL events) (which also explains the WTF of the uid - uid value assignment that happens there).

Actually, I'm not sure why that (and corresponding hiccups in user.install) is not removed in this patch...?

I agree that this patch should be committed. However, as webchick already mentioned, simpletest.install needs a module update to change the schema for existing sites accordingly. Back to RTBC afterwards.

Let's try to do this in Drupal 9.

Amusingly #2279395: The PostgreSQL backend does not handle NULL values for serial fields gracefully has resulted in other DBs behaving like MySQL for serial IDs when inserting a 0 as the ID. So we need to change that a little. I've been conservative with the change... we could gave the code from

if (!(empty($value) && $this->isColumnSerial($table_name, $schema_name))) {

to

if (!($value === NULL && $this->isColumnSerial($table_name, $schema_name))) {

but this feels risky it means that any code which is setting an ID to 0 would now really set it to 0 - I think it is better to leave this generic behaviour alone and hard-code the user zero issue since user is required entity and a very special case.

The patch attached installs and the update is successful on an existing site and user 0 still exists.

I think that this issue should deal with the user ID field because it is the whole point of setting NO_AUTO_VALUE_ON_ZERO

Fixing the update path and testing it. \Drupal\KernelTests\Core\Database\SchemaTest::testSchemaAddFieldDefaultInitial() is going to be a PITA.

It's late so I probably haven't thought of all the options but \Drupal\Core\Database\Query\InsertTrait::useDefaults() might be a significant barrier here. It really is not used much at all... in core it is tests and in contrib... http://codcontrib.hank.vps-private.net/search?text=-%3EuseDefaults%28&fi...

Added test needs @group to be added

Here's an un satisfactory solution to the useDefaults() issue... calling this with a serial column makes no sense - it's not necessary if you provided any other column...

Adding the group - we should remove that requirement - now we have no simpletest UI it's not necessary anymore. And oddly simpletest is the one proper use of useDefaults in contrib.

The patch looks good. It is failing for PostgreSQL.

1. +++ b/core/tests/Drupal/KernelTests/Core/Database/SchemaTest.php
   @@ -717,7 +716,7 @@ protected function assertFieldAdditionRemoval($field_spec) {
   -      ->useDefaults(['serial_column'])
   +      ->fields(['serial_column' => NULL])
   
   @@ -782,7 +781,7 @@ protected function assertFieldCharacteristics($table_name, $field_name, $field_s
   -        ->useDefaults(['serial_column'])
   +        ->fields(['serial_column' => NULL])
   

   These 2 changes make the test fail for PostgreSQL. Not sure how we can solve this the best way. It needs to be done this way because of https://bugs.mysql.com/bug.php?id=89225.

2. +++ b/core/modules/user/user.install
   @@ -97,3 +99,22 @@ function user_install() {
   +  $schema = \Drupal::database()->schema();
   +  $schema->dropPrimaryKey('users');
   +  $schema->changeField('users', 'uid', 'uid', ['type' => 'serial', 'not null' => TRUE], ['primary key' => ['uid']]);
   

   I do not think this is enough for PostgreSQL. It also need a sequence for the primary key to work properly.

3. +++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php
   @@ -1057,7 +1057,12 @@ protected function mapToStorageRecord(ContentEntityInterface $entity, $table_nam
   +        // The user entity is a very special case where the ID field is a serial
   +        // but we need to insert a row with an ID of 0 to represent the
   +        // anonymous user.
   +        $user_zero = $this->entityTypeId === 'user' && $value === 0;
   

   I came to the same conclusion in my testing patch in #3215116: [IGNORE] Testing. It is a ugly solution, only I do not see a better one.

There is an issue for the sequences for PostgreSQL. See #3028706: New serial columns do not own sequences.

Working on this.

Updated the patch for PostgreSQL.

The patch from the previous comment was the correct one.

Add a CR and updated the IS.

I am grateful this issue sees traction finally, thanks for the work!

-      'sql_mode' => "SET sql_mode = 'ANSI,TRADITIONAL'",
+      'sql_mode' => "SET sql_mode = 'ANSI,TRADITIONAL,NO_AUTO_VALUE_ON_ZERO'",

I do not know whether we have a BC policy on this but I think this change is very wide. I think it'd be better to have a separate setting and even that is only necessary for new sites as existing sites already have their anonymous user. Even with the separate setting it's not impossible some contrib or in house reuse modules would break.

Maybe keep the change as is but do this in Drupal 10 only. The delay is not bad: if this goes in now, it gets released in December, if it doesn't, it releases next summer anyways. We could announce now so people relying on this can prepare and put it in D10.

What do you think?

Personally I do not see a problem, but lets ask for the opinion of a release manager for changing the default SQL mode for MySQL.

Also I do not see a problem with this being a BC break, only having some input on that from @alexpott or @catch would be great. I do not want to break an existing site.

@chx: Thank you for your reply!

I think we can do something a little simpler with the postgres statefulness... this only happens in the test runner when it's run the update in another process.

Here's another approach that means we only change the SQL mode on MySQL when inserting user 0. It limits the impact but gives us the ability to have serial user columns. So there are more queries when we insert user 0 but there are less when we insert any other user.

If we go down this path, the other thing we might want / need to do is to add the NO_AUTO_VALUE_ON_ZERO in the db dumps we make. This would copy mysqldump behaviour.

Here's the DbDumpCommand fix. Tested in locally on MySQL with serial uid column - works great.

Great approach. Maybe we need to set the mode during the mysql part of the update, too.

typo.

Bahh!

+++ b/core/lib/Drupal/Core/Command/DbDumpCommand.php
@@ -410,9 +410,15 @@ protected function getTemplate() {
+// Ensure any tables with a serial column with a value of 0 are created as
+// expected.
+$sql_mode = $connection->query("SELECT @@sql_mode;")->fetchField();
+$connection->query("SET sql_mode = '$sql_mode,NO_AUTO_VALUE_ON_ZERO'");
...
+// Reset the SQL mode.
+$connection->query("SET sql_mode = '$sql_mode'");

I think we need here a if-statement for driver is mysql.

#41 addresses #42 :)

@alexpott: No it does not. #41 fixes the change in user.install and #42 is about the change in DbDumpCommand.php.

@daffie - from the top of DbDumpCommand

 * @todo This command is currently only compatible with MySQL. Making it
 *   backend-agnostic will require \Drupal\Core\Database\Schema support the
 *   ability to retrieve table schema information. Note that using a raw
 *   SQL dump file here (eg, generated from mysqldump or pg_dump) is not an
 *   option since these tend to still be database-backend specific.

There's a lot of MySQL specific stuff in the command already...

+++ b/core/modules/user/src/UserStorage.php
@@ -18,21 +18,22 @@ class UserStorage extends SqlContentEntityStorage implements UserStorageInterfac
-    // The anonymous user account is saved with the fixed user ID of 0.
-    // Therefore we need to check for NULL explicitly.
-    if ($entity->id() === NULL) {
-      $entity->uid->value = $this->database->nextId($this->database->query('SELECT MAX([uid]) FROM {' . $this->getBaseTable() . '}')->fetchField());
-      $entity->enforceIsNew();

This change is quite a big win for reducing queries when creating a user. It's going to result in 3 less queries for inserting a user with an ID > 0. The two queries here and the query to clean up the sequences table in shutdown. We even do one less query on MYSQL when we insert user 0 as we don't do the clean up on shutdown. For all other DBs we support the savings are for user 0 too.

I think given the switch to not affect any other query other than user CRUD means that the "needs release manager review" from #30 is not necessary anymore. That said, I'm pretty sure @catch is going to review this one too.

Yeah keeping things conservative in this patch seems sensible, I think we can open follow-ups to remove some of the special casing though.

1. +++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php
   @@ -1057,7 +1057,12 @@ protected function mapToStorageRecord(ContentEntityInterface $entity, $table_nam
            $value = SqlContentEntityStorageSchema::castValue($definition->getSchema()['columns'][$column_name], $value);
   -        if (!(empty($value) && $this->isColumnSerial($table_name, $schema_name))) {
   +        $empty_serial = empty($value) && $this->isColumnSerial($table_name, $schema_name);
   +        // The user entity is a very special case where the ID field is a serial
   +        // but we need to insert a row with an ID of 0 to represent the
   +        // anonymous user.
   +        $user_zero = $this->entityTypeId === 'user' && $value === 0;
   +        if (!$empty_serial || $user_zero) {
              $record->$schema_name = $value;
   

   Do we want a follow-up to try to change the behaviour here? Seems like a potential minefield, remember the issue that introduced this being tricky enough.

2. +++ b/core/modules/user/src/UserStorage.php
   @@ -18,21 +18,22 @@ class UserStorage extends SqlContentEntityStorage implements UserStorageInterfac
   -      $entity->enforceIsNew();
   +    // The anonymous user account is saved with the fixed user ID of 0. MySQL
   +    // does not support inserting an ID of 0 into serial field unless the SQL
   +    // mode is set to NO_AUTO_VALUE_ON_ZERO.
   +    if ($entity->id() === 0) {
   +      $database = \Drupal::database();
   +      if ($database->databaseType() === 'mysql') {
   +        $sql_mode = $database->query("SELECT @@sql_mode;")->fetchField();
   +        $database->query("SET sql_mode = '$sql_mode,NO_AUTO_VALUE_ON_ZERO'");
   +      }
        }
   

   Let's definitely create a follow-up to make NO_AUTO_VALUE_ON_ZERO the default (in Drupal 10?).

3. +++ b/core/modules/user/user.install
   @@ -97,3 +99,36 @@ function user_install() {
   +
   +  // The new PostgreSQL sequence for the uid field needs to start with the last
   +  // used user ID + 1 and the sequence must be owned by uid field.
   +  if ($connection->driver() == 'pgsql') {
   +    $maximum_uid = $connection->query('SELECT MAX([uid]) FROM {users}')->fetchField();
   +    $seq = $connection->makeSequenceName('users', 'uid');
   +    $connection->query("ALTER SEQUENCE " . $seq . " RESTART WITH " . ($maximum_uid + 1) . " OWNED BY {users}.uid");
   +  }
   

   If this is due to a bug in the pgsql driver can we add an @see? Also what happens if a contrib database driver needs similar handling?

4. +++ b/core/tests/Drupal/Tests/UpdatePathTestTrait.php
   @@ -105,6 +106,15 @@ protected function runUpdates($update_url = NULL) {
   +      // static information to refresh it in the update runner.
   +      // @todo consider doing this in
   +      //   \Drupal\Core\DrupalKernel::initializeContainer() for container
   +      //   rebuilds.
   

   Let's open an issue to point the @todo at.

Thanks for the review @catch

Re #46.1 and #46.2 I think these need to be tackled together. Unless we unpick #46.1 then the only entity type that'll support insert a 0 ID is user. Amusingly the code introduced by #2279395: The PostgreSQL backend does not handle NULL values for serial fields gracefully has resulted in all dbs working like MYSQL without the NO_AUTO_VALUE_ON_ZERO. I'll open a follow-up that details that we need to address the SQL mode and the entity code together. Created #3222123: Allow entity types other than user to use an ID of 0 when the ID is a serial field

#46.3 Yep I'll link this to #3028706: New serial columns do not own sequences

#46.4 Created #3222121: Database connections are persistent across a container rebuild

Updating issue summary for the scope of the current change which is much smaller than the original.

AS this really is now only about the UID field changing the issue title around.

Updated the CR.

All the changes look good to me.
All the points of @catch have been addressed.
The IS and the CR are up to date.
There is testing for the hook_update_N().
There is already a lot of testing for user Id is zero.
The requested follow-ups are created.
For me it is RTBC.

I tested this patch with the drupal 7 migrate fixture.

I began with an existing instance of the drupal 7 fixture in a mysql database. I then manually added the anonymous user. With this patch applied I exported that database (to d7_dump.php) and then imported it into a Drupal 7 site. The anonymous user was present on the D7 site. That is wonderful. It solves the problem #2678468: Add uid 0 when importing the test fixtures, so it looks like that can be closed as a duplicate.

I then did a diff of core/modules/migrate_drupal/tests/fixtures/drupal7.php with the just exported d7_dump.php. There were completely different. I then exported the database from the d7 site and compared that file to the one I used to import. Again they were completely different. Looking at the files, it appears that the tables are exported in a random order. I think this regression was introduced in another issue but for migrate the exported dump files need to be in a consistent order. On the other hand, those fixtures are only getting 'tweaks' now. So probably nothing to do on this point.

+++ b/core/modules/user/user.install
@@ -97,3 +99,38 @@ function user_install() {
+function user_update_9001(&$sandbox) {

Should this be 9301?

@longwave yep I guess so.

@quietone did you mean to un-rtbc this in #52. Cool to know this helps to fix #2678468: Add uid 0 when importing the test fixtures. I'm not sure that this has much to do with the order of the exported data in the file. Is it the user data that is all oddly ordered? Also I'm not sure that the dumper guarantees the order of the data exported.

@alexpott, I did not mean to change the status of this issue. Sorry about that.

@alexpott, I did not mean to change the status of this issue. Sorry about that.

@quietone: Thank you for your reply!

The renaming of the update function looks good to me.
Back to RTBC.

Very happy to see this RTBC.

There are workarounds in here, but most of them are working around workarounds we added because of the overall workaround of not using a serial column for the uid column, so the patch allows us to eventually remove all that technical debt in parallel, trying to do it in one go would get us nowhere I think.

#3028706: New serial columns do not own sequences is the main issue that's not a result of the logic we're changing, but we have postgres-specific upgrade code and testing to ensure we cover that.

Committed 139f911 and pushed to 9.3.x. Thanks!

Published CR