vsr: synchronize op and commit_max updates on_prepare #1576

matklad · 2024-02-20T15:44:45Z

Before, op was updated first, and commit_max at the end, breaking

assert(self.commit_max >= self.op -| constants.pipeline_prepare_queue_max);

invariant. Broken invariant was observed by, eg, op_repair_min (see the first commit).

To fix this:

rearrange the code a bit, to separate logical update to commit_max from attempting to commit stuff
move logical update forward in on_prepare flow

This currently fails: when appending a prepare, we set op and commit_max separately. Will fix in the next commit. Seed: 1648503712295043012

matklad · 2024-02-20T15:59:11Z

src/vsr/replica.zig

-                });
-                self.commit_max = self.commit_min;
-            }
+            self.advance_commit_max(self.commit_min, @src().fn_name);


This feels something suspicious. I think it would be better to update commit_max before the primary starts committing, not after it is done. Though I guess for solo mode updating later is actually desirable?

🤔 I like the "on primary, commit_min == commit_max invariant" that we have.

Ahh, this feels extra tricky --- it seems like we could be advancing commit_max here even if we are no longer primary, if view change happens while we are committing.

🤔 if we allow primary's commit_max to run ahead of commit_min, that might increase latency tolerance. If the primary is slow to commit, clients could get replies from backups which commit ahead of the primary.

sentientwaffle · 2024-02-20T16:02:19Z

src/vsr/replica.zig

@@ -1393,14 +1393,11 @@ pub fn ReplicaType(
            assert(message.header.op > self.op);
            assert(message.header.op > self.commit_min);


Maybe add a assert(message.header.op > message.header.commit); to emphasize that the self.advance_commit_max(message.header.commit, @src().fn_name); below is ok even though we won't have loaded message.header into our journal yet.

advancing commit_max should be ok even if we don't have a header? We allow maybe(self.op < self.commit_max)

Ah, good point!

sentientwaffle · 2024-02-20T16:32:46Z

src/vsr/replica.zig

@@ -309,7 +309,6 @@ pub fn ReplicaType(
        /// * never decreases.
        /// Invariants (status=normal primary):
        /// * `replica.commit_max = replica.commit_min`.
-        /// * `replica.commit_max = replica.op - pipeline.queue.prepare_queue.count`.


Where does this not hold?

Ah, forgot to mention that I was confused by this. This almost never holds? For example, in an idle cluster replica.commit_max = replica.op. Or am I misreading this?

pipeline.queue.prepare_queue.count is the number of items currently in the pipeline, not the capacity. So in an idle cluster pipeline.queue.prepare_queue.count == 0.

Ah, indeed, I misread that as pipeline_prepare_queue_max 🤦

Ok, now advance_commit_max actually checks that invariant (had to move pipeline prepare popping to commit_op for that)

matklad · 2024-02-20T17:14:36Z

src/vsr/replica.zig

@@ -784,7 +783,8 @@ pub fn ReplicaType(

            if (self.solo()) {
                if (self.commit_min < self.op) {
-                    self.commit_journal(self.op);
+                    self.advance_commit_max(self.op, @src().fn_name);


not sure whether I like @src().fn_name. On the one hand, it is cryptic and too clever. On the other hand, we did have bugs where we moved code around but didn't update call-sites.

If we double-down here, we might pass the whole @src() in even

Before, `commit_journal` played dual role: * it advanced commit_max * it tried executing everything up to commit_max In parciular, we had funny calls `commit_journal(self.commit_max)`, which neutered the first role in a somewhat roundabout way. Now, we have two functions: * "sensitive" `advance_commit_max` to be called when you have learned something that justifies _logically_ committing more * "safe" `commit_journal`, which can be called whenever, and just executes locally available prepares up to commit_max There shouldn't be any behavior changes here, but in the next commit I'll tweak the logic in `on_prepare` to advance commit_max _before_ appending the prepare, so as not to break op/commit_max invariant even temporarily.

Before, op was updated first, and commit_max at the end, breaking ``` assert(self.commit_max >= self.op -| constants.pipeline_prepare_queue_max); ``` invariant Seed: 1648503712295043012

matklad · 2024-02-20T18:05:09Z

Apparently, it's better not to forec-push VOPR-labeled pull requests!

sentientwaffle · 2024-02-20T18:37:28Z

src/vsr/replica.zig

-                    assert(prepare.message.header.op == self.commit_max);
-                    assert(prepare.ok_quorum_received);
-                }
-


I did like how "pop prepare" was immediately followed by "pop request -> push prepare to pipeline". But commit_op is already pretty gigantic 😓

vsr: assert op/commit_max invariant in op_repair min

ed77c8e

This currently fails: when appending a prepare, we set op and commit_max separately. Will fix in the next commit. Seed: 1648503712295043012

matklad added the vopr VOPR Hub testing is enabled label Feb 20, 2024

matklad assigned sentientwaffle Feb 20, 2024

matklad force-pushed the matklad/advance-commit-max branch from 654ad1e to 4d7d2a6 Compare February 20, 2024 15:47

matklad commented Feb 20, 2024

View reviewed changes

sentientwaffle reviewed Feb 20, 2024

View reviewed changes

matklad commented Feb 20, 2024

View reviewed changes

matklad force-pushed the matklad/advance-commit-max branch 2 times, most recently from 763793e to 4196714 Compare February 20, 2024 17:36

matklad added 2 commits February 20, 2024 17:57

vsr: synchronize op and commit_max updates on_prepare

3d021fd

Before, op was updated first, and commit_max at the end, breaking ``` assert(self.commit_max >= self.op -| constants.pipeline_prepare_queue_max); ``` invariant Seed: 1648503712295043012

matklad force-pushed the matklad/advance-commit-max branch from 4196714 to 3d021fd Compare February 20, 2024 17:57

matklad removed the vopr VOPR Hub testing is enabled label Feb 20, 2024

matklad requested a review from sentientwaffle February 20, 2024 18:28

sentientwaffle approved these changes Feb 20, 2024

View reviewed changes

matklad added this pull request to the merge queue Feb 20, 2024

Merged via the queue into main with commit c318c49 Feb 20, 2024
25 checks passed

matklad deleted the matklad/advance-commit-max branch February 20, 2024 18:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

vsr: synchronize op and commit_max updates on_prepare #1576

vsr: synchronize op and commit_max updates on_prepare #1576

matklad commented Feb 20, 2024

matklad Feb 20, 2024

sentientwaffle Feb 20, 2024

matklad Feb 20, 2024

matklad Feb 20, 2024

sentientwaffle Feb 20, 2024

matklad Feb 20, 2024

sentientwaffle Feb 20, 2024

sentientwaffle Feb 20, 2024

matklad Feb 20, 2024

sentientwaffle Feb 20, 2024

matklad Feb 20, 2024

matklad Feb 20, 2024

matklad Feb 20, 2024

matklad commented Feb 20, 2024

sentientwaffle Feb 20, 2024

		@@ -1393,14 +1393,11 @@ pub fn ReplicaType(
		assert(message.header.op > self.op);
		assert(message.header.op > self.commit_min);

vsr: synchronize op and commit_max updates on_prepare #1576

vsr: synchronize op and commit_max updates on_prepare #1576

Conversation

matklad commented Feb 20, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matklad commented Feb 20, 2024

Choose a reason for hiding this comment