Add snapshot support for point-in-time database copy#4346
Conversation
65a4d56 to
27a9bcc
Compare
faa0e3e to
510cb7b
Compare
sivukhin
left a comment
There was a problem hiding this comment.
I have some suggestion and concerns regarding current tsnapshot logic.
Also, it will be great to use new snapshotting logic in the .clone CLI command
| res.release_guard(); | ||
| // Release checkpoint guard if lock is not to be kept | ||
| if !keep_lock { | ||
| res.release_guard(); |
There was a problem hiding this comment.
I don't think that checkpoint guarantee that guard always will be set.
There are cases (for example, empty WAL - so nothing to checkpoint) - where checkpoint will return without holding any lock.
So, this fix do not prevent race condition situation in all situations, IIUC.
There was a problem hiding this comment.
Yeah you are right. I didn't want to touch the checkpoint function in the first place.
| let pager = self.pager.load(); | ||
| let result = (|| -> Result<()> { | ||
| // Checkpoint and keep the lock | ||
| let _res = pager.blocking_checkpoint_keep_lock( |
There was a problem hiding this comment.
Can we instead of patching checkpoint do snapshot like this:
- Execute truncate checkpoint
- Start read transaction (either explicitly with
BEGINor maybe by just issuingread_txon WAL) - Do the copy of DB file
- End read transaction
There was a problem hiding this comment.
I like this solution. I will implement this.
Although it does leave a tiny race window between truncating file and taking read lock where a writer can write and snapshot is taken. Maybe we can live with this right now. I will leave a detailed comment just in case we see a bug later on.
There was a problem hiding this comment.
tiny race window between truncating file and taking read lock where a writer can write and snapshot is taken
Ah, yeah. The problem here actually is not in writes specifically - but in a checkpoint that can happen in the middle.
Alternative suggestion from me is to start read transaction before checkpoint but issue FULL checkpoint instead of TRUNCATE. This will make it possible for checkpoint to succeed - but also we will hold WAL and prevent any further checkpoint from happening (even if writes will happen).
Also, we should be careful with deferred nature of transaction and maybe we need to execute something after BEGIN statement in order to properly initiate read transaction.
As we are writing database - its better to be safe than sorry :) So even tiny race window is bad actually.
There was a problem hiding this comment.
@sivukhin - apologies for late reply. I was out with viral last week.
I thought about your suggestion but this also doesn't work. I have finally found another approach which works. I have detailed everything in the PR Note. Also, PR is ready for review.
|
Thanks for the feedback @sivukhin. |
525d2ec to
7e71044
Compare
This addresses feedback from tursodatabase/agentfs#119 suggesting that snapshot functionality should be implemented in turso.git using direct fily copying rather than SQL-based row by row copying. Comment: https:://github.com/tursodatabase/agentfs/pull/119#issuecomment-3681336678 - extract core logic from Pager::checkpoint function to a new Pager::checkpoing_internal function and add a flag to keep_lock during the Finalize phase - create wrapper functions Pager::checkpoint, Pager::checkpoint_with_lock and Pager::block_checkpoint_keep_lock - add Connection::snapshot API that checkpoints while keeping the lock and copies the database file The lock is held to avoid a race condition after finishing checkpointing and before copying the file when concurrent writers can write to db. Signed-off-by: Prateek Singh Rathore <prateek.singh.rathore@gmail.com>
Signed-off-by: Prateek Singh Rathore <prateek.singh.rathore@gmail.com>
The previous implementation attempted to hold checkpoint locks during file copy by adding a keep_lock param to the checkpoint function. However, checkpoint can have multiple early exit paths like empty WAL where no lock is acquired. Also, it clutters the checkpoint function. This implementation executes TRUNCATE checkpoint and then acquires read lock on WAL before copying database file. This ensures new data is written to WAL and new Checkpoint can not be taken before this read lock is released. Signed-off-by: Prateek Singh Rathore <prateek.singh.rathore@gmail.com>
Signed-off-by: Prateek Singh Rathore <prateek.singh.rathore@gmail.com>
In this window, another checkpoint can run and we may end with corrupted db file To prevent this, we earlier tried to take a read lock just after checkpointing but that had two problems: 1) It still left a tinier window for another checkpointing to start, and 2) it doesn't guarantee slot 0 on the taken read lock and hence doesn't guarantee to prevent other checkpointing. In this commit, we TRUNCATE checkpoint and try to take slot 0 read lock in loop to handle this race condition Signed-off-by: Prateek Singh Rathore <prateek.singh.rathore@gmail.com>
943bdbd to
e8d3b55
Compare
Signed-off-by: Prateek Singh Rathore <prateek.singh.rathore@gmail.com>
- fixed cli tests for the new implementation - commented ApplyWritter's unused functions, used by older snapshot implementation in cli - removed unused imports - added missing Doc strings to .timer and .clone commands Signed-off-by: Prateek Singh Rathore <prateek.singh.rathore@gmail.com>
7ecd179 to
ba1f438
Compare
Signed-off-by: Prateek Singh Rathore <prateek.singh.rathore@gmail.com>
|
Maintainers - Please review this PR. |
|
@sivukhin - please look at it when you get time. |
|
@psrvere could you rebase with main and solve the conflicts |
|
This pull request has been marked as stale due to inactivity. It will be closed in 7 days if no further activity occurs. |
|
This pull request has been closed due to inactivity. Please feel free to reopen it if you have further updates. |
This PR addresses feedback from tursodatabase/agentfs#119 suggesting that snapshot functionality should be implemented in turso.git using direct file copying rather than SQL-based row by row copying. Comment
Implementation
read_lock[0]is acquired (explanation below)Other Additions
.cloneuses this snapshot API (the older implementation was based on logical copy)The new implementation does not support snapshot with in-memory database which is a BREAKING change for this command.
sdk-kitRace Condition Handling
There is a race condition between TRUNCATE checkpoint and copying the database file in which another snapshot can run and we may end up copying a corrupted db file. To prevent this, the implementation runs a retry loop to do two things: 1) TRUNCATE Checkpoint, and 2) Take
read_lock[0]. Since checkpoint flow need this lock to process, taking this lock prevents any other checkpoint.Other Approaches I Considered
1. Acquire any read lock before Checkpointing
Although, from first read of the
wal.rscode it looks like that checkpointing requires acquiring read locks in 1..N slotsMore specifically, these read lock are attempted to acquire in
determine_max_safe_checkpoint_framefunction which doesn't really enforce acquiring them -- if a slot is busy, it simply lowers the safe checkpoint boundary.Hence holding non-zero read lock does not prevent from checkpoints to run.
2. FULL vs TRUNCATE Checkpoint
The flow is same for FULL, RESTART and TRUNCATE checkpoint in terms of acquiring locks and back-filling entire WAL, so this didn't help.
Assisted by: Cursor + Claude