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

WAL sync checkpointing support #1928

Merged
merged 2 commits into from
Jan 30, 2025
Merged

WAL sync checkpointing support #1928

merged 2 commits into from
Jan 30, 2025

Conversation

penberg
Copy link
Collaborator

@penberg penberg commented Jan 22, 2025

This pull request adds WAL sync checkpointing support.

@penberg penberg force-pushed the sync-checkpoint branch 5 times, most recently from f908cb3 to 6535303 Compare January 29, 2025 08:29
@penberg penberg marked this pull request as ready for review January 29, 2025 08:29
This patch adds support for checkpointing during WAL sync, which allows
us to sync multiple checkpoint generations from the server.

The protocol is as follows:

  1. A client uses the pull endpoint to fetch frames.

  2. When we reach the end of a generation, the server returns "I am a teapot"
     (yes, really) with a JSON containing the maximum generation number on the
     server.

  3. If we need to pull more generations, we first checkpoint the WAL on the
     client, and then continue pulling frames from the newer generation.
Comment on lines +579 to 582
Ok(PullResult::Frame(frame)) => {
insert_handle.insert(&frame)?;
frame_no += 1;
sync_ctx.durable_frame_num = frame_no;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

We only need to insert here, apparently. Can we instead of maintaining the invariants of WalInsertHandle with a RefCell and check at runtime, use inherited mutability at comptime do the job by creating and dropping the handle on each frame? I introduced WalInsertHandle in a previous PR to be able to do that, but I might have encoded the wrong semantics. What you think?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I am not sure I understand what you mean. If you want, please submit a PR to do what you propose and happy to merge if it's cleaner.

As Levy pointed out, we need to make sure we don't call
`wal_insert_end()` unless we're still in a WAL insertion sesion.
@penberg penberg enabled auto-merge January 30, 2025 08:49
@penberg penberg added this pull request to the merge queue Jan 30, 2025
Merged via the queue into main with commit 016bea4 Jan 30, 2025
19 checks passed
@penberg penberg deleted the sync-checkpoint branch January 30, 2025 09:15
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