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

OnConflict implementation for SQLite provider uses wrong feature of SQLite #796

Open
gbtb opened this issue Jul 9, 2023 · 3 comments
Open

Comments

@gbtb
Copy link
Contributor

gbtb commented Jul 9, 2023

Describe the bug
I think that implementation of OnConflict uses wrong feature of SQLite.
If you check the SQLite docs mentioned in https://fsprojects.github.io/SQLProvider/core/crud.html#OnConflict you'll see that "ON CONFLICT clause applies to UNIQUE, NOT NULL, CHECK, and PRIMARY KEY constraints".
SQLite has another feature, specifically for UPSERTs - https://sqlite.org/lang_upsert.html . Docs for that feature mention that "UPSERT in SQLite follows the syntax established by PostgreSQL, with generalizations."

I don't know why this feature of SQLite was chosen back in the day. Even though ON CONFLICT was available in SQLite for decades and UPSERT was only added at 2018, when this feature was added to SQLProvider UPSERT had already been available.
Nevertheless, I think it's quite confusing. Behavior of this feature for SQLite is different from other supported providers. I suppose we have two options:

  1. Fix SQLProvider implementation to use proper UPSERTS for SQLite, probably adding a configuration toggle somewhere to enable backward compatibility.
  2. Add a warning to the docs stating that usage of OnConflict feature for SQLite has a broader scope than one could expect.

In my case, I set OnConflict.Ignore to one of my tables, expecting it to only deal with primary key violations.
In the meantime, I had an error in my app, one of the non-null column hasn't been filled, but SQLite hasn't complained about it and just silently refused to insert a row into a table. At the end, by debugging raw sql I was able to figure things out.

@gbtb gbtb changed the title OnConflict implementation for SQLite backend uses wrong feature of SQLite OnConflict implementation for SQLite provider uses wrong feature of SQLite Jul 9, 2023
@Thorium
Copy link
Member

Thorium commented Jul 10, 2023

I'd expect @piaste focus was PostgreSQL, and SQLite was just implemented to nicely being able to test this feature.

@gbtb
Copy link
Contributor Author

gbtb commented Jul 10, 2023

I'd expect @piaste focus was PostgreSQL, and SQLite was just implemented to nicely being able to test this feature.

I understand 🙂 What do you think about suggested options? I might look into this as well, after fixing SubmitUpdates ordering issue.

@Thorium
Copy link
Member

Thorium commented Jul 11, 2023

With old .NET Framework and 32 bit IDEs, SQLite used to be the easiest database, one file, no configuration and always working. But then came .NET Core and 64 bit vs 32 bit, and all kind of driver issues.

As I come from finance background, my default go to solution has always been transactions. I've never needed OnConflict.

I know people have different use cases and some think they'll be able to gain bit more performance boost and scalability by not using transaction, with then tolerating some level incorrect program state, or handling with issues manually. Which is fine by me too.

So sure, if you feel it helps your usability, if you make PR, I'll probably accept it.

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