engineccl/mvcc: work around time-bound iterator bug #32909
Conversation
|
This change is |
|
@benesch still figuring out how to write tests for this. first wanted to check that this is even fixing it at the right level. thoughts? |
|
Yeah, this looks like the right level! Are you planning to backport this to 2.1? |
|
Yup! |
|
👍 sounds good. And then we can hopefully rip this out if we come up with a more clever solution within RocksDB. |
|
Cleaned it up a bit and added some tests. Should be ready to go. Give this one an extra thorough look if you can. |
There was a problem hiding this comment.
I reviewed this closely, but if you want extra scrutiny perhaps @bdarnell should take a look too?
Reviewed 2 of 2 files at r1.
Reviewable status:complete! 1 of 0 LGTMs obtained
pkg/ccl/storageccl/engineccl/mvcc.go, line 54 at r1 (raw file):
iter engine.Iterator // fields used for a workaround for a bug in the time-bound iterator
// ... time-bound iterator (#28358)
pkg/ccl/storageccl/engineccl/mvcc_test.go, line 543 at r1 (raw file):
// kA -> (intent) [NB this overwrites the intent deletion] // kA:3 -> vA3 // kA _> vA2
Should this be kA:2 -> vA2?
pkg/ccl/storageccl/engineccl/mvcc_test.go, line 554 at r1 (raw file):
require.NoError(t, engine.MVCCPut(ctx, db, nil, kA, ts2, vA2, nil)) require.NoError(t, engine.MVCCPut(ctx, db, nil, kA, txnA3.Timestamp, vA3, txnA3)) require.NoError(t, db.Flush())
❤️ testify
pkg/ccl/storageccl/engineccl/mvcc_test.go, line 593 at r1 (raw file):
// Sanity check that we see the still unresolved intent for kC ts1. _, err = slurpKVs(db, kC, ts0, ts1) require.EqualError(t, err, `conflicting intents on "kC"`)
Can you also sanity check that you see the unresolved intent if you slurpKVs(db, kC, ts2, ts3) (i.e., scanning at a higher timestamp than the intent is written)? Just to lock in the behavior until #32421 goes back in.
|
Good idea. @bdarnell would you mind taking a look at this as well? |
There was a problem hiding this comment.
Reviewed 1 of 2 files at r1.
Reviewable status:
There was a problem hiding this comment.
Reviewable status:
pkg/ccl/storageccl/engineccl/mvcc_test.go, line 593 at r1 (raw file):
Previously, benesch (Nikhil Benesch) wrote…
Can you also sanity check that you see the unresolved intent if you
slurpKVs(db, kC, ts2, ts3)(i.e., scanning at a higher timestamp than the intent is written)? Just to lock in the behavior until #32421 goes back in.
Never mind. I'm totally mistaken on this point. Ignore this suggesiton.
For reasons described in cockroachdb#28358, a time-bound iterator will sometimes see an unresolved intent where there is none. A normal iterator doesn't have this problem, so we work around it in MVCCIncrementalIterator by double checking all non-value keys. If the normal iterator has a different value for the key, it's used instead. If the normal iterator doesn't have that key, it's skipped. This fixes both changefeeds and incremental backup. Closes cockroachdb#32104 Closes cockroachdb#32799 Release note (bug fix): `CHANGEFEED`s and incremental `BACKUP`s no longer indefinitely hang under an infrequent condition.
danhhz
changed the title
There was a problem hiding this comment.
Thanks for the reviews!
bors r=bdarnell,benesch
Reviewable status:
pkg/ccl/storageccl/engineccl/mvcc.go, line 54 at r1 (raw file):
Previously, benesch (Nikhil Benesch) wrote…
// ... time-bound iterator (#28358)
Done.
pkg/ccl/storageccl/engineccl/mvcc_test.go, line 543 at r1 (raw file):
Previously, benesch (Nikhil Benesch) wrote…
Should this be
kA:2 -> vA2?
Done.
pkg/ccl/storageccl/engineccl/mvcc_test.go, line 554 at r1 (raw file):
Previously, benesch (Nikhil Benesch) wrote…
❤️ testify
🙌
32909: engineccl/mvcc: work around time-bound iterator bug r=bdarnell,benesch a=danhhz For reasons described in #28358, a time-bound iterator will sometimes see an unresolved intent where there is none. A normal iterator doesn't have this problem, so we work around it in MVCCIncrementalIterator by double checking all non-value keys. If the normal iterator has a different value for the key, it's used instead. If the normal iterator doesn't have that key, it's skipped. This fixes both changefeeds and incremental backup. Closes #32104 Closes #32799 Release note (bug fix): `CHANGEFEED`s and incremental `BACKUP`s no longer indefinitely hang under an infrequent condition. Co-authored-by: Daniel Harrison <[email protected]>
Build succeeded |
For reasons described in #28358, a time-bound iterator will sometimes
see an unresolved intent where there is none. A normal iterator doesn't
have this problem, so we work around it in MVCCIncrementalIterator by
double checking all non-value keys. If the normal iterator has a
different value for the key, it's used instead. If the normal iterator
doesn't have that key, it's skipped.
This fixes both changefeeds and incremental backup.
Closes #32104
Closes #32799
Release note (bug fix):
CHANGEFEEDs and incrementalBACKUPs nolonger indefinitely hang under an infrequent condition.