ecto_sqlite3 icon indicating copy to clipboard operation
ecto_sqlite3 copied to clipboard

AUTOINCREMENT support

Open newmanjeff opened this issue 3 years ago • 10 comments

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

newmanjeff avatar Dec 06 '22 16:12 newmanjeff

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

warmwaffles avatar Dec 06 '22 16:12 warmwaffles

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.

warmwaffles avatar Dec 06 '22 17:12 warmwaffles

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

newmanjeff avatar Dec 06 '22 18:12 newmanjeff

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

warmwaffles avatar Dec 06 '22 18:12 warmwaffles

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.

newmanjeff avatar Jan 09 '23 18:01 newmanjeff

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.

warmwaffles avatar Jan 09 '23 18:01 warmwaffles

Sounds good to me.

newmanjeff avatar Jan 09 '23 18:01 newmanjeff

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?

joeljuca avatar May 01 '23 16:05 joeljuca

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?

tahirmurata avatar Oct 26 '24 16:10 tahirmurata

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

warmwaffles avatar Oct 28 '24 13:10 warmwaffles