Primary key drop when data present

[withinfocus]

Steps to reproduce

Our Bitwarden product has a beta offering where self-hosters can utilize Entity Framework Core and bring their own database. MySQL is one such offering. We build our migrations for that use case with your library.

We generated a migration for MySQL that involves changing the primary key. Our community has started to report (e.g. https://github.com/bitwarden/server/issues/3637 or https://github.com/bitwarden/server/issues/3635) that MariaDB or MySQL instances with data in this table cannot execute the migration.

We're not sure if we can just reorder the migration steps or what. Issues here such as #678 are quite similar and this seems resolved and we're not seeing what in our migration could be unique.

The issue

Command:

CALL POMELO_BEFORE_DROP_PRIMARY_KEY(NULL, 'Grant');
ALTER TABLE `Grant` DROP PRIMARY KEY;

is enqueued and we receive:

Exception message: Unhandled exception. MySqlConnector.MySqlException (0x80004005): Can't DROP INDEX `PRIMARY`; check that it exists
Stack trace: MySqlConnector.Core.ResultSet.ReadResultSetHeaderAsync(IOBehavior ioBehavior) in /_/src/MySqlConnector/Core/ResultSet.cs:line 43
   at MySqlConnector.MySqlDataReader.ActivateResultSet(CancellationToken cancellationToken) in /_/src/MySqlConnector/MySqlDataReader.cs:line 130
   at MySqlConnector.MySqlDataReader.CreateAsync(CommandListPosition commandListPosition, ICommandPayloadCreator payloadCreator, IDictionary`2 cachedProcedures, IMySqlCommand command, CommandBehavior behavior, Activity activity, IOBehavior ioBehavior, CancellationToken cancellationToken) in /_/src/MySqlConnector/MySqlDataReader.cs:line 468
   at MySqlConnector.Core.CommandExecutor.ExecuteReaderAsync(IReadOnlyList`1 commands, ICommandPayloadCreator payloadCreator, CommandBehavior behavior, Activity activity, IOBehavior ioBehavior, CancellationToken cancellationToken) in /_/src/MySqlConnector/Core/CommandExecutor.cs:line 56
   at MySqlConnector.MySqlCommand.ExecuteNonQueryAsync(IOBehavior ioBehavior, CancellationToken cancellationToken) in /_/src/MySqlConnector/MySqlCommand.cs:line 296
   at MySqlConnector.MySqlCommand.ExecuteNonQuery() in /_/src/MySqlConnector/MySqlCommand.cs:line 107
   at Microsoft.EntityFrameworkCore.Storage.RelationalCommand.ExecuteNonQuery(RelationalCommandParameterObject parameterObject)
   at Microsoft.EntityFrameworkCore.Migrations.MigrationCommand.ExecuteNonQuery(IRelationalConnection connection, IReadOnlyDictionary`2 parameterValues)
   at Microsoft.EntityFrameworkCore.Migrations.Internal.MigrationCommandExecutor.ExecuteNonQuery(IEnumerable`1 migrationCommands, IRelationalConnection connection)
   at Microsoft.EntityFrameworkCore.Migrations.Internal.Migrator.Migrate(String targetMigration)
   at Microsoft.EntityFrameworkCore.RelationalDatabaseFacadeExtensions.Migrate(DatabaseFacade databaseFacade)

Further technical details

MySQL version: MariaDB v10 Operating system: any Pomelo.EntityFrameworkCore.MySql version: v7.0.0 (latest) Microsoft.AspNetCore.App version: v6

[DuckThom]

FYI, I am running on MySQL 8.0.35 and also have this issue so this does not appear to be specific to MariaDB

[withinfocus]

We didn't see a migration problem on MySQL 8 installs -- with data in this table -- for the script we executed at least. @DuckThom do you have an example migration?

[DuckThom]

Sure thing @withinfocus , I've also added some information here: https://github.com/bitwarden/server/issues/3651#issuecomment-1885849819

This is my docker-compose.yaml file:

version: "3.8"

services:
  bitwarden:
    image: bitwarden/self-host:beta
    restart: always
    depends_on:
      - db
    env_file:
      - settings.env
    ports:
      - "8000:8080"
    volumes:
      - bitwarden:/etc/bitwarden

  db:
    image: mysql:8.0
    restart: always
    environment:
      MYSQL_DATABASE: "bitwarden_vault"
      MYSQL_RANDOM_ROOT_PASSWORD: "true"
    volumes:
      - data:/var/lib/mysql

volumes:
  bitwarden:
  data:

I did some digging in the bitwarden/self-hosted:2024.1.0-beta container and found that the first error in the admin.log file was actually this one:

ALTER TABLE `Grant` ADD CONSTRAINT `PK_Grant` PRIMARY KEY (`Id`);

Error: Duplicate entry '0' for key 'Grant.PRIMARY'

Because that one is failing, the admin process crashes and restarts. However, upon restart, it also tries to re-run the queries that did execute correctly, which is actually what's causing the Can't DROP INDEX `PRIMARY` issue since that query was already executed.

The Grant table looked like this when it's in this broken state:

CREATE TABLE `Grant` (
  `Key` varchar(200) CHARACTER SET utf8mb4 COLLATE utf8mb4_0900_ai_ci NOT NULL,
  `Type` varchar(50) CHARACTER SET utf8mb4 COLLATE utf8mb4_0900_ai_ci NOT NULL,
  `SubjectId` varchar(200) CHARACTER SET utf8mb4 COLLATE utf8mb4_0900_ai_ci DEFAULT NULL,
  `SessionId` varchar(100) CHARACTER SET utf8mb4 COLLATE utf8mb4_0900_ai_ci DEFAULT NULL,
  `ClientId` varchar(200) CHARACTER SET utf8mb4 COLLATE utf8mb4_0900_ai_ci NOT NULL,
  `Description` varchar(200) CHARACTER SET utf8mb4 COLLATE utf8mb4_0900_ai_ci DEFAULT NULL,
  `CreationDate` datetime(6) NOT NULL,
  `ExpirationDate` datetime(6) DEFAULT NULL,
  `ConsumedDate` datetime(6) DEFAULT NULL,
  `Data` longtext CHARACTER SET utf8mb4 COLLATE utf8mb4_0900_ai_ci NOT NULL,
  `Id` int NOT NULL DEFAULT '0'
) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_0900_ai_ci

What I noticed here is that there's no auto-increment on the newly added Id column yet. This, as far as I could find, is the main reason that the ALTER TABLE `Grant` ADD CONSTRAINT `PK_Grant` PRIMARY KEY (`Id`); migration failed as it gave all the existing rows a value of "0".

In the end I made a dump of the original table data (manually removed the Id column and then used myslqdump), truncated Grant, restarted Bitwarden, restored the data from the dump and then the table was migrated correctly. ( Detailed steps described here: https://github.com/bitwarden/server/issues/3635#issuecomment-1885923519 )

This is now the final state of the table:

CREATE TABLE `Grant` (
  `Key` varchar(200) CHARACTER SET utf8mb4 COLLATE utf8mb4_0900_ai_ci NOT NULL,
  `Type` varchar(50) CHARACTER SET utf8mb4 COLLATE utf8mb4_0900_ai_ci NOT NULL,
  `SubjectId` varchar(200) CHARACTER SET utf8mb4 COLLATE utf8mb4_0900_ai_ci DEFAULT NULL,
  `SessionId` varchar(100) CHARACTER SET utf8mb4 COLLATE utf8mb4_0900_ai_ci DEFAULT NULL,
  `ClientId` varchar(200) CHARACTER SET utf8mb4 COLLATE utf8mb4_0900_ai_ci NOT NULL,
  `Description` varchar(200) CHARACTER SET utf8mb4 COLLATE utf8mb4_0900_ai_ci DEFAULT NULL,
  `CreationDate` datetime(6) NOT NULL,
  `ExpirationDate` datetime(6) DEFAULT NULL,
  `ConsumedDate` datetime(6) DEFAULT NULL,
  `Data` longtext CHARACTER SET utf8mb4 COLLATE utf8mb4_0900_ai_ci NOT NULL,
  `Id` int NOT NULL AUTO_INCREMENT,
  PRIMARY KEY (`Id`),
  UNIQUE KEY `IX_Grant_Key` (`Key`)
) ENGINE=InnoDB AUTO_INCREMENT=17 DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_0900_ai_ci

As you can see, at some point it did now add AUTO_INCREMENT and removed "DEFAULT '0'" from the Id column. Though I'm not sure where.

One thing I am still a bit confused about is the admin page still showing 2023.12.0 on the bitwarden/self-hosted:2024.1.0-beta image, but that's probably not related here.

[withinfocus]

We extracted some of the migration process and see:

ALTER TABLE `Grant` ADD `Id` int NOT NULL DEFAULT 0;
ALTER TABLE `Grant` ADD CONSTRAINT `PK_Grant` PRIMARY KEY (`Id`);
CALL POMELO_AFTER_ADD_PRIMARY_KEY(NULL, 'Grant', 'Id');

Problem is, this table has data in it and the primary key application forces a generation. We only had one row of data in our tests so it succeeded but when a second is encountered it'll blow up. The POMELO_AFTER_ADD_PRIMARY_KEY proc makes the modification to AUTO_INCREMENT but it's too late.

We're experimenting with a PR with small metadata changes, but we'll probably have to execute custom SQL.

I'll edit the issue to make clear this is not Maria-specific.

[withinfocus]

Relates to #1460 and #1582, possibly being considered a duplicate.

[lauxjpn]

The initial commit [PM-5294][deps]: Update Duende.IdentityServer to v6.3.7 (#3499) should have worked, but there is a bug in Pomelo that leads to insufficient SQL being generated.

---

The following code fragement is generated as part of the migration Up() method:

migrationBuilder.AddColumn<int>(
    name: "Id",
    table: "Grant",
    type: "int",
    nullable: false,
    defaultValue: 0)
    .Annotation("MySql:ValueGenerationStrategy", MySqlValueGenerationStrategy.IdentityColumn);

migrationBuilder.AddPrimaryKey(
    name: "PK_Grant",
    table: "Grant",
    column: "Id");

It was correctly detected, that the new Id column should become an auto_increment primary key (MySqlValueGenerationStrategy.IdentityColumn), but the auto_increment flag is not emitted to the SQL statement:

ALTER TABLE `Grant` ADD `Id` int NOT NULL DEFAULT 0; /* <-- missing AUTO_INCREMENT */
ALTER TABLE `Grant` ADD CONSTRAINT `PK_Grant` PRIMARY KEY (`Id`);

The result is that the Id values for all existing rows are being initialized to 0. This then lets the creation of the primary key constraint fail (because there can't be multiple rows with the same PK value).

---

There are 2 bugs in Pomelo that are related to this issue:

1. We don't generate an auto_increment fragment, if a default value is specified (which EF Core does in your migration's Up method with defaultValue: 0, because the property is non-nullable).

After we fix this, the statements should look similar to the following:

ALTER TABLE `Grant` ADD `Id` int NOT NULL DEFAULT 0 AUTO_INCREMENT;
ALTER TABLE `Grant` ADD CONSTRAINT `PK_Grant` PRIMARY KEY (`Id`);

However, this will then result in a MySQL error. Which brings us to the next issue.

2. EF Core first generates the column statement (e.g. ALTER TABLE ... ADD ... AUTO_INCREMENT) and then the statement to make the column the primary key. However, MySQL only allows a single AUTO_INCREMENT column and requires that it is a key. Since it is not a key (yet) at the time the column is created, but is only made one with the next statement, the statement to add the column fails.

After we fix this, the statements should look similar to the following:

ALTER TABLE `Grant` ADD `Id` int NOT NULL DEFAULT 0 AUTO_INCREMENT, ADD CONSTRAINT `PK_Grant` PRIMARY KEY (`Id`);

It is a single statement that contains two clauses.

---

An unrelated note:

In [PM-5519] [PM-5526] [PM-5624] [PM-5600] Tweak EF settings for MySQL grant auto-increment (#3662), it looks like you want to use .UseIdentityColumn() for Pomelo, to explicitly declare that the Id property should be auto incremented.

However, the .UseIdentityColumn() call in that code executes NpgsqlPropertyBuilderExtensions.UseIdentityColumn() (Npgsql's extension method).

The Pomelo equivalent is UseMySqlIdentityColumn()/MySqlPropertyBuilderExtensions.UseMySqlIdentityColumn().

But you don't need to call it at all, because it was already successfuly determined, that Id should be a primary key with an AUTO_INCREMENT flag (as can be seen by the MySqlValueGenerationStrategy.IdentityColumn annotation in the original migration's Up() method).