MySQL Cluster support - Need primary keys on all tables

[jgupta]

Bug Report

Current Behavior
Installation on Percona MySQL Cluster fails with error - Something went wrong: SQLSTATE[HY000]: General error: 1105 Percona-XtraDB-Cluster prohibits use of DML command on a table (xxx.fl_migrations) without an explicit primary key with pxc_strict_mode = ENFORCING or MASTER (SQL: insert into xx_migrations (migration, extension) values (2015_02_24_000000_create_access_tokens_table, ?))

Steps to Reproduce

1. Install on Percona MySQL cluster (or any mysql cluster) with strict mode set to enforcing.

Expected Behaviour
Installation should work.

Possible Solution
All tables should have primary keys

Additional Context
https://www.percona.com/doc/percona-xtradb-cluster/LATEST/features/pxc-strict-mode.html#tables-without-primary-keys

[luceos]

We had the same issue at @blomstra using Percona and are migrating that manually for now. But this is a great addition to flarum/issue-archive#121.

[askvortsov1]

Agree we should implement this. Other than migrations, which tables are affected?

[jgupta]

@askvortsov1 All other tables except migrations seem to have primary keys.

Can you guide me to the correct place in code so that I could maybe patch this and finish installation at my end? I tried to look for the code that is creating migrations table.

[askvortsov1]

Interesting, looks like it's hardcoded outside of migrations in https://github.com/flarum/core/blob/94d69fe15fbcc459c84fcad5a0d5b274d9d7fe48/src/Database/DatabaseMigrationRepository.php#L91-L104.

We should adjust that to include a proper primary key on new installs, but how do we want to handle old installs? Could do a migration and include that by default on new installs...

[jgupta]

Thank you @askvortsov1
I changed the code to following.

        $schema->create($this->table, function ($table) {
            $table->increments('id');
            $table->string('migration');
            $table->string('extension')->nullable();
        });

There was another error related to posts table being MyISAM. Change it to InnoDB from the beginning (I think it is later altered to InnoDB later in setup).

Now that initial setup is done, we can test it further.

[askvortsov1]

@luceos @tankerkiller125 let's just add an incrementing ID in a migration + in the DatabaseMigrationRepository script? Or should we use the migration column as an explicit primary key?

[tankerkiller125]

I'm for making the migration name the primary key. Notably since it's basically impossible to have duplicate names there anyway.

[askvortsov1]

I'm for making the migration name the primary key. Notably since it's basically impossible to have duplicate names there anyway.

Tested locally, and immediately ran into a duplicate migration name 😞