Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

sql_require_primary_key Causes Tables With String Primary Key To Fail #33238

Closed
CalumTowers opened this issue Jun 17, 2020 · 65 comments
Closed

Comments

@CalumTowers
Copy link

  • Laravel Version: 7
  • PHP Version: 7.3.14
  • Database Driver & Version: MySQL 8

Description:

When MySQL has sql_require_primary_key enabled, migrations that call ->primary() on a column during table creation fail because it appears as though two queries are ran. One to create the table, and then a second to alter the table and add the primary key.

For context, DigitalOcean recently enforced primary keys on all newly created tables in their managed databases.

Steps To Reproduce:

  1. Ensure sql_require_primary_key is enabled on the MySQL server
  2. Create a migration that creates a new table with a string as the primary key (we used $table->string('string')->primary();) and does not have a default $table->id(); column
  3. Run the migration

The below error is generated when attempting to run the first query to create table.

General error: 3750 Unable to create or change a table without a primary key, when the system variable 'sql_require_primary_key' is set. Add a primary key to the table or unset this variable to avoid this message. Note that tables without a primary key can cause performance problems in row-based replication, so please consult your DBA before changing this setting.
@driesvints
Copy link
Member

Hmm this is a tricky one. Apparently the SQL generated to create the table doesn't immediately adds the primary key and thus indeed causes this error. I'm not sure how (or if) this can be solved.

@CalumTowers
Copy link
Author

Hmm this is a tricky one. Apparently the SQL generated to create the table doesn't immediately adds the primary key and thus indeed causes this error. I'm not sure how (or if) this can be solved.

Thanks for taking a look!

I know it doesn't help in finding a resolution, but it appears that this also affects Passport install. See here: https://github.com/laravel/passport/blob/9.x/database/migrations/2016_06_01_000001_create_oauth_auth_codes_table.php

$table->id(); appears to function without any issues, I think because of this logic.

@erwinweber96
Copy link

In my case I use Digital Ocean as well, but this problem happened for pivot tables only and I had to edit the migrations by adding $table->id(); and $table->unique(['table1_id', table2_id']);.

@driesvints
Copy link
Member

@erwinweber96 the problem is that that won't work for string based primary identifiers. Thinking password resets table, database notifications, passport, etc.

@erwinweber96
Copy link

I agree that this needs to be solved, however I added that comment in case someone stumbles upon the issue (like I did) regarding sql_require_primary_key and laravel. There's very few info online on this matter atm.

@Ekion-1
Copy link

Ekion-1 commented Jun 20, 2020

I'm experiencing the same issue. Unsure if it was a recent change to DigitalOcean's config but Laravel applications utilising Passport are effectively incompatible with DigitalOcean Managed Databases at the moment.

@kaphert
Copy link

kaphert commented Jun 21, 2020

update below solution won't work if you run tests in sqlite for example. See #33238 (comment) for a simpler workaround.

Same issue here, it affects the sessions, password_resets and the two Telescope tables telescope_entries_tags and telescope_monitoring as far as I know at this point.

For anyone that needs a fix, this is my workaround for now:

  • Run the default Laravel migrations (add a primary key where necessary) on a machine where the flag is turned off (Homestead for example)
  • Open your SQL editor and copy the CREATE TABLE ... statement. (TablePlus: open table, at the bottom: [Structure] and then the button [Info] on the right).
  • Open your migration file, comment your original Laravel Schema (keep it as a reference).
  • Run the CREATE TABLE ... statement with: \DB::statement($query);

sessions

\Illuminate\Support\Facades\DB::statement('CREATE TABLE `sessions` (
  `id` varchar(255) COLLATE utf8mb4_unicode_ci NOT NULL,
  `user_id` bigint(20) unsigned DEFAULT NULL,
  `ip_address` varchar(45) COLLATE utf8mb4_unicode_ci DEFAULT NULL,
  `user_agent` text COLLATE utf8mb4_unicode_ci,
  `payload` text COLLATE utf8mb4_unicode_ci NOT NULL,
  `last_activity` int(11) NOT NULL,
  PRIMARY KEY (`id`)
) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_unicode_ci;');

password_resets
You could probably also choose email as a primary key but in my case I have some extra (tenant related) columns in this table.

\Illuminate\Support\Facades\DB::statement('CREATE TABLE `password_resets` (
  `email` varchar(255) COLLATE utf8mb4_unicode_ci NOT NULL,
  `token` varchar(255) COLLATE utf8mb4_unicode_ci NOT NULL,
  `created_at` timestamp NULL DEFAULT NULL,
  PRIMARY KEY (`token`)
) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_unicode_ci;')

Laravel Telescope

See https://laravel.com/docs/7.x/telescope#migration-customization on how to use your own migrations instead of the default ones

telescope_entries_tags

\Illuminate\Support\Facades\DB::statement('CREATE TABLE `telescope_entries_tags` (
  `entry_uuid` char(36) COLLATE utf8mb4_unicode_ci NOT NULL,
  `tag` varchar(255) COLLATE utf8mb4_unicode_ci NOT NULL,
  PRIMARY KEY (`entry_uuid`,`tag`),
  KEY `telescope_entries_tags_tag_index` (`tag`),
  CONSTRAINT `telescope_entries_tags_entry_uuid_foreign` FOREIGN KEY (`entry_uuid`) REFERENCES `telescope_entries` (`uuid`) ON DELETE CASCADE
) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_unicode_ci;');

telescope_monitoring

DB::statement('CREATE TABLE `telescope_monitoring` (
  `tag` varchar(255) COLLATE utf8mb4_unicode_ci NOT NULL,
  PRIMARY KEY (`tag`)
) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_unicode_ci;');

@mgmgpyaesonewin
Copy link

Current workaround is setting
SET sql_require_primary_key=0, and I set the sql variable.

And I change to

alter table `TABLE_NAME` add column `id` int(10) unsigned primary KEY AUTO_INCREMENT;

@Ekion-1
Copy link

Ekion-1 commented Jun 22, 2020

Current workaround is setting
SET sql_require_primary_key=0, and I set the sql variable.

And I change to

alter table `TABLE_NAME` add column `id` int(10) unsigned primary KEY AUTO_INCREMENT;

Unable to do this on managed database instances (DigitalOcean) as far as I'm aware?

@CalumTowers
Copy link
Author

CalumTowers commented Jun 22, 2020

Yep, this is following a recent change to their config
If it wasn't for DO mandating it, I'd have disabled the setting and moved on. Because they are mandating it, I thought our team wouldn't be the only ones having issues.

From the tagging of this issue (by Dries as a bug and help wanted), I think there is an acceptance that this needs to be resolved in the framework in some way. Although I acknowledge that is is a fairly sizeable job to sort!

@taylorotwell
Copy link
Member

taylorotwell commented Jun 22, 2020

If someone wants to submit a PR to somehow make this one query behind the scenes be our guest 😄

Currently, this is not a "bug" in Laravel. It is a bug in DigitalOcean's configuration they are forcing upon you.

@Ekion-1
Copy link

Ekion-1 commented Jun 23, 2020

I've had the following response from DigitalOcean regarding this issue:

I understand you are getting error when trying to migrate database schemas. You can temporarily override the setting using SET SESSION sql_require_primary_key = 1;

However, You will need to add a primary key after you import or you will receive notifications at a point that adding primary key is required.

This setting is only for the current session though, it's not a permanent override.

Therefore I managed to get this to work by calling \Illuminate\Support\Facades\DB::statement('SET SESSION sql_require_primary_key=0'); above my Schema::create command in the migrations causing issues.

@Mic-B
Copy link

Mic-B commented Jun 29, 2020

Therefore I managed to get this to work by calling \Illuminate\Support\Facades\DB::statement('SET SESSION sql_require_primary_key=0'); above my Schema::create command in the migrations causing issues.

Can confirm this works for me as well. Thanks Alex!

@imrealashu
Copy link

Any latest update or any other solution as of now? The major tables that are affected are the pivot tables only.

@burakaktna
Copy link

burakaktna commented Aug 22, 2020

I've had the following response from DigitalOcean regarding this issue:

I understand you are getting error when trying to migrate database schemas. You can temporarily override the setting using SET SESSION sql_require_primary_key = 1;
However, You will need to add a primary key after you import or you will receive notifications at a point that adding primary key is required.
This setting is only for the current session though, it's not a permanent override.

Therefore I managed to get this to work by calling \Illuminate\Support\Facades\DB::statement('SET SESSION sql_require_primary_key=0'); above my Schema::create command in the migrations causing issues.

Thank you works for me as well. You can add this into

src/vendor/laravel/framework/src/Illuminate/Database/MigrationServiceProvider.php

so it works every time you use the migrate command.

@nladev
Copy link

nladev commented Sep 9, 2020

I've had the following response from DigitalOcean regarding this issue:

I understand you are getting error when trying to migrate database schemas. You can temporarily override the setting using SET SESSION sql_require_primary_key = 1;
However, You will need to add a primary key after you import or you will receive notifications at a point that adding primary key is required.
This setting is only for the current session though, it's not a permanent override.

Therefore I managed to get this to work by calling \Illuminate\Support\Facades\DB::statement('SET SESSION sql_require_primary_key=0'); above my Schema::create command in the migrations causing issues.

love you so much

@adddz
Copy link

adddz commented Sep 25, 2020

I've had the following response from DigitalOcean regarding this issue:

I understand you are getting error when trying to migrate database schemas. You can temporarily override the setting using SET SESSION sql_require_primary_key = 1;
However, You will need to add a primary key after you import or you will receive notifications at a point that adding primary key is required.
This setting is only for the current session though, it's not a permanent override.

Therefore I managed to get this to work by calling \Illuminate\Support\Facades\DB::statement('SET SESSION sql_require_primary_key=0'); above my Schema::create command in the migrations causing issues.

I'm trying to import an existing Laravel database (MySQL 5.x) into DigitalOcean Managed Database (MySQL 8) and I'm getting that error. How would I set that variable? At the top of the dump file? Thank you!

@krns
Copy link

krns commented Sep 26, 2020

I'm trying to import an existing Laravel database (MySQL 5.x) into DigitalOcean Managed Database (MySQL 8) and I'm getting that error. How would I set that variable? At the top of the dump file? Thank you!

Are you using a Tool like TablePlus? Then you can login to the database, execute SET SESSION sql_require_primary_key=0 manually and import your dump. This works because it's the same session :-)

@adddz
Copy link

adddz commented Sep 26, 2020 via email

@krns
Copy link

krns commented Sep 26, 2020

It works, thanks!

And for some Laravel tables like password_resets? Should we add a primary key to a column of choice?

We didn't make other changes because the error only occurs while creating the table.. after that everything should work as expected. But keep in mind that you cannot create more tables without a primary key (or you have to apply the workaround again).

@provydon
Copy link

I've had the following response from DigitalOcean regarding this issue:

I understand you are getting error when trying to migrate database schemas. You can temporarily override the setting using SET SESSION sql_require_primary_key = 1;
However, You will need to add a primary key after you import or you will receive notifications at a point that adding primary key is required.
This setting is only for the current session though, it's not a permanent override.

Therefore I managed to get this to work by calling \Illuminate\Support\Facades\DB::statement('SET SESSION sql_require_primary_key=0'); above my Schema::create command in the migrations causing issues.

Tnx alot!!

@kvasconcelos
Copy link

Thank you works for me as well. You can add this into

src/vendor/laravel/framework/src/Illuminate/Database/MigrationServiceProvider.php

so it works every time you use the migrate command.

Where in that file should i use it ?

@imomer
Copy link

imomer commented Nov 24, 2020

Thank you works for me as well. You can add this into

src/vendor/laravel/framework/src/Illuminate/Database/MigrationServiceProvider.php

so it works every time you use the migrate command.

Where in that file should i use it ?

Yeah where should we add in that file?

@RyonMB
Copy link

RyonMB commented Aug 11, 2021

Hi guys!

I opened a ticket with DO and they changed the parameter for me. So no need for any "hacky" stuff ;)

Hi there,
Thanks for getting in touch with us!
I understand you will like to disable the primary requirement on your managed database. The primary requirement for your managed database ******* has been disabled.

@JVillator0
Copy link

JVillator0 commented Aug 11, 2021

One simple solution is add this code at AppServiceProvider@register and listen the Migrations events

// imports 
use Illuminate\Support\Facades\Event;
use Illuminate\Database\Events\MigrationsEnded;
use Illuminate\Database\Events\MigrationsStarted;

// code in `register` method 
Event::listen(MigrationsStarted::class, function (){
    if (env('ALLOW_DISABLED_PK')) {
        DB::statement('SET SESSION sql_require_primary_key=0');
    }
});

Event::listen(MigrationsEnded::class, function (){
    if (env('ALLOW_DISABLED_PK')) {
        DB::statement('SET SESSION sql_require_primary_key=1');
    }
});

Or you can add a config variable, for example allow_disabled_pk in config file databases.php and call config('databases.allow_disabled_pk') instead of env('ALLOW_DISABLED_PK')

Controlled if you want run SET SESSION sql_require_primary_key from the .env, which normally only on servers you want to do.

@RyonMB
Copy link

RyonMB commented Aug 12, 2021

One simple solution is add this code at AppServiceProvider@register and listen the Migrations events

// imports 
use Illuminate\Support\Facades\Event;
use Illuminate\Database\Events\MigrationsEnded;
use Illuminate\Database\Events\MigrationsStarted;

// code in `register` method 
Event::listen(MigrationsStarted::class, function (){
    if (env('ALLOW_DISABLED_PK')) {
        DB::statement('SET SESSION sql_require_primary_key=0');
    }
});

Event::listen(MigrationsEnded::class, function (){
    if (env('ALLOW_DISABLED_PK')) {
        DB::statement('SET SESSION sql_require_primary_key=1');
    }
});

Or you can add a config variable, for example allow_disabled_pk in config file databases.php and call config('databases.allow_disabled_pk') instead of env('ALLOW_DISABLED_PK')

Controlled if you want run SET SESSION sql_require_primary_key from the .env, which normally only on servers you want to do.

Only don't use env() in your other files besides your config files. Or ask DO to change the sql requirement for you.

@JVillator0
Copy link

One simple solution is add this code at AppServiceProvider@register and listen the Migrations events

// imports 
use Illuminate\Support\Facades\Event;
use Illuminate\Database\Events\MigrationsEnded;
use Illuminate\Database\Events\MigrationsStarted;

// code in `register` method 
Event::listen(MigrationsStarted::class, function (){
    if (env('ALLOW_DISABLED_PK')) {
        DB::statement('SET SESSION sql_require_primary_key=0');
    }
});

Event::listen(MigrationsEnded::class, function (){
    if (env('ALLOW_DISABLED_PK')) {
        DB::statement('SET SESSION sql_require_primary_key=1');
    }
});

Or you can add a config variable, for example allow_disabled_pk in config file databases.php and call config('databases.allow_disabled_pk') instead of env('ALLOW_DISABLED_PK')
Controlled if you want run SET SESSION sql_require_primary_key from the .env, which normally only on servers you want to do.

Only don't use env() in your other files besides your config files. Or ask DO to change the sql requirement for you.

Same, i use the config('database.allow_disabled_pk')

@chopachom
Copy link

I don't think this issue should be closed because current workarounds are not solving the root cause.
The problem is that row based replication requires you to define primary keys, so this is not only an issue for DigitalOcean managed databases. Current workarounds with session variables are error prone - you can create tables without primary keys (passwords_reset for example doesn't have a primary key).

@anstapol
Copy link

anstapol commented Feb 3, 2022

In my case I use Digital Ocean as well, but this problem happened for pivot tables only and I had to edit the migrations by adding $table->id(); and $table->unique(['table1_id', table2_id']);.

so you still added the incremental id even if you have the combined primary? and it worked?

@Rickkie
Copy link

Rickkie commented Feb 16, 2022

I've had the following response from DigitalOcean regarding this issue:

I understand you are getting error when trying to migrate database schemas. You can temporarily override the setting using SET SESSION sql_require_primary_key = 1;
However, You will need to add a primary key after you import or you will receive notifications at a point that adding primary key is required.
This setting is only for the current session though, it's not a permanent override.

Therefore I managed to get this to work by calling \Illuminate\Support\Facades\DB::statement('SET SESSION sql_require_primary_key=0'); above my Schema::create command in the migrations causing issues.

Tnx alot!!

How did you get this to work?

@a-bashtannik
Copy link
Contributor

I would like to say we decided to remove Telescope from the project because of missing primary keys and infinite Digital Ocean warnings.

Our systems have indicated that your MySQL cluster has tables without primary keys. We have identified that MySQL tables without primary keys can lead to service replication issues that jeopardize performance and availability. If primary keys are not present for database tables exceeding 5,000 rows, data-loss can occur.

DB::statement('SET SESSION sql_require_primary_key=0'); workaround works, but risks remain.

Sad, but nothing to do if maintainers refuse of primary keys.

@madalinignisca
Copy link

Never ever disable primary keys on HA MySQL and MariaDB clusters as this might lead to corruption of replication between server instances. Probably developers that made this migrations where not aware of how MySQL and MariaDB replication works, especially how Galera, and simply decided as they thought not to add primary keys.

@joe-pritchard
Copy link
Contributor

Should this issue be reopened since the fix was reverted and the issue still exists?

@madalinignisca
Copy link

Should this issue be reopened since the fix was reverted and the issue still exists?

I do so. Any table MUST have primary keys. And the framework, should test any DB transaction against most limited setup of MariaDB or MySQL, that being default Primary/Replica and Galera setups. Testing against single instance mode does not prove the framework will be trustful in HA setups.

Today is very common to deploy on HA setups, and I wouldn't find a reason not to do it so.

@joe-pritchard
Copy link
Contributor

@taylorotwell Would you consider reopening the issue? You say above that it's a "bug" with the Digital Ocean setup but in reality it's a recommended and increasingly common way to configure MySQL and simply turning off required primary keys is not a solution. I believe as things stand several first-party laravel packages such as Nova and Telescope contain migrations that simply can't be run in these setups, and it also makes it a pain to use uuid primary ids.

I did look at the revert commit that was merged earlier but couldn't find much context about why the fix was reverted so forgive me if there's a good reason to NOT fix this!

@jrean
Copy link
Contributor

jrean commented Jun 18, 2022

DO engineers team has updated API documentation to make database configuration changes via API; you will now be able to disable sql_require_primary_key using API calls.

In certain cases, you may also want to adjust the sql_require_primary_key, which determines whether primary keys are a requirement. When set to True (default), primary keys are required. When set to False, primary keys are optional. However, we do not currently support adjusting this mode in the control panel. Instead, you can set this mode by making a configuration request via our API.

Then they add:

For clusters with more than one node, we strongly recommend keeping sql_require_primary_key as True. Setting it to False may severely impact replication and cause issues.

@joe-pritchard
Copy link
Contributor

The thing is I don't want to turn off this setting just in order to use uuid primary keys. This is the recommended setting for a reason and i want to retain it. I don't believe it should be necessary to use a less reliable mysql setup because of the way these migrations run under the hood

@jrean
Copy link
Contributor

jrean commented Jun 19, 2022

@joe-pritchard Then so far the best solution came from @JVillator0

class AppServiceProvider extends ServiceProvider
{
    /**
     * Register any application services.
     *
     * @return void
     */
    public function register()
    {
        Event::listen(MigrationsStarted::class, function () {
            if (config('database.allow_disabled_pk')) {
                DB::statement('SET SESSION sql_require_primary_key=0');
            }
        });

        Event::listen(MigrationsEnded::class, function () {
            if (config('database.allow_disabled_pk')) {
                DB::statement('SET SESSION sql_require_primary_key=1');
            }
        });
    }
...

@joe-pritchard
Copy link
Contributor

Yes that's what I'm doing right now but it's not ideal as it essentially turns off the primary key requirement completely, since all migrations will run without it meaning there's no functional difference to just turning it off in the db's config. Again this is not a solution to the problem, it's a workaround, and I believe there is value in just fixing it at source

@tobyberesford
Copy link

Turning off the requirement is a bit like intentionally incurring technical debt - you know it's going to come back and bite you, just when you're in a rapid scaling situation.

@madalinignisca
Copy link

Note: don't use primary keys and in case of replication failure, deal with the problem and accept also the financial loses for the downtime caused.

@pablorsk
Copy link
Contributor

Laravel 8 + DigitalOcean database

💡 Just add the first statement to your migration:

use Illuminate\Database\Migrations\Migration;
use Illuminate\Database\Schema\Blueprint;
use Illuminate\Support\Facades\Schema;
use Illuminate\Support\Facades\DB;

class AddTableTemplates extends Migration
{
    public function up()
    {
        // fixes DigitalOcean sql_require_primary_key problem
        DB::statement('SET SESSION sql_require_primary_key=0');
        Schema::create('objects', function (Blueprint $table) {
            $table->string('id', 30)
                ->primary();
            // ...
        });
    }

    public function down()
    {
        Schema::dropIfExists('objects');
    }
}

@tombanksme
Copy link

I encountered this issue myself recently and I think I found a better work around. 
It’s pretty similar to those who suggested dumping the schema using another tool such as TablePlus but uses the new schema:dump command and works with the migration system.


On the machine you use for development create a fresh database using (this will delete all existing tables and re-create them using your migrations):

php artisan migrate:fresh

Then dump your schema:

php artisan schema:dump

Then commit the changes and push them to the server. Now when you run the migrate command, Laravel will restore the database using the dumped schema file before running the migrations.

If you make any changes to the migrations, you will have to re-dump the schema file using this method. Once you are in production though, you shouldn’t have to do that again.

Hope this helps; sorry for commenting on an old issue; thanks Taylor and the Laravel Team for your amazing work 😁

@daniel-sellarix
Copy link

If anyone else who comes across this thread has a similar issue on Azure.

sql_generate_invisible_primary_key <- Make sure this one is turned off as well :D

@Cuji12
Copy link

Cuji12 commented Oct 10, 2023

Digital Ocean has an API that you can use to configure the settings of your DB, which I would highly recommend over other solutions. You can do the following:

  1. Create an API token if you don't have one already: https://cloud.digitalocean.com/account/api/tokens
  2. Find the ID of your database cluster by querying the List all database clusters endpoint:
curl -X GET \
  -H "Content-Type: application/json" \
  -H "Authorization: Bearer $DIGITALOCEAN_TOKEN" \
  "https://api.digitalocean.com/v2/databases"
  1. Once you've found the ID of your database cluster, you can make a patch request to the update database config endpoint:
curl -X PATCH \
  -H "Content-Type: application/json" \
  -H "Authorization: Bearer $DIGITALOCEAN_TOKEN" \
  -d '{"config": {"sql_require_primary_key": true}}' \
  "https://api.digitalocean.com/v2/databases/$DATABASE_CLUSTER_ID/config"

Now your database cluster will be configured, permanently, without any code changes.

@taylorotwell
Copy link
Member

Should be fixed by: #49374

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests