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

Always set SQLITE_OPEN_URI on in-memory sqlite #3289

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

LecrisUT
Copy link
Contributor

Does your PR solve an issue?

I have encountered this issue when trying to package a downstream project where the in-memory sqlite database was simply gone at subsequent accesses. The issue is that URI format is implicitly used, but the flag is never set

if database == ":memory:" {
options.in_memory = true;
options.shared_cache = true;
let seqno = IN_MEMORY_DB_SEQ.fetch_add(1, Ordering::Relaxed);
options.filename = Cow::Owned(PathBuf::from(format!("file:sqlx-in-memory-{seqno}")));
} else {

This issue was not detected because only the bundled libsqlite3-sys version is tested which has the USE_URI compile flag enabled. On distros, however this is not yet enabled, and it is just started to be investigated 1

Related to:

Footnotes

  1. https://bugzilla.redhat.com/show_bug.cgi?id=2291157

@LecrisUT
Copy link
Contributor Author

@abonander can you review this PR?

@LecrisUT
Copy link
Contributor Author

@abonander Gentle ping

@@ -87,6 +87,7 @@ impl EstablishParams {

if options.in_memory {
flags |= SQLITE_OPEN_MEMORY;
flags |= libsqlite3_sys::SQLITE_OPEN_URI;
Copy link
Collaborator

@abonander abonander Aug 27, 2024

Choose a reason for hiding this comment

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

Why did you choose to set the flag here and not where we set filename with a URI? It's a bit non-local.

Moreover, there's really no reason that we use the URI format there anyway. You could just drop the file: prefix.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why did you choose to set the flag here and not where we set filename with a URI? It's a bit non-local.

I am not sure how you would add it next to the fileneame, probably you are considering adding it as a new variable to options? Setting the flags here seems to make more sense because this is where the sqlite flags are being constructed.

Moreover, there's really no reason that we use the URI format there anyway. You could just drop the file: prefix.

I am not sure that would work. Presumably without the SQLITE_OPEN_URI it would have treated the whole file:sqlx-in-memory-... as the filename, but that doesn't seem to trigger the shared cache mode which is where I've encountered my issues.


Alternatively, SQLITE_OPEN_URI could be enabled for all flags. The Fedora packagers for sqlite also considered that it should be safe to compile with this feature always enabled, but because there hasn't been any other report that this is needed other than sqlx, and because it is possible to set it at runtime, they've decided to keep it open until there is a bigger need for that change. It would make sense to have sqlx setup all of the flags that it expects to have by default in the sqlite in order to make sure that any system sqlite is configured as it would have expected.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I've been thinking we probably need to completely re-do our handling of connection URLs for SQLite, in part due to #3099

I'm not against enabling SQLITE_OPEN_URI unconditionally, though for cases where the user manually sets the filename, it might break the parsing: https://docs.rs/sqlx/latest/sqlx/sqlite/struct.SqliteConnectOptions.html#method.filename

See also #3401 but I kinda got stuck on the tempfile thing and then had to focus on #3440

Copy link
Collaborator

Choose a reason for hiding this comment

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

Cause at the end of the day, I think :memory: with shared-cache mode is probably not the best choice anyway. shared-cache mode is soft-deprecated by the SQLite devs: https://www.sqlite.org/sharedcache.html#dontuse

And people keep using :memory: with SqlitePool but don't change the default pool settings or enable shared-cache mode, then wonder why their data disappears.

I think if people just want a temporary database, they should use WAL mode in a temporary directory, so the design of the PR is kinda focused on that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The place where I've encountered was within test environments, and probably over there it should be supported to have as simple configuration as possible. I think setting SQLITE_OPEN_URI on would be orthogonal to any further refactoring, so how about in the current form where the flag is set always on?

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