storage: don't consult TxnSpanGCThreshold when creating txn records#34778
storage: don't consult TxnSpanGCThreshold when creating txn records#34778craig[bot] merged 1 commit intocockroachdb:masterfrom
Conversation
|
This change is |
tbg
left a comment
There was a problem hiding this comment.
Reviewed 13 of 13 files at r1.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @andreimatei)
andreimatei
left a comment
There was a problem hiding this comment.
LGTM
#cleanerEveryday
Reviewable status:
pkg/roachpb/api.proto, line 679 at r1 (raw file):
// TxnSpanGCThreshold is the timestamp below which inactive transactions were // considered for GC (and thus might have been removed). // TODO(nvanbenschoten): Remove this in 2.3, at which point we won't need to
2.3->19.2 everywhere
pkg/roachpb/errors.proto, line 138 at r1 (raw file):
// got GCed (so, we might be in the ABORT_REASON_ABORTED_RECORD_FOUND case and // not know). // TODO(nvanbenschoten): Remove this error reason in 2.3, at which point no
2.3->19.2
pkg/storage/replica_tscache.go, line 266 at r1 (raw file):
// information to transactions who have not yet written their transaction // record. In doing so, it protects against transaction records from being // created with too low of a timestamp if a PushTxn(TIMESTAMP) has succeeded
I'm very tired today, but it took me a second to parse the difference between "being created with too low of a timestamp" and "being written in the first place".
I ignored the "with too low of a ts" part partially because the one clause uses "created" when another uses "written" and "in the first place" is confusing.
I'd suggest:
s/in the first place/at all
s/being written/being created
and emphasizing that in the PushTxn(ts) case, the txn record will be written with a higher timestamp.
Cleanup from cockroachdb#33523. This commit weakens the semantics of `Replica.CanCreateTxnRecord` slightly to allow it to ignore the `TxnSpanGCThreshold`. `CanCreateTxnRecord` no longer consults the `TxnSpanGCThreshold`, so it doesn't prevent the creation of new transaction records beneath it. That's ok though, because we still prevent the recreation of aborted and GCed transaction records, which was the important part of the check. This protection is now provided by updating the write timestamp cache on successful `TxnPush(ABORT)` requests (added in cockroachdb#33523). The GC queue will always `PushTxn(ABORT)` transactions if it finds them `PENDING`, so the `TxnSpanGCThreshold` check isn't giving us anything important. For a visual summary of this change, look at the update to the state machine diagram in `replica_tscache.go`. _### Migration Safety In the next release cycle, this change will allow us to remove the `TxnSpanGCThreshold` entirely because the protection it was providing is now entirely handled by the timestamp cache. In the meantime, we need to keep setting it to ensure that the protection still works properly on old (v2.1) leaseholders. However, we're safe to no-longer consult it at all on new (v2.2) leaseholders. This is because even if a new node is given the lease from an old node that isn't correctly bumping the timestamp cache on `PushTxn` requests, we know that the old node will have a clock at least as large as a txn in question's OrigTimestamp. That means that the new leaseholder will necessarily bump its write timestamp cache to at least as large as this txn's OrigTimestamp, so `Replica.CanCreateTxnRecord` will always fail. Release note: None
e5a7861 to
1bf315d
Compare
|
bors r+ |
nvb
left a comment
There was a problem hiding this comment.
TFTRs.
Reviewable status:
pkg/roachpb/api.proto, line 679 at r1 (raw file):
Previously, andreimatei (Andrei Matei) wrote…
2.3->19.2 everywhere
I'll do that all in one go.
pkg/roachpb/errors.proto, line 138 at r1 (raw file):
Previously, andreimatei (Andrei Matei) wrote…
2.3->19.2
Ditto.
pkg/storage/replica_tscache.go, line 266 at r1 (raw file):
Previously, andreimatei (Andrei Matei) wrote…
I'm very tired today, but it took me a second to parse the difference between "being created with too low of a timestamp" and "being written in the first place".
I ignored the "with too low of a ts" part partially because the one clause uses "created" when another uses "written" and "in the first place" is confusing.I'd suggest:
s/in the first place/at all
s/being written/being createdand emphasizing that in the
PushTxn(ts)case, the txn record will be written with a higher timestamp.
Done.
34778: storage: don't consult TxnSpanGCThreshold when creating txn records r=nvanbenschoten a=nvanbenschoten Cleanup from #33523. This commit weakens the semantics of `Replica.CanCreateTxnRecord` slightly to allow it to ignore the `TxnSpanGCThreshold`. `CanCreateTxnRecord` no longer consults the `TxnSpanGCThreshold`, so it doesn't prevent the creation of new transaction records beneath it. That's ok though, because we still prevent the recreation of aborted and GCed transaction records, which was the important part of the check. This protection is now provided by updating the write timestamp cache on successful `TxnPush(ABORT)` requests (added in #33523). The GC queue will always `PushTxn(ABORT)` transactions if it finds them `PENDING`, so the `TxnSpanGCThreshold` check isn't giving us anything important. For a visual summary of this change, look at the update to the state machine diagram in `replica_tscache.go`. ### Migration Safety In the next release cycle, this change will allow us to remove the `TxnSpanGCThreshold` entirely because the protection it was providing is now entirely handled by the timestamp cache. In the meantime, we need to keep setting it to ensure that the protection still works properly on old (v2.1) leaseholders. However, we're safe to no-longer consult it at all on new (v2.2) leaseholders. This is because even if a new node is given the lease from an old node that isn't correctly bumping the timestamp cache on `PushTxn` requests, we know that the old node will have a clock at least as large as a txn in question's OrigTimestamp. That means that the new leaseholder will necessarily bump its write timestamp cache to at least as large as this txn's OrigTimestamp, so `Replica.CanCreateTxnRecord` will always fail. Release note: None Co-authored-by: Nathan VanBenschoten <nvanbenschoten@gmail.com>
Build succeeded |
Cleanup from #33523.
This commit weakens the semantics of
Replica.CanCreateTxnRecordslightly toallow it to ignore the
TxnSpanGCThreshold.CanCreateTxnRecordno longerconsults the
TxnSpanGCThreshold, so it doesn't prevent the creation of newtransaction records beneath it. That's ok though, because we still prevent the
recreation of aborted and GCed transaction records, which was the important part
of the check. This protection is now provided by updating the write timestamp
cache on successful
TxnPush(ABORT)requests (added in #33523). The GC queuewill always
PushTxn(ABORT)transactions if it finds themPENDING, so theTxnSpanGCThresholdcheck isn't giving us anything important.For a visual summary of this change, look at the update to the state machine
diagram in
replica_tscache.go.Migration Safety
In the next release cycle, this change will allow us to remove the
TxnSpanGCThresholdentirely because the protection it was providing isnow entirely handled by the timestamp cache.
In the meantime, we need to keep setting it to ensure that the protection still
works properly on old (v2.1) leaseholders. However, we're safe to no-longer
consult it at all on new (v2.2) leaseholders. This is because even if a new node
is given the lease from an old node that isn't correctly bumping the timestamp
cache on
PushTxnrequests, we know that the old node will have a clock atleast as large as a txn in question's OrigTimestamp. That means that the new
leaseholder will necessarily bump its write timestamp cache to at least as large
as this txn's OrigTimestamp, so
Replica.CanCreateTxnRecordwill always fail.Release note: None