-
Notifications
You must be signed in to change notification settings - Fork 472
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
*: simplify pointCollapsingIter, use vanilla iters for foreign SSTs #2776
Conversation
Enjoy the code deletions, y'all |
Previously we were using pointCollapsingIters to collapse obsolete points in foreign sstables. This was necessary because of snapshots that were open at the time those sstables were written, however they really blew up code complexity and performance. This change transitions to using vanilla sstable iterators for foreign sstables with `hideObsoletePoints` set to true. We error out if the sstable format for a foreign sstable is not high enough to support the obsolete flag. Also remove all cases in the pointCollapsingIter that were not used by ScanInternal (which is the last remaining user of it). Part of cockroachdb#2756.
5f6b6c8
to
3531d8b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 5 files reviewed, all discussions resolved (waiting on @sumeerbhola)
TFTR! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 5 files reviewed, 1 unresolved discussion
table_cache.go
line 523 at r2 (raw file):
} if provider.IsForeign(objMeta) {
(tardy drive-by comment)
this could use a code comment with justification and a possible optimization. Something like:
// A foreign sst is assigned a seqnum s when it is ingested into Pebble. Only non-obsolete point keys in that sst are potentially relevant to an iterator, and these non-obsolete keys have seqnum s, and are only relevant if opts.snapshotForHideObsoletePoints > s. So we can always set hideObsoletePoints=true
. Additionally, we could optimize by returning emptyIter
when opts.snapshotForHideObsoletePoints <= s.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 5 files reviewed, 1 unresolved discussion
table_cache.go
line 523 at r2 (raw file):
Previously, sumeerbhola wrote…
(tardy drive-by comment)
this could use a code comment with justification and a possible optimization. Something like:// A foreign sst is assigned a seqnum s when it is ingested into Pebble. Only non-obsolete point keys in that sst are potentially relevant to an iterator, and these non-obsolete keys have seqnum s, and are only relevant if opts.snapshotForHideObsoletePoints > s. So we can always set
hideObsoletePoints=true
. Additionally, we could optimize by returningemptyIter
when opts.snapshotForHideObsoletePoints <= s.
Ignore that optimization comment:
- I remembered that we were still doing
SeqNumL5
etc. when I saw internal/rangekey: remove ForeignSSTTransformer #2782 (review) - Once we remove
SeqNumL5
etc., the only "common" case wherepts.snapshotForHideObsoletePoints <= s
is when the iterator is being created using a (conventional)Snapshot
. But we're breaking conventional snapshots inIngestAndExcise
, so they will not be getting used. And EFOS will not typically see this case.
Previously we were using pointCollapsingIters to collapse obsolete points in foreign sstables. This was necessary because of snapshots that were open at the time those sstables were written, however they really blew up code complexity and performance. This change transitions to using vanilla sstable iterators for foreign sstables with
hideObsoletePoints
set to true. We error out if the sstable format for a foreign sstable is not high enough to support the obsolete flag.Also remove all cases in the pointCollapsingIter that were not used by ScanInternal (which is the last remaining user of it).
Part of #2756.