kv: don't consult ReadTimestamp in Transaction.LastActive#117119
kv: don't consult ReadTimestamp in Transaction.LastActive#117119craig[bot] merged 1 commit intocockroachdb:masterfrom
Conversation
Informs cockroachdb#101938. Without the synthetic timestamp bit, we don't know for sure whether the transaction's ReadTimestamp is a ClockTimestamp or not. To avoid comparing a future-time MVCC timestamp against a clock timestamp for purposes of detecting transaction liveness, we stop consulting the ReadTimestamp. This was always an unproven optimization anyway, so it's safe to remove. Release note: None
|
This change is |
arulajmani
left a comment
There was a problem hiding this comment.
Reviewed 1 of 3 files at r1, all commit messages.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @nvanbenschoten)
pkg/roachpb/data.go line 1000 at r1 (raw file):
// LastActive returns the last timestamp at which client activity definitely // occurred, i.e. the maximum of MinTimestamp and LastHeartbeat.
Why switch from ReadTimestamp to MinTimestamp here?
IIUC, MinTimestamp is the first ReadTimestamp for a transaction. From then on, the ReadTimestamp may advance if the transaction successfully refreshes -- don't we want to treat this advanced timestamp as last active, as there was activity at that timestamp which prompted the refresh?
nvb
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/roachpb/data.go line 1000 at r1 (raw file):
Previously, arulajmani (Arul Ajmani) wrote…
Why switch from
ReadTimestamptoMinTimestamphere?IIUC,
MinTimestampis the firstReadTimestampfor a transaction. From then on, theReadTimestampmay advance if the transaction successfully refreshes -- don't we want to treat this advanced timestamp as last active, as there was activity at that timestamp which prompted the refresh?
We want LastActive to have a relationship with real-time so that we can compare it with a clock reading and determine how long it has been since the transaction was last active. If we can't check whether the ReadTimestamp is synthetic then we don't know whether it's a future-time timestamp or not, so we stop trying.
I don't think this ever actually mattered anyway, as this is only used when pushing a transaction to see whether it has expired, in which case the LastHeartbeat is the important part. ReadTimestamp isn't even advanced in the txn record on txn heartbeats. So I think this was just an premature optimization from 8c0b2ec, as part of the much larger refactor in #42236.
arulajmani
left a comment
There was a problem hiding this comment.
Reviewable isn't letting me type comments for some reason, but your explanation makes sense. My bad on not reading the commit message properly 😅
|
TFTR! bors r+ |
|
Build succeeded: |
Informs #101938.
Without the synthetic timestamp bit, we don't know for sure whether the transaction's ReadTimestamp is a ClockTimestamp or not. To avoid comparing a future-time MVCC timestamp against a clock timestamp for purposes of detecting transaction liveness, we stop consulting the ReadTimestamp. This was always an unproven optimization anyway, so it's safe to remove.
Release note: None