kvserver: fix bug in ephemeral app batch #94930
Conversation
|
This change is |
There was a problem hiding this comment.
Actually, I believe that change only made an existing bug (or at least
semantic incorrectness) more common: it was removing that update
altogether, but the old code blindly updated the MaxLeaseIndex, which
also doesn't seem correct as not all commands (in particular leases)
specify it in the first place. So conceivably, a command that would
be rejected could have reset the MaxLeaseIndex and then allowed a
later command that should also have been rejected to pass.I think this was mostly theoretical in nature since we only batch
"trivial" commands
Right, we were only batching trivial commands that could not affect the outcome of later commands (other than through the lease applied index check). Admin operations like splits and especially lease requests have more complicated side effects, so we never consider them or entries later than them for early ack-ing. In fact, we never batch them at all.
Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @pavelkalinnikov and @tbg)
pkg/kv/kvserver/replica_app_batch.go line 768 at r1 (raw file):
fr = replicaApplyTestingFilters(ctx, mb.r, cmd, fr) cmd.ForcedErrResult = fr if !cmd.Rejected() && cmd.LeaseIndex > mb.state.LeaseAppliedIndex {
Should we change this to exactly mirror the logic in replicaAppBatch.stageTrivialReplicatedEvalResult?
A bug was introduced recently[^1] which could lead to commands being early-acked despite ultimately being rejected during command application. The reason for this was that we previously updated the lease applied index of the ephemeral batch and didn't any more, meaning that commands that would fail the MaxLeaseIndex check on the actual apply batch could erroneously pass it on the ephemeral batch. Actually, I believe that change only made an existing bug (or at least semantic incorrectness) more common: it was removing that update altogether, but the old code blindly updated the MaxLeaseIndex, which also doesn't seem correct as not all commands (in particular leases) specify it in the first place. So conceivably, a command that would be rejected could have reset the MaxLeaseIndex and then allowed a later command that should also have been rejected to pass. I think this was mostly theoretical in nature since we only batch "trivial" commands but this PR improves the update to only occur for non-rejected commands. A regression test is added. Thanks to Nathan for bisecting and diagnosis. No release note because unreleased. Fixes cockroachdb#94409. [^1]: cockroachdb@e61de15#diff-0fd06523f1f485024aef0c7a11d3472945d5ac7cf228d6007b2475ccf6f44cd6L795 Epic: CRDB-220 Release note: None
Let me know if you see a nice way of doing that and I'll send a follow-up, but this didn't seem trivial to me. The "real" code relies on the command having been cleared if it has been rejected but the ephemeral batch here is sort of weird already in that it mutates the command (assigning the results of the forced err check), but it seems even weirder if it cleared it out (maybe I'm wrong?). TFTR! bors r=nvanbenschoten |
|
Build failed (retrying...): |
|
This PR was included in a batch that successfully built, but then failed to merge into master. It will not be retried. Additional information: |
|
bors r+ |
|
bors single on |
|
bors r- |
|
Canceled. |
|
bors r+ single off |
|
This PR was included in a batch that successfully built, but then failed to merge into master. It will not be retried. Additional information: |
|
bors r- |
1 similar comment
|
bors r- |
A bug was introduced recently1 which could lead to commands being
early-acked despite ultimately being rejected during command
application. The reason for this was that we previously updated
the lease applied index of the ephemeral batch and didn't any more,
meaning that commands that would fail the MaxLeaseIndex check on the
actual apply batch could erroneously pass it on the ephemeral batch.
Actually, I believe that change only made an existing bug (or at least
semantic incorrectness) more common: it was removing that update
altogether, but the old code blindly updated the MaxLeaseIndex, which
also doesn't seem correct as not all commands (in particular leases)
specify it in the first place. So conceivably, a command that would
be rejected could have reset the MaxLeaseIndex and then allowed a
later command that should also have been rejected to pass.
I think this was mostly theoretical in nature since we only batch
"trivial" commands but this PR improves the update to only occur
for non-rejected commands.
A regression test is added.
Thanks to Nathan for bisecting and diagnosis.
No release note because unreleased.
Fixes #94409.
Epic: CRDB-220
Release note: None
Footnotes
https://github.com/cockroachdb/cockroach/commit/e61de155017a1821d08ba0a414967927adabe0eb#diff-0fd06523f1f485024aef0c7a11d3472945d5ac7cf228d6007b2475ccf6f44cd6L795 ↩