Skip to content

Bottomless skip snapshot#1238

Merged
MarinPostma merged 4 commits intomainfrom
bottomless-skip-snapshot
Mar 21, 2024
Merged

Bottomless skip snapshot#1238
MarinPostma merged 4 commits intomainfrom
bottomless-skip-snapshot

Conversation

@Horusiath
Copy link
Contributor

@Horusiath Horusiath commented Mar 20, 2024

Build on top of #1237

This PR enables setting up (through export LIBSQL_BOTTOMLESS_SKIP_SNAPSHOT=true) ability to skip snapshot generation when a checkpoint is being made. It still may happen if machine ie. recovered from backup after process restart (however it's just a matter of calling snapshot_main_db_file(force)).

This feature could work together with #1229 - backup WAL without snapshots on sqld server and periodically call snapshot creation from bottomless-cli on separate process.

@Horusiath Horusiath requested a review from MarinPostma March 20, 2024 13:25
bottomless::replicator::RestoreAction::SnapshotMainDbFile => {
replicator.new_generation().await;
if let Some(_handle) = replicator.snapshot_main_db_file().await? {
if let Some(_handle) = replicator.snapshot_main_db_file(true).await? {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we don't want to force snapshot generation after db restore, this should be changed to false.

Copy link
Contributor

Choose a reason for hiding this comment

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

What is the benefit in creating a snapshot right after we restored from backup?

Copy link
Contributor Author

@Horusiath Horusiath Mar 20, 2024

Choose a reason for hiding this comment

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

I'd say it's mostly related to forking, as I haven't tested if we don't get data loss if we won't snapshot after forking.

Copy link
Contributor

Choose a reason for hiding this comment

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

We should probably snapshot explicitly after forking though, and not after other kind of restores

if self.get_dependency(&generation).await?.is_some() {
// empty generation and empty local state, but we have a dependency
// to previous generation: restore required
return Ok(None);
Copy link
Contributor Author

@Horusiath Horusiath Mar 20, 2024

Choose a reason for hiding this comment

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

This is fairly important and could be counted as a bug fix on its own. It doesn't happen atm. (probably), but in general it could potentially cause DB to restore to empty state if we didn't manage to snapshot DB and WAL log was not present in current generation.

@MarinPostma MarinPostma force-pushed the bottomless-skip-snapshot branch from f3947f3 to c9a218a Compare March 21, 2024 09:58
@MarinPostma MarinPostma enabled auto-merge March 21, 2024 09:58
@MarinPostma MarinPostma force-pushed the bottomless-skip-snapshot branch from c9a218a to dbc1dd5 Compare March 21, 2024 09:58
@MarinPostma MarinPostma added this pull request to the merge queue Mar 21, 2024
Merged via the queue into main with commit 427421b Mar 21, 2024
@MarinPostma MarinPostma deleted the bottomless-skip-snapshot branch March 21, 2024 10:29
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.

3 participants