feat(sqlite): add WAL hook support

[FlorianUekermann]

Add wal hook suppport.

Does your PR solve an issue?

Fixes #4012

Is this a breaking change?

No. The PR only adds a public method.

[abonander]

I'm amenable to this change, but it seems like a more thought needs to be put into this API.

The docs aren't very clear on this, but my interpretation suggests that sqlite3_wal_hook() disables the default automatic checkpointing:

https://sqlite.org/c3ref/wal_hook.html

A single database handle may have at most a single write-ahead log callback registered at one time. Calling sqlite3_wal_hook() replaces any previously registered write-ahead log callback. The return value is a copy of the third parameter from the previous call, if any, or 0. Note that the sqlite3_wal_autocheckpoint() interface and the wal_autocheckpoint pragma both invoke sqlite3_wal_hook() and will overwrite any prior sqlite3_wal_hook() settings.

https://sqlite.org/c3ref/wal_autocheckpoint.html

Every new database connection defaults to having the auto-checkpoint enabled with a threshold of 1000 or SQLITE_DEFAULT_WAL_AUTOCHECKPOINT pages. The use of this interface is only necessary if the default setting is found to be suboptimal for a particular application.

We can confirm this by finding the sqlite3_wal_autocheckpoint call in the openDatabase() function, the implementation of sqlite3_open*(): https://github.com/sqlite/sqlite/blob/7fa9e45746f16af409922f9125f375b400dff840/src/main.c#L3610

So ironically, setting this hook as-implemented would cause the exact thing you're worried about--the WAL file growing without bound--because it would never checkpoint otherwise.

After setting this hook, the user must explicitly call sqlite3_wal_checkpoint_v2() at some point in order for checkpointing to actually occur.

We should design this hook in a way that the user cannot forget to call the checkpoint function. I'm thinking the return type should force them to choose whether to checkpoint or not, and then we invoke sqlite3_wal_checkpoint_v2 on their behalf.

This can be as simple as an enum:

pub enum SqliteWalHookResult {
    Continue,
    Checkpoint(SqliteCheckpoint),
}

pub enum SqliteCheckpoint {
    Passive,
    Full,
    Restart,
    Truncate,
}

Or perhaps instead of a bespoke SqliteWalHookResult enum, we could use ControlFlow<SqliteCheckpoint>. That's potentially more flexible since it can do stuff like integrate with the ? operator.

We also need to be careful exposing remove_wal_hook because it appears that just deleting the hook won't restore the default behavior. I think we should only support replacing the hook with another callback, or calling sqlite3_wal_autocheckpoint(db, N) to restore the auto-checkpointing behavior.

[FlorianUekermann]

The docs aren't very clear on this, but my interpretation suggests that sqlite3_wal_hook() disables the default automatic checkpointing

Oof. The docs don't state this at all imo. But now it makes sense why sqlite3_wal_autocheckpoint() replaces the hook.

Also thanks for reporting upstream.

The interfaces you propose sound good in principle, but the user would need to make a decision about WAL truncation, which can be non-trivial. The main issue is that all checkpoint types except for "passive" block until all readers of a state before the most recent snapshot are done. While this is going on new writers are blocked. Depending on other settings and usage patterns this can actually cause deadlocks (if writes are done while holding a read transaction).

[FlorianUekermann]

Thanks for filing the documentation bug upstream. This does indeed seem to be problematic.

I like your API proposals, but the checkpointing decision is a tricky one, because you need to do a truncate checkpoint at some point, but that's a complex decision. Mainly because (non-passive) checkpoint completion is blocked on current readers and blocks new writers, so it can fairly easily lead to deadlocks (if some context holds a read transaction while starting a write transaction). I don't know how the autocheckpointer implements the decision making exactly, but it does seem to take readers into account when deciding whether a truncate checkpoint should be done..

[abonander]

I don't know how the autocheckpointer implements the decision making exactly, but it does seem to take readers into account when deciding whether a truncate checkpoint should be done..

https://sqlite.org/wal.html#concurrency

Whenever a write operation occurs, the writer checks how much progress the checkpointer has made, and if the entire WAL has been transferred into the database and synced and if no readers are making use of the WAL, then the writer will rewind the WAL back to the beginning and start putting new transactions at the beginning of the WAL. This mechanism prevents a WAL file from growing without bound.

So it's not that the checkpointer occasionally decides to do a TRUNCATE checkpoint. Any write operation could restart the WAL file from the beginning (apparently without truncating) if no readers are accessing the WAL file. This can still happen concurrent to any read transactions as long as they're not reading data from the WAL.

However, that seems really dependent on access patterns. If you're constantly inserting into one table and then reading from it at very high contention, it seems like it could cause the WAL to grow without bound. I don't see a way around having to take an exclusive lock now and then.

I'm honestly not sure why they didn't implement the WAL file as a circular buffer. It seems like all the space preceding the checkpoint marker is just wasted until the file resets. Probably because growing a circular buffer is a lot more complicated. Having to copy frames around is a non-starter, so at a certain point you'd have to implement it more like a memory heap.

At the end of the day, though, SQLite is not designed for every use-case. If you're truly concerned about this, it could be time to consider a different DBMS.

[abonander]

The interfaces you propose sound good in principle, but the user would need to make a decision about WAL truncation, which can be non-trivial.

Another possibility I had in mind was something like this:

pub struct SqliteWalHookContext<'a> {
    handle: NonNull<sqlite3>,
    db: &'a str,
    num_pages: c_int,
}

impl SqliteWalHookContext<'a> {
    pub fn num_pages(&self) -> c_int { self.num_pages }

    pub fn database(&self) -> &str { self.db }

    // Doesn't consume `self` so the checkpoint may be called again
    pub fn checkpoint(&mut self, kind: SqliteCheckpointKind) -> Result<SqliteCheckpointResult, SqliteError> {
        // ...
    }

    pub fn ignore(&mut) -> SqliteCheckpointResult { ... }
}

// Cannot be constructed manually
pub struct SqliteCheckpointResult {
    log_size_frames: c_int,
    checkpointed_frames: c_int
}

Then require the callback to return SqliteCheckpointResult (or Result<SqliteCheckpointResult, SqliteError>).

Because SqliteCheckpointResult cannot be constructed manually, the user has to call one of the methods on the context to get an instance of it. Then we can use the type system itself to teach them that they have to checkpoint the database manually.

[abonander]

I thought about this more, and I think a more idiomatic solution would probably be to just have a type marked #[must_use]:

pub SqliteWalStatus<'a> {
    pub db: &'a str,
    pub wal_size_frames: u32,
}

#[must_use = "setting a WAL hook on a SQLite database disables automatic checkpointing; see type docs for details"]
pub struct SqliteCheckpointer<'a> {
    handle: NonNull<sqlite3>,
    _lifetime: PhantomData<&'a mut ()>,
}

impl<'a> SqliteCheckpointer<'a> {
    // Can be called multiple times
    pub fn checkpoint(&mut self, kind: SqliteCheckpointKind) -> Result<(), SqliteError>  { ... }

    pub fn do_nothing(self) { 
        drop(self) 
    }
}

and the WAL hook signature is Fn(SqliteWalStatus<'a>, SqliteCheckpointer<'a>) -> Result<(), SqliteError>.

This way, if the user sets the hook but ignores the checkpointer, the unused_must_use lint will trigger and the compiler will explain to them exactly why.