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

[8.x] "null" constraint prevents aliasing SQLite ROWID #35792

Merged
merged 2 commits into from Jan 6, 2021
Merged

[8.x] "null" constraint prevents aliasing SQLite ROWID #35792

merged 2 commits into from Jan 6, 2021

Conversation

Max13
Copy link
Contributor

@Max13 Max13 commented Jan 6, 2021

On SQLite, columns are nullable by default. Adding null forcibly, prevents users aliasing ROWID and will create another (autoincrementing in some cases) column and it's overhead in calculation (see: SQLite ROWID and the INTEGER PRIMARY KEY)

To summerize SQLite documentation: by default, every table has a rowid column, which already is an INTEGER (64bits), is autoincrementing (not always by 1) and is unique across the table. When a column type is exactly INTEGER and a PRIMARY KEY, then this column is an alias of rowid. When the column type is INTEGER NULL, INT, BIGINT or anything else, it's a totally different column.

This change should be backward compatible as columns are nullable by default anyway.

@GrahamCampbell GrahamCampbell changed the title "null" constraint prevents aliasing SQLite ROWID [8.x] "null" constraint prevents aliasing SQLite ROWID Jan 6, 2021
@taylorotwell taylorotwell merged commit 6e56312 into laravel:8.x Jan 6, 2021
@Max13 Max13 deleted the patch-1 branch January 6, 2021 14:03
@Max13
Copy link
Contributor Author

Max13 commented Jan 6, 2021

@taylorotwell Regarding this "issue", would it be a good idea to alias $…Increments() column to ROWID anyway?

Currently, with this PR merged, to use the internal rowid as primary key aliased, the user must manually use $table->integer('id')->primary() in Schema, as $…Increments() won't. If we use rowid as primary key by default, then to implement another primary key, with AUTOINCREMENT or with another affinity then INTEGER, then users will have to use a specific syntax. As it would be a breaking change, I consider this for the next major.

@driesvints
Copy link
Member

@Max13 it's unlikely Taylor will see that message. Best that if you want to propose something that you send in a PR so we can look at the code.

@Max13
Copy link
Contributor Author

Max13 commented Jan 6, 2021

@driesvints Thanks for the input. Then I prefer asking on discord because I don't consider it a breaking change, other members might. Then depending on this status, it should be sent to 8.x or master. And ultimately, it can also be rejected so by asking I may have other inputs and ideas.

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

3 participants