backupccl: create readAsOfIterator for RESTORE #77281
Conversation
|
This change is |
msbutler
force-pushed
the
butler_multi_iter_asof_wrapper
branch
from
4b2ba3c to
ea04166
Compare
msbutler
force-pushed
the
butler_multi_iter_asof_wrapper
branch
from
ea04166 to
672746e
Compare
msbutler
force-pushed
the
butler_multi_iter_asof_wrapper
branch
from
672746e to
8eac3fc
Compare
pkg/storage/read_as_of_iterator.go
Outdated
| // (i.e. asOf, if set) and iterate to the next valid key at or below the caller's | ||
| // key that also obeys the iterator's constraints. | ||
| maxTime := f.asOf | ||
| if f.asOf.IsEmpty() { |
There was a problem hiding this comment.
nit: this check isn't necessary, since an empty timestamp sorts before all other timestamps in MVCC keys -- passing f.asOf will do the right thing.
There was a problem hiding this comment.
huh, this is kinda subtle. When you pass a key with an empty timestamp to the underlying iterator's SeekGE, you're right, it will seek to the latest version of that key. So yes, we can remove this check, but only because we call the internal seek before doing any key comparisons.
We can't, for example, remove this isEmpty() check in advance because an empty hlcTimestamp.walltime defaults to 0.
} else if !f.asOf.IsEmpty() && f.asOf.Less(f.iter.UnsafeKey().Timestamp) {
There was a problem hiding this comment.
Yeah, MVCCKey and Timestamp order timestamps differently. The former orders them in descending order but with 0 first (which means that 3 is "less than" or "ordered before" 2, but 0 is before 3, i.e. 0,3,2,1), while Timestamp uses regular ascending order (0,1,2,3...). I suppose you get used to it, but definitely subtle and a bit of a footgun.
|
PS: remember to squash the commits before merging. |
Previously while ingesting SSTs in the restore processor, `processRestoreSpanEntry` would manually skip delete tombstones and keys with timestamps later than the AS OF SYSTEM TIME timestamp provided in the RESTORE command. This PR wraps the restore processor's iterator with a new readAsOfIterator which abstracts this logic away. The readAsOfIterator wraps a SimpleMVCCIterator ond only surfaces the latest valid key of a given MVCC key that is also below the iterator's AOST timestamp, if set. Further, the iterator does not surface delete tombstones, nor any MVCC keys shadowed by delete tombstones below the AOST timestamp, if set. Fixes cockroachdb#77276 Release note: None
msbutler
force-pushed
the
butler_multi_iter_asof_wrapper
branch
from
8eac3fc to
cc71281
Compare
msbutler
changed the title
msbutler
changed the title
|
TFTR!!! bors r = erikgrinaker |
|
bors r=erikgrinaker |
|
Build succeeded: |
Previously while ingesting SSTs in the restore processor,
processRestoreSpanEntrywould manually skip delete tombstones and keys withtimestamps later than the AS OF SYSTEM TIME timestamp provided in the RESTORE
command. This PR wraps the restore processor's iterator with a new
readAsOfIterator which abstracts this logic away.
The readAsOfIterator wraps a SimpleMVCCIterator
ond only surfaces the latest valid key of a given MVCC key that is also below
the iterator's AOST timestamp, if set. Further, the iterator does not surface
delete tombstones, nor any MVCC keys shadowed by delete tombstones below the
AOST timestamp, if set.
Fixes #77276
Release note: None