-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Add a "sqlite-unbundled" feature that dynamically links to system libsqlite3.so library #3507
Conversation
…sqlite3.so library
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please add a sentence documenting the feature in the readme and "why it exists"?
Otherwise other devs will have a pretty difficult time finding this.
Sure, I've updated the README.md file. It's a bit long, but should be clear about what and why. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Documentation nits, otherwise it seems okay.
I think it would also be helpful to document the difference between the sqlite
and sqlite-unbundled
features in sqlx-sqlite/lib.rs
. And it may be prudent to remind users of our SemVer guarantees regarding the SQLite linkage.
@@ -23,6 +23,9 @@ uuid = ["dep:uuid", "sqlx-core/uuid"] | |||
|
|||
regexp = ["dep:regex"] | |||
|
|||
bundled = ["libsqlite3-sys/bundled"] | |||
unbundled = ["libsqlite3-sys/buildtime_bindgen"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, we specifically didn't want this because of the hit to build times it would cause.
I suppose if people are willing to accept that to link their own SQLite, then it's fine, but then this needs to be clearly documented.
Unfortunately, it looks to be required because the current pre-generated bindings are for version 3.14.0 but we need sqlite3_prepare_v3()
which was introduced in 3.20.0, which is 7 years old at this point: https://www.sqlite.org/changes.html#version_3_20_0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(A large part of the reason for going with bundled
in the first place was just a general disdain amongst us for how slowly things move with system packages, especially in a lot of Linux distributions.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose we really only use sqlite3_prepare_v3
to pass SQLITE_PREPARE_PERSISTENT
as an optimization, but it does seem somewhat important: https://www.sqlite.org/c3ref/c_prepare_normalize.html#sqlitepreparepersistent
The SQLITE_PREPARE_PERSISTENT flag is a hint to the query planner that the prepared statement will be retained for a long time and probably reused many times. Without this flag, sqlite3_prepare_v3() and sqlite3_prepare16_v3() assume that the prepared statement will be used just once or at most a few times and then destroyed using sqlite3_finalize() relatively soon. The current implementation acts on this hint by avoiding the use of lookaside memory so as not to deplete the limited store of lookaside memory. Future versions of SQLite may act on this hint differently.
It looks like sqlite3_prepare_v2()
behaves the same as _v3()
with prepFlags = 0
: https://github.com/sqlite/sqlite/blob/a95620c1414b06ac73b656b518ae364ff41f1e81/src/prepare.c#L954
The way the lookaside allocator is described to work, it seems quite detrimental to hold onto prepared statement handles using it for very long, as running out of lookaside memory means spilling over to regular malloc()
calls, defeating the whole purpose of the lookaside allocator to begin with: https://www.sqlite.org/malloc.html#lookaside
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I checked repology and it seems that only ancient distros are shipping a too old sqlite, e.g. CentOS 7. (Some are shipping sqlite 2.x in addition and the naming is quite mixed.)
Co-authored-by: Austin Bonander <austin.bonander@gmail.com>
Co-authored-by: Austin Bonander <austin.bonander@gmail.com>
and also mention possible build time increasement.
Besides getting CI passing, a couple final things:
|
by duplicating the relevant test section
…sqlite3.so library (launchbadge#3507) * Add a "sqlite-unbundled" feature that dynamically links to system libsqlite3.so library * update README abouot the newly-added `sqlite-unbundled` feature * Update README.md to make it clear with bulleted list Co-authored-by: Austin Bonander <austin.bonander@gmail.com> * more cfg feature updates Co-authored-by: Austin Bonander <austin.bonander@gmail.com> * update documentation in sqlx-sqlx/src/lib.rs too and also mention possible build time increasement. * cargo fmt * Add "sqlite-unbundled" feature to sqlx-cli * Add sqlite-unbundled to gituhb actions tests * cfg(feature = "sqlite") => cfg(any(feature = "sqlite", feature = "sqlite-unbundled")) * fix * CI: make sqlite-unbundled tests workaround required-features by duplicating the relevant test section * use an internal "_sqlite" feature to do the conditional compilation --------- Co-authored-by: Austin Bonander <austin.bonander@gmail.com>
…sqlite3.so library (launchbadge#3507) * Add a "sqlite-unbundled" feature that dynamically links to system libsqlite3.so library * update README abouot the newly-added `sqlite-unbundled` feature * Update README.md to make it clear with bulleted list Co-authored-by: Austin Bonander <austin.bonander@gmail.com> * more cfg feature updates Co-authored-by: Austin Bonander <austin.bonander@gmail.com> * update documentation in sqlx-sqlx/src/lib.rs too and also mention possible build time increasement. * cargo fmt * Add "sqlite-unbundled" feature to sqlx-cli * Add sqlite-unbundled to gituhb actions tests * cfg(feature = "sqlite") => cfg(any(feature = "sqlite", feature = "sqlite-unbundled")) * fix * CI: make sqlite-unbundled tests workaround required-features by duplicating the relevant test section * use an internal "_sqlite" feature to do the conditional compilation --------- Co-authored-by: Austin Bonander <austin.bonander@gmail.com>
fixes #191 as suggested in #191 (comment).