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

AUTOINCREMENT support #94

Open
newmanjeff opened this issue Dec 6, 2022 · 8 comments
Open

AUTOINCREMENT support #94

newmanjeff opened this issue Dec 6, 2022 · 8 comments

Comments

@newmanjeff
Copy link
Contributor

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

@warmwaffles
Copy link
Member

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

@warmwaffles
Copy link
Member

warmwaffles commented Dec 6, 2022

I think the area to do that would be here

defp composite_pk_definition(%Table{} = table, columns) do
pks =
Enum.reduce(columns, [], fn {_, name, _, opts}, pk_acc ->
case Keyword.get(opts, :primary_key, false) do
true -> [name | pk_acc]
false -> pk_acc
end
end)
if length(pks) > 1 do
composite_pk_expr = pks |> Enum.reverse() |> Enum.map_join(", ", &quote_name/1)
{
%{table | primary_key: :composite},
", PRIMARY KEY (" <> composite_pk_expr <> ")"
}
else
{table, ""}
end
end

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

@newmanjeff
Copy link
Contributor Author

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

@warmwaffles
Copy link
Member

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

@newmanjeff
Copy link
Contributor Author

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
Copy link
Member

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
Copy link
Contributor Author

Sounds good to me.

@joeljuca
Copy link
Contributor

joeljuca commented May 1, 2023

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?

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

3 participants