Migration fails when sql_require_primary_key is set

[haugstrup]

Pulse Version

1.0beta

Laravel Version

10

PHP Version

8.1

Description

When sql_require_primary_key is set, the Pulse migration fails:

SQLSTATE[HY000]: General error: 3750 Unable to create or change a table without a primary key, when the system vari  
  able '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. (Connection: mysql, SQL: create table `pulse_values` (`timestamp` int unsign  
  ed not null, `type` varchar(255) not null, `key` text not null, `key_hash` char(16) character set binary as (unhex(  
  md5(`key`))), `value` text not null) default character set utf8mb4 collate 'utf8mb4_unicode_ci')  

It errors on the first table, but presumably will fail on all three tables. This setting is required on Digital Ocean Managed MySQL servers so Pulse can't be installed for any application backed by that service.

pulse_values and pulse_aggregates tables both have a unique index so both tables can probably be created by disabling sql_require_primary_key while the tables are being created.

But pulse_entries doesn't appear to have a column or index that can be used as a primary key so I don't know what to do there.

Steps To Reproduce

• Set sql_require_primary_key to true in your MySQL configuration.
• Attempt to install Pulse

[ahinkle]

FWIW, You can reach out to DO to get the sql_require_primary_key flag removed.

[haugstrup]

Not really a great trade-off since it can break replication 🤷

[GalahadXVI]

Encountered the exact same problem. It's not exactly ideal that we have to reach out to DO to solve this issue ourselves and I personally would rather not disable sql_require_primary_key just for the sake of allowing Pulse to function.

[jbrooksuk]

While I agree it's not ideal, you can also make an API request to DO to change this setting; https://stackoverflow.com/questions/62418099/unable-to-create-or-change-a-table-without-a-primary-key-laravel-digitalocean

[driesvints]

@jessarcher @timacdonald could we get auto incrementing primary keys on the tables or was there a big reason we didn't?

[driesvints]

If we can't we could maybe add a note in the docs for DO instances that should contact them to request them to remove the setting.

[GalahadXVI]

Is there any reason why we can't publish the migration files?

As placing this at the beginning of up() solves the issue:
\Illuminate\Support\Facades\DB::statement('SET SESSION sql_require_primary_key=0');

[jbrooksuk]

Migrations should be publishable soon, #81

[GalahadXVI]

That's great. Though, despite there being a temporary solution to it, I hope this doesn't detract from the issue of needing to disable sql_require_primary_key at all.

I believe the reason is that not using primary keys can significantly impact replication performance and I'm presuming that DO is enforcing it because you can't manually exclude tables from replication if using DO's read-only nodes (as far as I'm aware).

Edit:

Contacted DigitalOcean to confirm it. Heres what they said:

We currently have the variable sql_require_primary_key turned on to enable users to create a primary key on tables to avoid replication, node replication, etc. This has worked in the past and was not enforced but experience, time, and the information we have gathered from frequent issues ex. the time it takes to create
a new node for a service from a backup with large tables, replication issues, etc

Primary keys are essential for certain management operations also for services that do not have standby or read replica service; any node replacements are performed by first bringing up a standby to which all data from old master is replicated and without primary keys this process may take exceedingly long or fail, also failed nodes are replaced by restoring a backup, which requires playing back binary logs and that may not work if large tables without primary keys have had recent changes. Hence we decided to enable primary requirement, especially for large tables.

I can see this being an issue for users who use don't have a secondary DB for pulse if they have read-only nodes activated.

[zoispag]

Similar issue with Telescope: laravel/telescope#1407
The suggestion was to publish the migration and modify as seen fit. I still think that the provided migrations should always have primary keys, to allow replication.

[timacdonald]

This will be fixed in #142

If you have already run the migrations you will need to add this column manually.

[haugstrup]

Big thank you for this change -- those mimic the columns I added manually, but it's fantastic that Pulse will work out-of-the-box going forward. It's a fantastic package!