-
Notifications
You must be signed in to change notification settings - Fork 18
perf(database): Enable exclusive WAL mode for DB #358
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
base: unstable
Are you sure you want to change the base?
Conversation
anchor/database/src/lib.rs
Outdated
// create all of the tables | ||
conn.execute_batch(include_str!("table_schema.sql"))?; | ||
Ok(conn_pool) | ||
fn create(path: &Path) -> Result<(), DatabaseError> { |
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 explain why the code below was removed?
let _file = File::options()
.write(true)
.read(true)
.create_new(true)
.open(path)?;
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.
Sorry, I added a comment here but forgot to push it -_-
// Do not use a connection pool yet, as WAL mode is only enabled after reopening the
// connection, so we use a one-off connection here.
As for the _file
: It was only used to create the file, but it is not needed when using Connection
directly as it creates the file for us if it does not exist. Thinking about it again, I guess it also served as a final safeguard ensuring that the file did not exist before, so maybe I should add it back 😅
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.
Have you decided about this?
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.
ah sorry. thanks for the heads up. added it back
I don't know what "very slightly slower" means, but what is our case? |
As we read relevant info into memory when launching the application, "applications that do mostly reads" does not apply to us w.r.t. SQLite, so I have not measured this. If you want, I can measure and compare startup time anyway, but it should be neglegible. |
Sorry for changing this last minute: I realized that I understood section 8 wrong:
They mean the first WAL-mode database access PER CONNECTION. So the exclusive mode still needs to be set on every connection. I added that now. Furthermore, while testing this I now realized that WAL mode works immediately (without reconnecting), so we can already create the pool immediately as before, making this PR simpler. |
@@ -90,6 +90,7 @@ mod state_database_tests { | |||
|
|||
// drop and recrate database | |||
drop(fixture.db); | |||
drop(conn); |
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.
why is this necessary now?
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.
Apparently connections can outlive their pools. Not dropping the connection here explicitly keeps it alive - and as we lock the database exclusively (for real now), creating a new connection would cause a "database is locked" error.
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.
Pull Request Overview
This PR enables exclusive WAL mode for SQLite to improve performance and enforce locking so that only a single anchor instance can use the database at a time.
- Separates connection cleanup from database reinitialization in state tests
- Updates the SQL schema to set PRAGMA journal_mode=WAL
- Introduces a connection customizer (CustomizeConnectionExclusive) to enforce exclusive locking mode
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
File | Description |
---|---|
anchor/database/src/tests/state_tests.rs | Added explicit drops of the connection for cleaner test state |
anchor/database/src/table_schema.sql | Added WAL mode configuration via PRAGMA journal_mode |
anchor/database/src/lib.rs | Added a connection customizer to enable exclusive locking mode |
Comments suppressed due to low confidence (2)
anchor/database/src/tests/state_tests.rs:93
- [nitpick] Consider adding a comment explaining why 'conn' is explicitly dropped here to aid future maintainers in understanding the need for manual cleanup before reinitializing the database.
drop(conn);
anchor/database/src/tests/state_tests.rs:131
- [nitpick] Consider adding a comment here to clarify the rationale for dropping 'conn' before reestablishing a new connection, ensuring test isolation and proper cleanup.
drop(conn);
Reminder for myself: If we end up closing #362 in favour of waiting via |
WAL mode offers slightly better performance in SQLite on my machine. Additionally, by using EXCLUSIVE mode, we get a nice locking mechanism to prevent two anchor instances from using the same database.