release-20.2: kvserver: avoid bootstrapping closedTS state with the lease start time #65823
Conversation
aayushshah15
changed the title
|
This change is |
aayushshah15
force-pushed
the
stopForwardingClosedTSToLeaseStartTime
branch
from
94fd3ce to
c3441f0
Compare
aayushshah15
force-pushed
the
stopForwardingClosedTSToLeaseStartTime
branch
from
c3441f0 to
ac60e52
Compare
There was a problem hiding this comment.
Release note (bug fix): A rare bug present in 20.2 that allowed
AS OF SYSTEM TIMEqueries to serve inconsistent results has been resolved.
Consider saying a bit more here about what makes this bug "rare" (i.e. the necessary timing with a range merge and a lease change).
Reviewed 6 of 6 files at r1.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @andreimatei)
|
Hey @nvanbenschoten, @andreimatei and I were talking about this PR today and I was wondering how you feel about the regression in follower read availability that this PR concedes. After a lease transfer, follower reads on the range will be unavailable until the first closed timestamp update from the new leaseholder. Are we okay with that regression? |
This is a good point. It was the reason why we didn't make this change in v21.1 before the new closed timestamp system started to be used. @andreimatei should also weigh-in, but I think this regression is ok, given the correctness issues that this change is solving. Also, lease transfers are already disruptive to all other traffic. |
|
Would be nice to get this merged, since it can cause panics as well (due to a race condition with CDC, closed timestamps, and range merges). What's your take @andreimatei? |
There was a problem hiding this comment.
Reviewable status:
aayushshah15
force-pushed
the
stopForwardingClosedTSToLeaseStartTime
branch
3 times, most recently
from
4dc0424 to
8ecea13
Compare
This commit reverts the main change introduced in cockroachdb#35130. That change made it such that each replica could bootstrap its `maxClosed` timestamp value with the start time of the latest lease that it knew about. However, it doesn't consider the fact that when the lease transfer occurred, the range may have been in a subsumed state and thus, is not allowed to serve any requests past the subsumption time. Bumping a replica's closed timestamp by the lease start time allows for a bug where the closed timestamp of a replica may be advanced past the subsumption time of a range, which would allow the range's non-leaseholder replicas to serve follower reads past its subsumption time. Such a sequence of events would in turn allow follower reads queries to serve results that could be invalidated by future writes on the keyspan owned by the subsumed range. Release note (bug fix): A rare bug in 20.2 that manifested itself when a lease change occured during a range merge has been resolved. This bug allowed `AS OF SYSTEM TIME` queries to serve inconsistent results.
aayushshah15
force-pushed
the
stopForwardingClosedTSToLeaseStartTime
branch
from
8ecea13 to
c2a08bd
Compare
This commit reverts the main change introduced in #35130. That change made it
such that each replica could bootstrap its
maxClosedtimestamp value with thestart time of the latest lease that it knew about. However, it doesn't consider
the fact that when the lease transfer occurred, the range may have been in a
subsumed state and thus, is not allowed to serve any requests past the
subsumption time. Bumping a replica's closed timestamp by the lease start time
allows for a bug where the closed timestamp of a replica may be advanced past
the subsumption time of a range, which would allow the range's non-leaseholder
replicas to serve follower reads past its subsumption time.
Such a sequence of events would in turn allow follower reads queries to serve
results that could be invalidated by future writes on the keyspan owned by the
subsumed range.
Release note (bug fix): A rare bug in 20.2 that manifested itself when a lease
change occured during a range merge has been resolved. This bug allowed
AS OF SYSTEM TIMEqueries to serve inconsistent results.