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.

Sign up for GitHub

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

Merged
merged 12 commits into from
Oct 2, 2024
Merged

Add a "sqlite-unbundled" feature that dynamically links to system libsqlite3.so library #3507

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
0a25254
Add a "sqlite-unbundled" feature that dynamically links to system lib…
lilydjwg Sep 15, 2024
60b633b
update README abouot the newly-added `sqlite-unbundled` feature
lilydjwg Sep 15, 2024
6b4876d
Update README.md to make it clear with bulleted list
lilydjwg Sep 16, 2024
6cee67e
more cfg feature updates
lilydjwg Sep 16, 2024
937096a
update documentation in sqlx-sqlx/src/lib.rs too
lilydjwg Sep 16, 2024
9136a4f
cargo fmt
lilydjwg Sep 16, 2024
877af00
Add "sqlite-unbundled" feature to sqlx-cli
lilydjwg Sep 16, 2024
7431d17
Add sqlite-unbundled to gituhb actions tests
lilydjwg Sep 16, 2024
ff30bc6
cfg(feature = "sqlite") => cfg(any(feature = "sqlite", feature = "sql…
lilydjwg Sep 16, 2024
ea41477
fix
lilydjwg Sep 16, 2024
71f3e7d
CI: make sqlite-unbundled tests workaround required-features
lilydjwg Sep 16, 2024
b45a967
use an internal "_sqlite" feature to do the conditional compilation
lilydjwg Sep 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,8 @@ _rt-tokio = []
any = ["sqlx-core/any", "sqlx-mysql?/any", "sqlx-postgres?/any", "sqlx-sqlite?/any"]
postgres = ["sqlx-postgres", "sqlx-macros?/postgres"]
mysql = ["sqlx-mysql", "sqlx-macros?/mysql"]
sqlite = ["sqlx-sqlite", "sqlx-macros?/sqlite"]
sqlite = ["sqlx-sqlite/bundled", "sqlx-macros?/sqlite"]
sqlite-unbundled = ["sqlx-sqlite/unbundled", "sqlx-macros?/sqlite-unbundled"]

# types
json = ["sqlx-macros?/json", "sqlx-mysql?/json", "sqlx-postgres?/json", "sqlx-sqlite?/json"]
Expand Down
3 changes: 2 additions & 1 deletion sqlx-macros-core/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,8 @@ migrate = ["sqlx-core/migrate"]
# database
mysql = ["sqlx-mysql"]
postgres = ["sqlx-postgres"]
sqlite = ["sqlx-sqlite"]
sqlite = ["sqlx-sqlite/bundled"]
sqlite-unbundled = ["sqlx-sqlite/unbundled"]

# type integrations
json = ["sqlx-core/json", "sqlx-mysql?/json", "sqlx-postgres?/json", "sqlx-sqlite?/json"]
Expand Down
1 change: 1 addition & 0 deletions sqlx-macros/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ migrate = ["sqlx-macros-core/migrate"]
mysql = ["sqlx-macros-core/mysql"]
postgres = ["sqlx-macros-core/postgres"]
sqlite = ["sqlx-macros-core/sqlite"]
sqlite-unbundled = ["sqlx-macros-core/sqlite-unbundled"]

# type
bigdecimal = ["sqlx-macros-core/bigdecimal"]
Expand Down
4 changes: 3 additions & 1 deletion sqlx-sqlite/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,9 @@ uuid = ["dep:uuid", "sqlx-core/uuid"]

regexp = ["dep:regex"]

bundled = ["libsqlite3-sys/bundled"]
unbundled = ["libsqlite3-sys/buildtime_bindgen"]
Copy link
Collaborator

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

Copy link
Collaborator

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.)

Copy link
Collaborator

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

Copy link
Contributor Author

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.)


[dependencies]
futures-core = { version = "0.3.19", default-features = false }
futures-channel = { version = "0.3.19", default-features = false, features = ["sink", "alloc", "std"] }
Expand Down Expand Up @@ -55,7 +58,6 @@ default-features = false
features = [
"pkg-config",
"vcpkg",
"bundled",
"unlock_notify"
]

Expand Down
2 changes: 1 addition & 1 deletion src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ pub use sqlx_mysql::{self as mysql, MySql, MySqlConnection, MySqlExecutor, MySql
#[doc(inline)]
pub use sqlx_postgres::{self as postgres, PgConnection, PgExecutor, PgPool, Postgres};

#[cfg(feature = "sqlite")]
#[cfg(any(feature = "sqlite", feature = "sqlite-unbundled"))]
#[cfg_attr(docsrs, doc(cfg(feature = "sqlite")))]
#[doc(inline)]
pub use sqlx_sqlite::{self as sqlite, Sqlite, SqliteConnection, SqliteExecutor, SqlitePool};
Expand Down