kv: bump timestamp cache to Pushee.MinTimestamp on PUSH_ABORT #60835
Conversation
|
This change is |
There was a problem hiding this comment.
We were only checking that the batch header timestamp was equal to or
greater than this pushee's min timestamp...
So? Issues with introducing timestamp in the tscache potentially above the local clock?
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @nvanbenschoten)
pkg/kv/kvserver/replica_tscache.go, line 185 at r1 (raw file):
} else { key = transactionPushMarker(start, pushee.ID) pushTS = pushee.WriteTimestamp
so why is this one safe if the other one isn't?
nvanbenschoten
force-pushed
the
nvanbenschoten/pushTime
branch
2 times, most recently
from
65e8855
to
fbc6796
Compare
There was a problem hiding this comment.
Issues with introducing timestamp in the tscache potentially above the local clock?
Right. Even though we've removed the assertion in #61130, I think we still want the invariant that the timestamp cache doesn't contain non-synthetic timestamps above the local clock, if only for the mixed-version cluster case.
Reviewable status:
pkg/kv/kvserver/replica_tscache.go, line 185 at r1 (raw file):
Previously, andreimatei (Andrei Matei) wrote…
so why is this one safe if the other one isn't?
Because we checked the PushTo timestamp against the local clock in cmd_push_txn.go, but this may be below the pushee's WriteTimestamp for PUSH_ABORT requests. I added a comment.
Fixes cockroachdb#60779. We were only checking that the batch header timestamp was equal to or greater than this pushee's min timestamp, so this is as far as we can bump the timestamp cache.
nvanbenschoten
force-pushed
the
nvanbenschoten/pushTime
branch
from
fbc6796
to
8ba492c
Compare
|
Friendly ping. |
|
Added the GA blocker label, as a GA blocker (#60773) depends on this. |
There was a problem hiding this comment.
No easy test to be written?
Reviewable status:
|
TFTR! This jumps right out on the kvnemesis test I'm about to add, so I'm satisfied with that. bors r+ |
|
Build failed: |
|
Flake in bors r+ |
|
Build succeeded: |
With cockroachdb#60835 merged, this test no longer flakes. I've stressed it on my GCE worker now for a while an it's all good. Resolves cockroachdb#60773. Release note: None
62954: sql: Re-enable multi_region_backup test r=arulajmani a=ajstorm With #60835 merged, this test no longer flakes. I've stressed it on my GCE worker now for a while an it's all good. Resolves #60773. Release note: None 62959: sql: lease acquisition of OFFLINE descs may starve bulk operations r=ajwerner a=fqazi Fixes: #61798 Previously, offline descriptors would never have their leases cached and they would be released once the reference count hit zero. This was inadequate because when attempting to online these tables again the lease acquisition could be pushed back by other operations, leading to starvation / live locks. To address this, this patch will allow the leases of offline descriptors to be cached. Release note (bug fix): Lease acquisitions of descriptor in a offline state may starve out bulk operations (backup / restore) Co-authored-by: Adam Storm <storm@cockroachlabs.com> Co-authored-by: Faizan Qazi <faizan@cockroachlabs.com>
With cockroachdb#60835 merged, this test no longer flakes. I've stressed it on my GCE worker now for a while an it's all good. Resolves cockroachdb#60773. Release note: None
Fixes #60779.
Fixes #60580.
We were only checking that the batch header timestamp was equal to or
greater than this pushee's min timestamp, so this is as far as we can
bump the timestamp cache.