kvserver: add MVCC-compliant AddSSTable variant#72085
kvserver: add MVCC-compliant AddSSTable variant#72085craig[bot] merged 4 commits intocockroachdb:masterfrom
AddSSTable variant#72085Conversation
9b73a35 to
768d3b6
Compare
768d3b6 to
2598c11
Compare
9995d6f to
8bb7a20
Compare
6f275ce to
e75afdc
Compare
erikgrinaker
left a comment
There was a problem hiding this comment.
Reviewable status:
complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @aliher1911, @andreimatei, @dt, @jbowens, @sumeerbhola, and @tbg)
pkg/roachpb/api.go, line 95 at r23 (raw file):
Previously, tbg (Tobias Grieger) wrote…
Very nice!
How aboutisAdmin -> isAlone
skipsLeaseCheck -> isAloneand of course we could entertain
skipsLeaseCheck -> !isIntentWrite
and probably a lot more of that kindbut maybe that's past the scope of this PR, very happy about the unit test already.
Also I don't understand
isPrefix, can you add more words while you're here?I wish we just had, like 12, APIs for all of these requests instead of jamming them all into batches
Sure, done. I'm not familiar with a lot of the interactions here, but at least we have the infra in place to add them.
91c434b to
2ddd715
Compare
|
Looks good for the KV side, so I think we're mostly waiting for @sumeerbhola at this point. |
2ddd715 to
29fb61c
Compare
|
I opened #73047 to track the possibility of making blind |
|
Didn't realize this was waiting on me. I was happy after my last comments were addressed. |
29fb61c to
e1a49d5
Compare
|
bors r=dt,tbg,sumeerbhola,nvanbenschoten TFTRs! |
|
Merge conflict. |
This renames `ScanIntents` from `ScanSeparatedIntents`, and adds a context parameter. Callers have been updated to pass `storage.mvcc.max_intents_per_error` for the intent limit, as is done elsewhere, and `targetBytes` is passed as 0 for now (no limit) since other intent collectors currently don't use a byte limit. Release note: None
Request flags have implicit dependencies and incompatibilities (e.g. `isLocking` implies `isTxn`). However, these were never checked and developers were expected to satisfy them manually, which is error-prone. This patch adds `TestFlagCombinations` that checks these dependencies and incompatibilities, based on `flagDependencies` and `flagExclusions` maps which encodes them. It also adds a new `flag` type for flags, renames `skipLeaseCheck` to `skipsLeaseCheck`, and adds `isAlone` for `CheckConsistencyRequest`. Release note: None
Previously, `isIntentWrite` determined whether a request checked the timestamp cache and possibly pushed its timestamp. However, some requests may want to check the timestamp cache without writing intents, notably an MVCC-compliant `AddSSTable` request. This patch introduces a new flag `appliesTSCache`, and uses it as a condition for applying the timestamp cache to the request. Release note: None
`AddSSTable` does not comply with MVCC, the timestamp cache, nor the closed timestamp, since the SST MVCC timestamps are written exactly as given and thus can rewrite history. This patch adds three new parameters that can make `AddSSTable` fully MVCC-compliant, with a corresponding `MVCCAddSSTable` version gate: * `WriteAtRequestTimestamp`: rewrites the MVCC timestamps to the request timestamp, complying with the timestamp cache and closed timestamp. * `DisallowConflicts`: checks for any conflicting keys and intents at or above the SST's MVCC timestamps, complying with MVCC. * `DisallowShadowingBelow`: implies `DisallowConflicts`, and also errors if shadowing visible keys (but not tombstones). Unlike the existing `DisallowShadowing`, this allows shadowing existing keys above the given timestamp if the new key has the same value as the existing one, and also allows idempotent writes at or above the given timestamp. The existing `DisallowShadowing` parameter implies `DisallowConflicts`, and also errors on any existing visible keys below the SST key's timestamp (but not tombstones). It no longer allows replacing a tombstone with a value at the exact same timestamp. Additionally, even blind `AddSSTable` requests that do not check for conflicts now take out lock spans and scan for existing intents, returning a `WriteIntentError` to resolve them. This should be cheap in the common case, since the caller is expected to ensure there are no concurrent writes over the span, and so there should be no or few intents. The `WriteAtRequestTimestamp` SST rewrite implementation here is correct but slow. Optimizations will be explored later. Release note: None
e1a49d5 to
54f5746
Compare
|
bors r=dt,tbg,sumeerbhola,nvanbenschoten |
|
Build succeeded: |
storage: tweak
ScanIntents()This renames
ScanIntentsfromScanSeparatedIntents, and adds acontext parameter. Callers have been updated to pass
storage.mvcc.max_intents_per_errorfor the intent limit, as is doneelsewhere, and
targetBytesis passed as 0 for now (no limit) sinceother intent collectors currently don't use a byte limit.
Touches #72563.
Release note: None
roachpb: assert request flag combinations
Request flags have implicit dependencies and incompatibilities (e.g.
isLockingimpliesisTxn). However, these were never checked anddevelopers were expected to satisfy them manually, which is error-prone.
This patch adds
TestFlagCombinationsthat checks these dependenciesand incompatibilities, based on
flagDependenciesandflagExclusionsmaps which encodes them.
It also adds a new
flagtype for flags, renamesskipLeaseChecktoskipsLeaseCheck, and addsisAloneforCheckConsistencyRequest.Release note: None
roachpb: add
appliesTSCacherequest flagPreviously,
isIntentWritedetermined whether a request checked thetimestamp cache and possibly pushed its timestamp. However, some
requests may want to check the timestamp cache without writing intents,
notably an MVCC-compliant
AddSSTablerequest.This patch introduces a new flag
appliesTSCache, and uses it as acondition for applying the timestamp cache to the request.
Release note: None
kvserver: add MVCC-compliant
AddSSTablevariantAddSSTabledoes not comply with MVCC, the timestamp cache, nor theclosed timestamp, since the SST MVCC timestamps are written exactly as
given and thus can rewrite history.
This patch adds three new parameters that can make
AddSSTablefullyMVCC-compliant, with a corresponding
MVCCAddSSTableversion gate:WriteAtRequestTimestamp: rewrites the MVCC timestamps to the requesttimestamp, complying with the timestamp cache and closed timestamp.
DisallowConflicts: checks for any conflicting keys and intents at orabove the SST's MVCC timestamps, complying with MVCC.
DisallowShadowingBelow: impliesDisallowConflicts, and also errorsif shadowing visible keys (but not tombstones). Unlike the existing
DisallowShadowing, this allows shadowing existing keys above the giventimestamp if the new key has the same value as the existing one, and
also allows idempotent writes at or above the given timestamp.
The existing
DisallowShadowingparameter impliesDisallowConflicts,and also errors on any existing visible keys below the SST key's
timestamp (but not tombstones). It no longer allows replacing a
tombstone with a value at the exact same timestamp.
Additionally, even blind
AddSSTablerequests that do not check forconflicts now take out lock spans and scan for existing intents,
returning a
WriteIntentErrorto resolve them. This should be cheapin the common case, since the caller is expected to ensure there are no
concurrent writes over the span, and so there should be no or few
intents.
The
WriteAtRequestTimestampSST rewrite implementation here is correctbut slow. Optimizations will be explored later.
Resolves #70422.
Release note: None