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

Change sqlx-sqlite to check for uniqueness error instead of checking for existence #52

Merged
merged 1 commit into from
Oct 3, 2024

Conversation

ColonelThirtyTwo
Copy link
Contributor

Currently, the sqlx-sqlite store has an issue with databases running using the WAL journal if multiple requests come in at the same time, causing a "database is locked" error. The sequence is roughly this:

  1. Two connections call SqliteStore::create, at the same time.
  2. Concurrently, they both call SqliteStore::id_exists. Both lock the database for reading and sets their "end mark" to the current database state.
  3. Connection 1 calls save_with_conn first and commits its transaction. This upgrades its lock to a write lock and advances the database state - connection 2's end mark is still before this update.
  4. Connection 2 calls save_with_conn. It attempts to upgrade its read lock to a write lock, but fails since the database has been modified since the read lock was acquired, and sqlite returns a "database is locked" error.

This patch fixes the issue by removing the if_exists check and instead attempts to insert the ID, check if the insert fails with a uniqueness error, and regenerate the ID if so. With this, all connections immediately lock the database for writing and will wait for each other to finish before inserting. It should also be more performant in the common case of the generated ID being unique.

@maxcountryman
Copy link
Owner

I think the latest commit on main should address the test failures.

…for existence

Currently, the sqlx-sqlite store has an issue with databases running
using the WAL journal if multiple requests come in at the same time,
causing a "database is locked" error. The sequence is roughly this:

1. Two connections call `SqliteStore::create`, at the same time.
2. Concurrently, they both call `SqliteStore::id_exists`. Both lock the
   database for reading and sets their "end mark" to the current
   database state.
3. Connection 1 calls `save_with_conn` first and commits its
   transaction. This upgrades its lock to a write lock and advances the
   database state - connection 2's end mark is still before this update.
4. Connection 2 calls `save_with_conn`. It attempts to upgrade its read
   lock to a write lock, but fails since the database has been modified
   since the read lock was acquired, and sqlite returns a "database is
   locked" error.

This patch fixes the issue by removing the `if_exists` check and instead
attempts to insert the ID, check if the insert fails with a uniqueness
error, and regenerate the ID if so. With this, all connections
immediately lock the database for writing and will wait for each other
to finish before inserting. It should also be more performant in the
common case of the generated ID being unique.
@maxcountryman maxcountryman merged commit b2b2596 into maxcountryman:main Oct 3, 2024
9 checks passed
@maxcountryman
Copy link
Owner

Thank you!

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.

2 participants