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

[5.3] Add primary key to migrations table #15770

Merged
merged 1 commit into from
Oct 6, 2016
Merged

[5.3] Add primary key to migrations table #15770

merged 1 commit into from
Oct 6, 2016

Conversation

ctoma
Copy link
Contributor

@ctoma ctoma commented Oct 5, 2016

I recently upgraded my XtraDB cluster and encountered errors when running any migrations. By default now XtraDB cluster ships with strict mode on which requires an explicit primary key for all tables.

Background

XtraDB is a performance-enhanced fork of the InnoDB storage engine. It is the primary engine for both MariaDB and Percona. It is a drop-in replacement.

The Problem

The latest XtraDB cluster (and I assume Galera cluster) hard fail because they now ship with strict mode on. Every table needs an explicit primary key defined. This means out of the box laravel doesn't work with XtraDB cluster/Galera cluster. At the moment this only affects the latest cluster versions but there is talk in the community of rolling this out further to non-clustered versions.

All InnoDB tables have primary keys

All InnoDB tables actually have primary keys. If the user does not explicitly define a primary key then a hidden 6-byte PK is created. Since most Laravel users are probably on MySQL and the default storage engine is InnoDB, this means we already have a primary key anyway, it's just very inefficient (as tests prove). Further, I don't believe that adding a primary key would affect any of the other db engines supported by Laravel (Postgres, SQLite, SQL Server).

How to fix and why

We should explicitly state the primary key. This ensures Laravel will work out of the box on clustered setups and improves efficiency for all MySQL users as we are no longer using the inefficient hidden PK. In other words, on the majority of setups the PK is there anyway, let's just explicitly define it. This will also help for future proofing if later on explicit primary keys are required in the non-clustered versions. Think back to what a pain NO_ZERO_DATE was.

@ctoma ctoma changed the title Added primary key to migrations table [5.3] Added primary key to migrations table Oct 5, 2016
@ctoma ctoma changed the title [5.3] Added primary key to migrations table [5.3] Add primary key to migrations table Oct 5, 2016
@taylorotwell
Copy link
Member

This has been proposed before and causes problems.

@taylorotwell taylorotwell reopened this Oct 5, 2016
@taylorotwell
Copy link
Member

I will have to look through the past PRs to find the issue.

@alexwhitman
Copy link
Contributor

alexwhitman commented Oct 5, 2016

The issue is probably when this approach is used in conjunction with the utf8mb4 charset as it will cause a key length error because migration is defined as a VARCHAR(255). This can be worked around by setting the primary key length to 191 or a better solution is to enable innodb_large_prefix but this requires setting some other options also and may not be do-able on shared hosting.

A much simpler solution would be to add an auto incrementing integer column to the migration table and make that the primary key. See #7249

@taylorotwell
Copy link
Member

Probably simplest solution that won't cause any problems for anyone is the auto-incrementing integer solution. Could you update this PR to reflect that?

@ctoma
Copy link
Contributor Author

ctoma commented Oct 6, 2016

@taylorotwell Done

@taylorotwell taylorotwell merged commit 3ba17de into laravel:5.3 Oct 6, 2016
@browner12
Copy link
Contributor

this solution is going to help with new applications. will we run into any issues with existing applications that do not have this key?

is there some type of automated solution we could come up with? or is the solution to tell people to create a migration, and update the migration table?

@MircoBabin
Copy link

this solution is going to help with new applications. will we run into any issues with existing applications that do not have this key?

is there some type of automated solution we could come up with? or is the solution to tell people to create a migration, and update the migration table?

For anybody stumbling upon a missing id column, see #39473 .

Here is a migration that at least works with MySql.

2021_11_15_070000_update_migrations_id.php

<?php

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

class UpdateMigrationsId extends Migration
{
    /**
     * Run the migrations.
     *
     * @return void
     */
    public function up()
    {
        if (Schema::hasColumn('migrations', 'id')) {
            return;
        }
        
        //create `migrations`.`id` as first field,  before `migration`
        Schema::table('migrations', function ($table) {
            $table->increments('id')->first();
        });
    }

    /**
     * Reverse the migrations.
     *
     * @return void
     */
    public function down()
    {
        if (Schema::hasColumn('migrations', 'id')) {
            Schema::table('migrations', function ($table) {
                $table->dropColumn('id');
            });
        }
    }
}

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

Successfully merging this pull request may close these issues.

None yet

6 participants