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

Exception on digital oceans managed database #8

Closed
nticaric opened this issue Apr 14, 2023 · 11 comments
Closed

Exception on digital oceans managed database #8

nticaric opened this issue Apr 14, 2023 · 11 comments

Comments

@nticaric
Copy link

When running this migration with a digital ocean managed database, I get this error:

SQLSTATE[HY000]: 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. (SQL: create table `languages` (`id` char(2) not null, `name` varchar(255) not null, `native_name` varchar(255) not null, `created_at` timestamp null, `updated_at` timestamp null) default character set utf8mb4 collate 'utf8mb4_unicode_ci')
@fulopattila122
Copy link
Contributor

Thanks for the report. I'll sort this out tomorrow! What version of mysql/Postgres is that?

@nticaric
Copy link
Author

MySQL 8.0.27. They have sql_require_primary_key enabled. Although I could do set sql_require_primary_key = off it's a good thing to have a primary key, when creating the table

So I guess this would work:

public function up()
{
    if (!Schema::hasTable('languages')) {
        Schema::create('languages', function (Blueprint $table) {
            $table->char('id', 2)->primary();
            $table->string('name');
            $table->string('native_name');

            $table->timestamps();
        });
    }
}

Instead of defining the primary key at the end

@fulopattila122
Copy link
Contributor

fulopattila122 commented Apr 14, 2023

I remember having issues with your recommended solution, that's why I went with this one. Which is not good either. I'm out already, but I'll fix it tomorrow, I hope it doesn't break things too much on your end.

@fulopattila122
Copy link
Contributor

Did the countries table get appropriately created?
The reason why I'm asking is because it has in fact the very same structure: https://github.com/artkonekt/address/blob/2.8.0/src/resources/database/migrations/2016_11_24_104017_create_countries_table.php#L18

@fulopattila122
Copy link
Contributor

I've added the change you suggested, and the tests are passing. But it won't solve the problem, because when a pkey field is a string, then Laravel compiles it into a two-step statement. Here's Taylor's comment on it:

image

@fulopattila122
Copy link
Contributor

As far as I see (from discussions around the topic), a possible solution would be to disable the primary key constraint on the database only for the session that's running the migrations:

DB::statement('SET SESSION sql_require_primary_key=0');

Sources: 1, 2 and 3

Do you think this would this work in your case?

@nticaric
Copy link
Author

What about to keep it plain simple and introduce an id key, and rename this column to code. Would this introduce some other inconveniences?

For me personally this is not a big problem, since I can change the sql_require_primary_key manually, but it breaks automatic deployments

public function up()
{
    if (!Schema::hasTable('languages')) {
        Schema::create('languages', function (Blueprint $table) {
            $table->increments('id');
            $table->char('code', 2)->unique();
            $table->string('name');
            $table->string('native_name');

            $table->timestamps();
        });
    }
}

@fulopattila122
Copy link
Contributor

fulopattila122 commented May 1, 2023

It would break the library and especially its consumers. Please bear in mind that this is a library that is being used by many other libraries and applications. We cannot do arbitrary breaking changes.

@nticaric
Copy link
Author

nticaric commented May 2, 2023

In this case, I think it's best to keep it as it is now. Thank you for your support!

@nticaric nticaric closed this as completed May 2, 2023
@fulopattila122
Copy link
Contributor

Thanks for your understanding. I keep thinking about an alternative solution though.

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

No branches or pull requests

2 participants