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

Fix sqlite memory connection issues #2104

Closed
wants to merge 1 commit into from

Conversation

LecrisUT
Copy link
Contributor

Basically for in-memory sqlite databases it can happen that the pool sharing is not handled properly and the database overwrites the old one. Since the in-memory databases are used only for testing it should be fine to simply ensure the connection lives forever. Thoughts?

Resolves #2073
Reference:

Checks

  • I am happy for maintainers to push small adjustments to this PR, to speed up the review cycle
  • I have checked that there are no existing pull requests for the same thing

Signed-off-by: Cristian Le <cristian.le@mpsd.mpg.de>
@ellie
Copy link
Member

ellie commented Jun 10, 2024

Hey!

Do you have an example run in our CI where this fails?

According to this comment, below the one you linked, this issue should be fixed in SQLx: launchbadge/sqlx#362 (comment)

I've just ran our unit tests 100x both on mac and linux, and on neither platform can I replicate a failure.

@LecrisUT
Copy link
Contributor Author

Not in the local CI because the system sqlite is not used, but this is reproduced with libsqlite3-sys package on Fedora. I believe that's the case because the original issue is not reproduced with the vendored packages (SriRamanujam/atuin-rpmspec#11 vs SriRamanujam/atuin-rpmspec#7).

According to this comment, below the one you linked, this issue should be fixed in SQLx

Yes this is an upstream issue. The fix is relatively simple to incorporate here though until these fixes are found over there.

Copy link
Member

@ellie ellie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple of comments, though I'm not sure this change is needed.

EG, this approach suggests using a named in memory file: launchbadge/sqlx#2510 (comment)

Which it looks like they already do: https://github.com/launchbadge/sqlx/blob/1388fc8acc07b41ab38f4dc096f03ba9c7b851f8/sqlx-sqlite/src/options/parse.rs#L18-L22

@@ -122,7 +135,7 @@ impl SqliteStore {
impl Store for SqliteStore {
async fn push_batch(
&self,
records: impl Iterator<Item = &Record<EncryptedData>> + Send + Sync,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason for the formatting change?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just my IDE being overzealous, I will fix them shortly

@@ -72,16 +85,16 @@ impl SqliteStore {
"insert or ignore into store(id, idx, host, tag, timestamp, version, data, cek)
values(?1, ?2, ?3, ?4, ?5, ?6, ?7, ?8)",
)
.bind(r.id.0.as_hyphenated().to_string())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Formatting change here too

@ellie
Copy link
Member

ellie commented Jun 10, 2024

Not in the local CI because the system sqlite is not used, but this is reproduced with libsqlite3-sys package on Fedora.

I'm afraid we do not support using the system sqlite. If you can replicate the failure with our lockfile, then I'm happy to merge the fixes. Please don't upstream any more changes unless they are fixing a failure that can be replicated with our build.

@ellie ellie closed this Jun 10, 2024
@ellie
Copy link
Member

ellie commented Jun 10, 2024

Also, I know I linked you it before, but it looks like you need to enable SQLITE_USE_URI in your version of sqlite.

Void, having the same issue: https://forum.atuin.sh/t/packaging-atuin-for-void-linux/20/6?u=ellie
Void, finding the solution: https://forum.atuin.sh/t/packaging-atuin-for-void-linux/20/9?u=ellie
Your version, not having the flag set: https://src.fedoraproject.org/rpms/sqlite/blob/rawhide/f/sqlite.spec#_145

@LecrisUT LecrisUT deleted the fix/sqlite branch July 10, 2024 16:17
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.

sqlite tests failing in Fedora packaging environment
2 participants