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

Snapshot checkpointing #710

Merged
merged 9 commits into from
Oct 7, 2024
Merged

Conversation

marco6
Copy link
Contributor

@marco6 marco6 commented Oct 3, 2024

This PR aims at reducing the amount of memory required for a snapshot and removing entirely page allocation from it.

The main idea is not to save the WAL during a snapshot as it is not necessary to do so. Instead, we can apply pages in the WAL to the database being saved. This means that the snapshotting logic will not allocate memory for pages, but just for the buffer list and the headers.

The saving in terms of memory and write size is in the order of 0 - 4MiB (unless a very big transaction happened) as the current implementation has a threshold of 1000 pages in the WAL before trying to checkpoint.

While the gain is quite small in absolute terms, it can become important when the size of the database is small, which seems to be likely.

Copy link

codecov bot commented Oct 3, 2024

Codecov Report

Attention: Patch coverage is 86.60714% with 15 lines in your changes missing coverage. Please review.

Project coverage is 81.10%. Comparing base (d79661f) to head (7656e39).
Report is 89 commits behind head on master.

Files with missing lines Patch % Lines
src/vfs.c 75.47% 4 Missing and 9 partials ⚠️
src/dqlite.c 0.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #710      +/-   ##
==========================================
- Coverage   85.74%   81.10%   -4.65%     
==========================================
  Files         197      197              
  Lines       29040    29048       +8     
  Branches     4084     4059      -25     
==========================================
- Hits        24901    23558    -1343     
+ Misses       3915     3844      -71     
- Partials      224     1646    +1422     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@cole-miller cole-miller self-requested a review October 7, 2024 14:03
Copy link
Contributor

@cole-miller cole-miller left a comment

Choose a reason for hiding this comment

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

Thanks Marco! As discussed on MM I think this change is sound and the implementation looks good to me. I have a few style-related nits but with your permission I'll fix them by adding a commit directly to this branch instead of leaving comments.

src/vfs.c Outdated
Comment on lines 1277 to 1280
} else if (sqlite3_stricmp(left, "wal_checkpoint") == 0
|| (sqlite3_stricmp(left, "wal_autocheckpoint") == 0 && right)) {
fnctl[0] = sqlite3_mprintf("custom checkpoint not allowed");
return SQLITE_IOERR;
Copy link
Contributor

Choose a reason for hiding this comment

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

+1, thanks for thinking of this and adding it.

Signed-off-by: Cole Miller <[email protected]>
Signed-off-by: Cole Miller <[email protected]>
Signed-off-by: Cole Miller <[email protected]>
Signed-off-by: Cole Miller <[email protected]>
@cole-miller cole-miller merged commit 014aece into canonical:master Oct 7, 2024
11 of 13 checks passed
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