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

Refactor vfs2 #631

Merged
merged 29 commits into from
Apr 19, 2024
Merged

Refactor vfs2 #631

merged 29 commits into from
Apr 19, 2024

Conversation

cole-miller
Copy link
Contributor

The headline change is that almost everything now lives in the linked-list entry, but various other changes were swept up in this as well.

Still incomplete, the state machine needs to be updated along with the implementations of the vfs2.h functions and quite a few smaller TODOs that I've tried to note in the code for my own benefit.

The big refactor was motivated by thinking in more detail about how the WAL-index is going to be managed throughout the node's whole lifetime, including the "follower" part that I'd previously neglected. I think I now have a good mental model for this, which I intend to write down as a doc comment on the state machine stuff once that is brought up to date/stabilized.

Signed-off-by: Cole Miller [email protected]

The headline change is that almost everything now lives in the entry,
but various other changes were swept up in this as well.

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]>
Signed-off-by: Cole Miller <[email protected]>
Signed-off-by: Cole Miller <[email protected]>
This happens when we start up and then transition to leader before
writing the first WAL header.

Signed-off-by: Cole Miller <[email protected]>
We can't "pass through" BASE while the write lock is held with the
current invariant, and I don't want to relax the write lock check.

Signed-off-by: Cole Miller <[email protected]>
Now we are sometimes in the ACTIVE state when the current transaction
still has zero frames.

Signed-off-by: Cole Miller <[email protected]>
Part of the ripple from fixing the BASE transitions.

Signed-off-by: Cole Miller <[email protected]>
Signed-off-by: Cole Miller <[email protected]>
This happens when we do a checkpoint. Part of the "EMPTY reform".

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 marked this pull request as ready for review April 5, 2024 10:23
Signed-off-by: Cole Miller <[email protected]>
Copy link

codecov bot commented Apr 5, 2024

Codecov Report

Attention: Patch coverage is 57.90139% with 333 lines in your changes are missing coverage. Please review.

Project coverage is 80.56%. Comparing base (ae7a727) to head (4d74efa).
Report is 12 commits behind head on dqlite-next.

Files Patch % Lines
src/vfs2.c 56.14% 271 Missing and 61 partials ⚠️
src/utils.h 50.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@               Coverage Diff               @@
##           dqlite-next     #631      +/-   ##
===============================================
- Coverage        81.15%   80.56%   -0.60%     
===============================================
  Files              194      196       +2     
  Lines            28045    28300     +255     
  Branches          5211     5297      +86     
===============================================
+ Hits             22761    22799      +38     
- Misses            3586     3808     +222     
+ Partials          1698     1693       -5     

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

@cole-miller
Copy link
Contributor Author

@just-now I would like to merge this into dqlite-next and follow up with additional tests/fixes in future, smaller PRs.

@cole-miller cole-miller requested a review from just-now April 5, 2024 10:42
Copy link
Contributor

@just-now just-now left a comment

Choose a reason for hiding this comment

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

This is the first part, haven't yet finished with the review.

src/vfs2.c Show resolved Hide resolved
src/vfs2.c Outdated Show resolved Hide resolved
src/vfs2.c Outdated Show resolved Hide resolved
Comment on lines +263 to +267
struct sqlite3_file base;
/* Flags passed to the xOpen that created this file. */
int flags;
/* File object created by the base (unix) VFS. Not used for WAL. */
sqlite3_file *orig;
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Can we use either struct sqlite3_file or just sqlite3_file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Needs to be a pointer since the size of sqlite3_file doesn't reflect the size of the file object created by the unix VFS -- it adds its own fields afterward just like we do with our struct file

src/vfs2.c Outdated Show resolved Hide resolved
src/vfs2.c Show resolved Hide resolved
src/vfs2.c Outdated Show resolved Hide resolved
src/vfs2.c Show resolved Hide resolved
src/vfs2.c Show resolved Hide resolved
src/vfs2.c Show resolved Hide resolved
Copy link
Contributor

@just-now just-now left a comment

Choose a reason for hiding this comment

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

Looks like I was too paranoid about e->shm_locks[]. Still please, comment out what are expectations regarding unlocking them as functions taking locks are not "symmetrical".

src/vfs2.c Show resolved Hide resolved
src/vfs2.c Show resolved Hide resolved
src/vfs2.c Outdated Show resolved Hide resolved
src/vfs2.c Show resolved Hide resolved
src/vfs2.c Outdated Show resolved Hide resolved
src/vfs2.c Outdated Show resolved Hide resolved
Signed-off-by: Cole Miller <[email protected]>
@cole-miller cole-miller requested a review from just-now April 18, 2024 09:30
Copy link
Contributor

@just-now just-now left a comment

Choose a reason for hiding this comment

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

+1

@cole-miller cole-miller merged commit 29dc967 into canonical:dqlite-next Apr 19, 2024
10 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