elixir-sqlite / ecto_sqlite3

An Ecto SQLite3 adapter.
https://hexdocs.pm/ecto_sqlite3
MIT License
300 stars 45 forks source link

AUTOINCREMENT support #94

Open newmanjeff opened 1 year ago

newmanjeff commented 1 year ago

Is there any way to specify that a table's primary key should be AUTOINCREMENT?

warmwaffles commented 1 year ago

@newmanjeff I think we can, there is overhead associated with it https://www.sqlite.org/autoinc.html

warmwaffles commented 1 year ago

I think the area to do that would be here

https://github.com/elixir-sqlite/ecto_sqlite3/blob/d4689abb69b7010ffac096dabae825578cb8d4ef/lib/ecto/adapters/sqlite3/connection.ex#L1731-L1750

Just need to handle the case where two primary keys are provided and not to use autoincrement in that case.

newmanjeff commented 1 year ago

Thanks -- I'll take a stab at it in a bit.

warmwaffles commented 1 year ago

Perhaps it could also be an opt in using the opts?

newmanjeff commented 1 year ago

I added an MR that includes AUTOINCREMENT for :bigserial. :serial was already set as AUTOINCREMENT, so it seems inconsistent that :bigserial would not be marked that same way. The option to disable auto-increment exists by using a different column type (e.g. :id). I think this follows the principal of least-surprise on "what does the data type :bigserial mean for ecto_sqlite3."

My only hesitation here is backwards-compatibility. The resulting schema will be different if the database was created on an older version vs this version. :bigserial column types will be common since it's the default primary key for ecto. I could add an Application env option to control it if you think that would be better.

warmwaffles commented 1 year ago

A lot of the issues I am grappling with is trying to maintain some backwards compatibility with old the old ecto2 sqlite adapter and decisions made there and this one.

I think, let's just rip the band-aid off and force the issue. If we backwards compatibility is absolutely necessary, we can add in an option to turn off the new behavior.

newmanjeff commented 1 year ago

Sounds good to me.

joeljuca commented 1 year ago

I just stumbled upon this issue after searching about how ecto_sqlite3 deals with AUTOINCREMENT. Considering that it brings some known overhead to SQLite tables, and that INTEGER PRIMARY KEY columns are aliased to the SQLite-internal rowid column, why not make primary key definitions default to INTEGER PRIMARY KEY?

tahirmurata commented 1 month ago

I just stumbled upon this issue after searching about how ecto_sqlite3 deals with AUTOINCREMENT. Considering that it brings some known overhead to SQLite tables, and that INTEGER PRIMARY KEY columns are aliased to the SQLite-internal rowid column, why not make primary key definitions default to INTEGER PRIMARY KEY?

Any updates on this?

warmwaffles commented 1 month ago

No updates on this. I haven't explored this more. If you want to, you can take a stab at it 😄