storage/engine: add pebbleIterator support for time-bound iterators #41854
Conversation
|
This change is |
petermattis
force-pushed
the
pmattis/pebble-time-bound
branch
from
42e46c1
to
508f108
Compare
Define the Pebble table property collectors in `storage/engine` so they can be used for more than just writing sstables. Release note: None
Fixes cockroachdb#41743 Release note: None
petermattis
force-pushed
the
pmattis/pebble-time-bound
branch
from
508f108
to
1fd5533
Compare
There was a problem hiding this comment.
Doing string comparisons on big endian encoded timestamps seems a bit unintuitive at first but I assume it's worth it for avoiding two extra timestamp decodes for every table.
Reviewed 2 of 2 files at r1, 6 of 6 files at r3.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @ajkr and @sumeerbhola)
There was a problem hiding this comment.
TFTR!
Doing string comparisons on big endian encoded timestamps seems a bit unintuitive at first but I assume it's worth it for avoiding two extra timestamp decodes for every table.
I was confused about this as well initially, but decided to stick with this approach as it mirrors what we do on the C++ side.
Reviewable status:
There was a problem hiding this comment.
Reviewable status:
pkg/storage/engine/pebble.go, line 97 at r3 (raw file):
func (t *pebbleTimeBoundPropCollector) Add(key pebble.InternalKey, value []byte) error { _, ts, ok := enginepb.SplitMVCCKey(key.UserKey)
are we not doing the write intent metadata handling that is done by TimeBoundTblPropCollector? I remember you had said it was buggy but I can't reconstruct the reason. We won't accurately account for deletions of the metadata so it won't be as tight as it could be but it still seems tighter than using the 0 timestamp. What am I missing?
There was a problem hiding this comment.
Reviewable status:
pkg/storage/engine/pebble.go, line 97 at r3 (raw file):
Previously, sumeerbhola wrote…
are we not doing the write intent metadata handling that is done by TimeBoundTblPropCollector? I remember you had said it was buggy but I can't reconstruct the reason. We won't accurately account for deletions of the metadata so it won't be as tight as it could be but it still seems tighter than using the 0 timestamp. What am I missing?
Good catch. Tat seems to be missing. This was all just code movement in this PR and I didn't check that this property collector was identical to the C++ one. I'll file an issue.
Fixes #41743
Release note: None