Two-node vfs2 unit test

[cole-miller]

This PR contains a new unit test for vfs2 that simulates replicating a transaction between a leader and a follower, plus supporting fixes:

• Remove FLUSH from the state machine and vfs2_commit_barrier from the public API.
• Several fixes picked from the previous integration PR (see individual commits for more details):
  • Replace vfs2_wal_frame with dqlite_vfs_frame.
  • Calculate and set the checksum fields in the WAL header.
  • Initialize the page size for a database, if not already set, when calling vfs2_add_uncommitted.
  • Call sqlite3_randomness instead of xRandomness from the unix VFS to randomize the WAL header salts.
• Updating the hash table in the first shm region of the WAL-index whenever mxFrame increases on the follower.
• Properly writing the WAL header when the WAL is written for the first time.
• Finalizing the memory allocated for the WAL-index when an entry is closed, rather than when xShmUnmap is last called.

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

[codecov]

Codecov Report

Attention: Patch coverage is 71.76471% with 144 lines in your changes missing coverage. Please review.

Project coverage is 78.15%. Comparing base (ee07596) to head (3951d1e).
Report is 100 commits behind head on master.

Files with missing lines	Patch %	Lines
src/vfs2.c	65.63%	80 Missing and 64 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #674      +/-   ##
==========================================
+ Coverage   77.56%   78.15%   +0.59%     
==========================================
  Files         197      197              
  Lines       27638    27962     +324     
  Branches     5486     5532      +46     
==========================================
+ Hits        21438    21855     +417     
+ Misses       4326     4262      -64     
+ Partials     1874     1845      -29     

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

[letFunny]

Thanks Cole this is looking very good! The new comments and the commit descriptions are super helpful and make the code much easier to follow (though you extensive explanations might have also helped :P). I left a bunch of comments but there are a lot of nitpicks and questions.

The only part that I am not sure I understand is the removal of the FLUSH state, and I couldn't find the reasoning behind that in the commit or in the code. It will probably make sense once we discuss it offline and/or once the new diagram is in place.

[letFunny]

[Nit]: Should we add start != 0 && ... for clarity with the condition below? That way we don't have to remember that mx < wal_cursor && mx > 0 => wal_cursor > 0

[cole-miller]

Yes, thanks, this could use refactoring.

[cole-miller]

This logic has been substantially re-organized in a follow-up commit (43ec47c), hopefully it's more clear now!

[cole-miller]

As a supplement to the observability improvements in this PR, here's a chronoscope.yaml:

.p1: &tick_parser_positions
  id: 10
  pid: 8
  type: 6
  time: 1
  event: 11

.p2: &rel_parser_positions
  type: 6
  orig_pid: 8
  dest_pid: 10
  orig_id: 12
  dest_id: 14

.p3: &attr_parser_positions
  id: 7
  pid: 5
  type: 3
  name: 8
  value: 1

tick:
  - type: wtx
    pos: *tick_parser_positions
  - type: wal
    pos: *tick_parser_positions
  - type: wlk
    pos: *tick_parser_positions
  - type: shm
    pos: *tick_parser_positions
  - type: ckpt
    pos: *tick_parser_positions

relation:
  - type: wtx-to-wtx
    pos: *rel_parser_positions
  - type: wtx-to-wal
    pos: *rel_parser_positions
  - type: wtx-to-wlk
    pos: *rel_parser_positions
  - type: wtx-to-shm
    pos: *rel_parser_positions
  - type: wtx-to-ckpt
    pos: *rel_parser_positions

attr:

[letFunny]

First part of the review, I am not sure if GH is able to keep up when switching these many commits, so I am posting it in two parts.

[letFunny]

If we are adding code for the tests here because of visibility I wonder if there is a better solution like making the inner fields visible for the tests, an attribute that makes this function uncallable from main code, etc.

[cole-miller]

I'm reluctant to move our struct file or struct entry into vfs2.h---it seems good to maintain the conciseness of that file and keep all the implementation details of vfs2 in one place. As for making the function uncallable outside tests, I agree it would be nice to be able to ifdef out bits of code so that they only exist if dqlite is being built for testing (and this is a good candidate). We'd need to tinker with the build system to implement this, because right now we don't (always) compile source files separately for the installable artifacts and the tests.

I think a good local improvement for this function specifically would be to just expose an accessor for the wtx sm, which will be useful also outside the unit tests (e.g. we'll want to sm_relate it to other state machines related to write transactions). What do you think?

[letFunny]

Finished the review. I think the code is looking very good Cole and kudos for all the comments! Even though it has a lot of moving pieces that you need to keep track of I found the code very easy to follow.

No major comments, mostly questions and suggestions.

[letFunny]

Nit: you can move this closer to usage.

[cole-miller]

I'm not sure about this---it's used by shm_init which is just two functions down (and the function in the middle, write_basic_hdr_cksums, is also used by shm_init). (I do think the overall order of functions in the file needs tidying up, just not sure how to achieve a quick improvement in this case.)

[letFunny]

Maybe we should initialize shm here to zero just in case the caller is using uninitialized memory and that leads to bugs, I cannot think of any use case that requires setting something by hand.

BTW: another custom vector type we just created.

[cole-miller]

Maybe we should initialize shm here to zero just in case the caller is using uninitialized memory and that leads to bugs, I cannot think of any use case that requires setting something by hand.

I don't think this is true of any of the callsites right now, but better safe than sorry. I'll add the defensive initialization.

BTW: another custom vector type we just created.

I disclaim responsibility for this one since it's the same implementation from the old VFS :)

[cole-miller]

Looking at this again, I think maybe it would make more sense to have an invariant for the shm state machine that asserts that the other fields are zeroed when the state is SHM_EMPTY. Defensively zeroing them here would suggest that we expect the struct to be filled with garbage, but that's not the case, we expect it to be "initialized as empty" already. (Maybe shm_init is not the best name for that reason.)