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

sqld: disable checkpoint on primary conn create #1898

Merged
merged 1 commit into from
Jan 6, 2025

Conversation

LucioFranco
Copy link
Contributor

This commit changes our primary connection initialization code in two ways to achieve the ability to disable checkpointing the wal.

  1. We ignore the initial checkpoint that we call directly into sqlite3
    before we restore from bottomless. There is a fixme above that
    explains why we need this but to me right now its not totally clear
    why without digging deeper into the internals of bottomless. We
    should do this but for the moment this unblocks us and from the fixme
    comment it does not sound unsafe rather doing extra work potentially.

  2. When bottomless needs to get the local change counter is creates a
    sqlite connection. When this connection drops it seems like it
    checkpoints the wal. I took a brief look at the sqlite3_close code
    and did not find anything obvious, I have been told in the past that
    sqlite3 likes to checkpoint at weird points so this could be one of
    those. For the moment, the temporary fix like above is to
    std::mem::forget the connection so that Drop never gets called
    and thus sqlite3_close never gets called.

With both of these changes we now don't checkpoint the wal unless we hit the max size or interval (which for testing I have set very high).

@LucioFranco LucioFranco force-pushed the lucio/disable-checkpointing-bottomless branch 2 times, most recently from c8673f4 to a807731 Compare December 31, 2024 21:00
@LucioFranco LucioFranco marked this pull request as ready for review December 31, 2024 21:01
// TODO: we shouldn't leak the connection here but for some reason when this connection get
// dropped it seems to checkpoint the database
if std::env::var("LIBSQL_DISABLE_INIT_CHECKPOINTING").is_ok() {
std::mem::forget(conn);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this safe? @penberg @sivukhin May I borrow your brains here? Isn't leaking sqlite connection leading to some disaster over time?

bottomless/src/replicator.rs Outdated Show resolved Hide resolved
@LucioFranco LucioFranco force-pushed the lucio/disable-checkpointing-bottomless branch from a807731 to 0bc55b5 Compare January 6, 2025 15:21
Copy link
Contributor

@haaawk haaawk left a comment

Choose a reason for hiding this comment

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

So now we have both LIBSQL_BOTTOMLESS_DISABLE_INIT_CHECKPOINTING and LIBSQL_DISABLE_INIT_CHECKPOINTING?

@LucioFranco LucioFranco force-pushed the lucio/disable-checkpointing-bottomless branch from 0bc55b5 to f79525d Compare January 6, 2025 16:59
This commit changes our primary connection initialization code in two
ways to achieve the ability to disable checkpointing the wal.

1) We ignore the initial checkpoint that we call directly into sqlite3
   before we restore from bottomless. There is a fixme above that
   explains why we need this but to me right now its not totally clear
   why without digging deeper into the internals of bottomless. We
   should do this but for the moment this unblocks us and from the fixme
   comment it does not sound unsafe rather doing extra work potentially.

2) When bottomless needs to get the local change counter is creates a
   sqlite connection. When this connection drops it seems like it
   checkpoints the wal. I took a brief look at the `sqlite3_close` code
   and did not find anything obvious, I have been told in the past that
   sqlite3 likes to checkpoint at weird points so this could be one of
   those. For the moment, the temporary fix like above is to
   `std::mem::forget` the connection so that `Drop` never gets called
   and thus `sqlite3_close` never gets called.

With both of these changes we now don't checkpoint the wal unless we hit
the max size or interval (which for testing I have set very high). This
changes are not enabled by default but must be enabled by setting the
following env var:

```
LIBSQL_DISABLE_INIT_CHECKPOINTING=1
LIBSQL_BOTTOMLESS_DISABLE_INIT_CHECKPOINTING=1
```
@LucioFranco LucioFranco force-pushed the lucio/disable-checkpointing-bottomless branch from f79525d to d95fe5d Compare January 6, 2025 17:00
@LucioFranco LucioFranco added this pull request to the merge queue Jan 6, 2025
@LucioFranco
Copy link
Contributor Author

Merging and releasing this since its behind a feature flag. Platform testing will follow.

Merged via the queue into main with commit 3497d8a Jan 6, 2025
19 checks passed
@LucioFranco LucioFranco deleted the lucio/disable-checkpointing-bottomless branch January 6, 2025 17:30
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