kvserver: fix a reproposal check #59502
Conversation
|
This change is |
There was a problem hiding this comment.
Nice catch!
Reviewed 2 of 2 files at r1.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @andreimatei)
pkg/kv/kvserver/replica_application_result.go, line 208 at r1 (raw file):
minTS, untrack := r.store.cfg.ClosedTimestamp.Tracker.Track(ctx) defer untrack(ctx, 0, 0, 0) // covers all error paths below if p.Request.WriteTimestamp().Less(minTS) {
Should this be LessEq? I think we need to push all writes above the closed timestamp, not just to it.
pkg/kv/kvserver/replica_application_result.go, line 208 at r1 (raw file):
minTS, untrack := r.store.cfg.ClosedTimestamp.Tracker.Track(ctx) defer untrack(ctx, 0, 0, 0) // covers all error paths below if p.Request.WriteTimestamp().Less(minTS) {
I wonder if we should be checking what's in the request and only doing this if the request needs to respect the closed timestamp. There are certain requests, like a lone EndTxn request that we don't want bumped by the closed timestamp, which is currently the behavior they get in applyTimestampCache.
pkg/roachpb/batch.go, line 77 at r1 (raw file):
} // WriteTimestamp returns the timestamps at which this request is writing. For
Did you consider LatestActiveTimestamp instead, to be slightly more general and to parallel the method above this?
Either way, thanks for adding this!
There was a problem hiding this comment.
Reviewable status:
pkg/kv/kvserver/replica_application_result.go, line 208 at r1 (raw file):
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
Should this be
LessEq? I think we need to push all writes above the closed timestamp, not just to it.
Fuck if I know. The variable is called minTS and there's no comments on TrackerI.Track(). applyTimestampCache() indeed bumps above this. I've changed it in the hope it'll all go away soon.
pkg/kv/kvserver/replica_application_result.go, line 208 at r1 (raw file):
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
I wonder if we should be checking what's in the request and only doing this if the request needs to respect the closed timestamp. There are certain requests, like a lone
EndTxnrequest that we don't want bumped by the closed timestamp, which is currently the behavior they get inapplyTimestampCache.
good point, see now
pkg/roachpb/batch.go, line 77 at r1 (raw file):
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
Did you consider
LatestActiveTimestampinstead, to be slightly more general and to parallel the method above this?Either way, thanks for adding this!
I've considered it but... I wouldn't know what "latest active" would mean. For "earliest active" there seem to be some subtleties needed to separate it from ReadTimestamp, but not so here.
andreimatei
force-pushed
the
repropose-write-timestamp
branch
2 times, most recently
from
73e4d81 to
c8f5cd2
Compare
There was a problem hiding this comment.
Reviewed 2 of 3 files at r2.
Reviewable status:
pkg/roachpb/batch.go, line 95 at r2 (raw file):
// avoid re-writing history. func (ba *BatchRequest) ConsultsTimestampCache() bool { for _, union := range ba.Requests {
You can move this down right below IsUnsplittable and implement it as return ba.hasFlag(consultsTSCache)
In some situations, we repropose a command with a new LAI. When doing so, we need to make sure that the closed timestamp has not advanced past the command's write timestamp since the original proposal. Except we were checking the command's read timestamp, not write timestamp. Also changes a Less to a LessEq when comparing with the closed timestamp, which I think is how that comparison should be. Release note (bug fix): Fixed a very rare chance of inconsistent follower reads.
There was a problem hiding this comment.
Reviewable status:
pkg/roachpb/batch.go, line 95 at r2 (raw file):
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
You can move this down right below
IsUnsplittableand implement it asreturn ba.hasFlag(consultsTSCache)
done
andreimatei
force-pushed
the
repropose-write-timestamp
branch
from
c8f5cd2 to
645882d
Compare
There was a problem hiding this comment.
Reviewed 2 of 3 files at r3.
Reviewable status:
There was a problem hiding this comment.
bors r+
Reviewable status:
|
Build succeeded: |
In some situations, we repropose a command with a new LAI. When doing
so, we need to make sure that the closed timestamp has not advanced past
the command's write timestamp since the original proposal. Except we
were checking the command's read timestamp, not write timestamp.
Also changes a Less to a LessEq when comparing with the closed
timestamp, which I think is how that comparison should be.
Release note (bug fix): Fixed a very rare chance of inconsistent
follower reads.