From d4e335572f1a1701e699ea502dd8d1c421c58c8a Mon Sep 17 00:00:00 2001 From: Jackson Owens Date: Wed, 10 Jan 2024 16:08:05 -0500 Subject: [PATCH] internal/keyspan: add error return value to FragmentIterator methods This commit refactors the keyspan.FragmentIterator interface to propagate errors from the positioning method that encounters the error, removing the `Error()` method from the interface. This makes it more difficult to accidentally ignore an error value by forgetting to check `Error()`. --- batch_test.go | 12 +- compaction.go | 30 ++- data_test.go | 33 +++- db.go | 50 +++-- error_iter.go | 19 +- flushable_test.go | 16 +- ingest.go | 76 ++++---- internal/keyspan/assert_iter.go | 41 ++-- internal/keyspan/assert_iter_test.go | 5 +- internal/keyspan/bounded.go | 77 ++++---- internal/keyspan/datadriven_test.go | 70 +++---- internal/keyspan/defragment.go | 184 ++++++++++-------- internal/keyspan/filter.go | 81 +++++--- internal/keyspan/get.go | 8 +- internal/keyspan/interleaving_iter.go | 30 +-- internal/keyspan/iter.go | 65 +++---- internal/keyspan/iter_test.go | 45 ++--- internal/keyspan/level_iter.go | 128 +++++++------ internal/keyspan/level_iter_test.go | 21 +- internal/keyspan/logging_iter.go | 58 +++--- internal/keyspan/merging_iter.go | 254 +++++++++++++------------ internal/keyspan/merging_iter_test.go | 20 +- internal/keyspan/seek.go | 20 +- internal/keyspan/seek_test.go | 5 +- internal/keyspan/testdata/logging_iter | 48 ++--- internal/keyspan/testdata/merging_iter | 24 --- internal/keyspan/transformer.go | 73 +++---- internal/keyspan/truncate_test.go | 6 +- internal/rangekey/coalesce_test.go | 37 ++-- keyspan_probe_test.go | 45 ++--- level_checker.go | 10 +- level_iter_test.go | 20 +- mem_table_test.go | 6 +- merging_iter.go | 27 ++- metamorphic/ops.go | 17 +- open.go | 10 +- replay/replay.go | 12 +- scan_internal.go | 28 ++- scan_internal_test.go | 32 ++-- sstable/block.go | 85 ++++----- sstable/prefix_replacing_iterator.go | 49 +++-- sstable/reader_test.go | 12 +- sstable/suffix_rewriter.go | 8 +- sstable/writer_rangekey_test.go | 6 +- sstable/writer_test.go | 12 +- table_stats.go | 8 +- table_stats_test.go | 6 +- tool/find.go | 16 +- tool/sstable.go | 25 ++- 49 files changed, 1066 insertions(+), 904 deletions(-) diff --git a/batch_test.go b/batch_test.go index c9778742fe..437f84d450 100644 --- a/batch_test.go +++ b/batch_test.go @@ -981,7 +981,8 @@ func TestBatchRangeOps(t *testing.T) { var buf bytes.Buffer if fragmentIter != nil { - for s := fragmentIter.First(); s != nil; s = fragmentIter.Next() { + s, err := fragmentIter.First() + for ; s != nil; s, err = fragmentIter.Next() { for i := range s.Keys { s.Keys[i].Trailer = base.MakeTrailer( s.Keys[i].SeqNum()&^base.InternalKeySeqNumBatch, @@ -990,6 +991,9 @@ func TestBatchRangeOps(t *testing.T) { } fmt.Fprintln(&buf, s) } + if err != nil { + return err.Error() + } } else { for k, v := internalIter.First(); k != nil; k, v = internalIter.Next() { k.SetSeqNum(k.SeqNum() &^ InternalKeySeqNumBatch) @@ -1184,9 +1188,13 @@ func scanInternalIter(w io.Writer, ii internalIterator) { } func scanKeyspanIterator(w io.Writer, ki keyspan.FragmentIterator) { - for s := ki.First(); s != nil; s = ki.Next() { + s, err := ki.First() + for ; s != nil; s, err = ki.Next() { fmt.Fprintln(w, s) } + if err != nil { + fmt.Fprintf(w, "err=%q", err.Error()) + } } func TestFlushableBatchBytesIterated(t *testing.T) { diff --git a/compaction.go b/compaction.go index ec0f68607f..b74ca19ce9 100644 --- a/compaction.go +++ b/compaction.go @@ -871,7 +871,7 @@ func adjustGrandparentOverlapBytesForFlush(c *compaction, flushingBytes uint64) func newFlush( opts *Options, cur *version, baseLevel int, flushing flushableList, beganAt time.Time, -) *compaction { +) (*compaction, error) { c := &compaction{ kind: compactionKindFlush, cmp: opts.Comparer.Compare, @@ -895,7 +895,7 @@ func newFlush( panic("pebble: ingestedFlushable must be flushed one at a time.") } c.kind = compactionKindIngestedFlushable - return c + return c, nil } } @@ -929,24 +929,29 @@ func newFlush( } } - updateRangeBounds := func(iter keyspan.FragmentIterator) { + updateRangeBounds := func(iter keyspan.FragmentIterator) error { // File bounds require s != nil && !s.Empty(). We only need to check for // s != nil here, as the memtable's FragmentIterator would never surface // empty spans. - if s := iter.First(); s != nil { + if s, err := iter.First(); err != nil { + return err + } else if s != nil { if key := s.SmallestKey(); !smallestSet || base.InternalCompare(c.cmp, c.smallest, key) > 0 { smallestSet = true c.smallest = key.Clone() } } - if s := iter.Last(); s != nil { + if s, err := iter.Last(); err != nil { + return err + } else if s != nil { if key := s.LargestKey(); !largestSet || base.InternalCompare(c.cmp, c.largest, key) < 0 { largestSet = true c.largest = key.Clone() } } + return nil } var flushingBytes uint64 @@ -954,10 +959,14 @@ func newFlush( f := flushing[i] updatePointBounds(f.newIter(nil)) if rangeDelIter := f.newRangeDelIter(nil); rangeDelIter != nil { - updateRangeBounds(rangeDelIter) + if err := updateRangeBounds(rangeDelIter); err != nil { + return nil, err + } } if rangeKeyIter := f.newRangeKeyIter(nil); rangeKeyIter != nil { - updateRangeBounds(rangeKeyIter) + if err := updateRangeBounds(rangeKeyIter); err != nil { + return nil, err + } } flushingBytes += f.inuseBytes() } @@ -971,7 +980,7 @@ func newFlush( } c.setupInuseKeyRanges() - return c + return c, nil } func (c *compaction) hasExtraLevelData() bool { @@ -1909,8 +1918,11 @@ func (d *DB) flush1() (bytesFlushed uint64, err error) { } } - c := newFlush(d.opts, d.mu.versions.currentVersion(), + c, err := newFlush(d.opts, d.mu.versions.currentVersion(), d.mu.versions.picker.getBaseLevel(), d.mu.mem.queue[:n], d.timeNow()) + if err != nil { + return 0, err + } d.addInProgressCompaction(c) jobID := d.mu.nextJobID diff --git a/data_test.go b/data_test.go index 4548d5015d..91c4d60a87 100644 --- a/data_test.go +++ b/data_test.go @@ -531,8 +531,9 @@ func runBuildRemoteCmd(td *datadriven.TestData, d *DB, storage remote.Storage) e } if rdi := b.newRangeDelIter(nil, math.MaxUint64); rdi != nil { - for s := rdi.First(); s != nil; s = rdi.Next() { - err := rangedel.Encode(s, func(k base.InternalKey, v []byte) error { + s, err := rdi.First() + for ; s != nil && err == nil; s, err = rdi.Next() { + err = rangedel.Encode(s, func(k base.InternalKey, v []byte) error { k.SetSeqNum(0) return w.Add(k, v) }) @@ -540,10 +541,14 @@ func runBuildRemoteCmd(td *datadriven.TestData, d *DB, storage remote.Storage) e return err } } + if err != nil { + return err + } } if rki := b.newRangeKeyIter(nil, math.MaxUint64); rki != nil { - for s := rki.First(); s != nil; s = rki.Next() { + s, err := rki.First() + for ; s != nil; s, err = rki.Next() { for _, k := range s.Keys { var err error switch k.Kind() { @@ -561,6 +566,9 @@ func runBuildRemoteCmd(td *datadriven.TestData, d *DB, storage remote.Storage) e } } } + if err != nil { + return err + } } return w.Close() @@ -617,8 +625,9 @@ func runBuildCmd(td *datadriven.TestData, d *DB, fs vfs.FS) error { } if rdi := b.newRangeDelIter(nil, math.MaxUint64); rdi != nil { - for s := rdi.First(); s != nil; s = rdi.Next() { - err := rangedel.Encode(s, func(k base.InternalKey, v []byte) error { + s, err := rdi.First() + for ; s != nil && err == nil; s, err = rdi.Next() { + err = rangedel.Encode(s, func(k base.InternalKey, v []byte) error { k.SetSeqNum(0) return w.Add(k, v) }) @@ -626,10 +635,14 @@ func runBuildCmd(td *datadriven.TestData, d *DB, fs vfs.FS) error { return err } } + if err != nil { + return err + } } if rki := b.newRangeKeyIter(nil, math.MaxUint64); rki != nil { - for s := rki.First(); s != nil; s = rki.Next() { + s, err := rki.First() + for ; s != nil; s, err = rki.Next() { for _, k := range s.Keys { var err error switch k.Kind() { @@ -647,6 +660,9 @@ func runBuildCmd(td *datadriven.TestData, d *DB, fs vfs.FS) error { } } } + if err != nil { + return err + } } return w.Close() @@ -841,8 +857,11 @@ func runDBDefineCmd(td *datadriven.TestData, opts *Options) (*DB, error) { flushable: mem, flushed: make(chan struct{}), }} - c := newFlush(d.opts, d.mu.versions.currentVersion(), + c, err := newFlush(d.opts, d.mu.versions.currentVersion(), d.mu.versions.picker.getBaseLevel(), toFlush, time.Now()) + if err != nil { + return err + } c.disableSpanElision = true // NB: define allows the test to exactly specify which keys go // into which sstables. If the test has a small target file diff --git a/db.go b/db.go index fd93408b12..0baf20dbbd 100644 --- a/db.go +++ b/db.go @@ -3090,7 +3090,10 @@ func (d *DB) checkVirtualBounds(m *fileMetadata) { pointKey, _ := pointIter.First() var rangeDel *keyspan.Span if rangeDelIter != nil { - rangeDel = rangeDelIter.First() + rangeDel, err = rangeDelIter.First() + if err != nil { + panic(err) + } } // Check that the lower bound is tight. @@ -3102,7 +3105,10 @@ func (d *DB) checkVirtualBounds(m *fileMetadata) { pointKey, _ = pointIter.Last() rangeDel = nil if rangeDelIter != nil { - rangeDel = rangeDelIter.Last() + rangeDel, err = rangeDelIter.Last() + if err != nil { + panic(err) + } } // Check that the upper bound is tight. @@ -3119,15 +3125,18 @@ func (d *DB) checkVirtualBounds(m *fileMetadata) { } if rangeDelIter != nil { - for key := rangeDelIter.First(); key != nil; key = rangeDelIter.Next() { - if d.cmp(key.SmallestKey().UserKey, m.SmallestPointKey.UserKey) < 0 { - panic(errors.Newf("pebble: virtual sstable %s point key %s is not within bounds", m.FileNum, key.SmallestKey().UserKey)) + s, err := rangeDelIter.First() + for ; s != nil; s, err = rangeDelIter.Next() { + if d.cmp(s.SmallestKey().UserKey, m.SmallestPointKey.UserKey) < 0 { + panic(errors.Newf("pebble: virtual sstable %s point key %s is not within bounds", m.FileNum, s.SmallestKey().UserKey)) } - - if d.cmp(key.LargestKey().UserKey, m.LargestPointKey.UserKey) > 0 { - panic(errors.Newf("pebble: virtual sstable %s point key %s is not within bounds", m.FileNum, key.LargestKey().UserKey)) + if d.cmp(s.LargestKey().UserKey, m.LargestPointKey.UserKey) > 0 { + panic(errors.Newf("pebble: virtual sstable %s point key %s is not within bounds", m.FileNum, s.LargestKey().UserKey)) } } + if err != nil { + panic(err) + } } } @@ -3136,28 +3145,35 @@ func (d *DB) checkVirtualBounds(m *fileMetadata) { } rangeKeyIter, err := d.tableNewRangeKeyIter(m, keyspan.SpanIterOptions{}) - defer rangeKeyIter.Close() - if err != nil { panic(errors.Wrap(err, "pebble: error creating range key iterator")) } + defer rangeKeyIter.Close() // Check that the lower bound is tight. - if d.cmp(rangeKeyIter.First().SmallestKey().UserKey, m.SmallestRangeKey.UserKey) != 0 { + if s, err := rangeKeyIter.First(); err != nil { + panic(err) + } else if d.cmp(s.SmallestKey().UserKey, m.SmallestRangeKey.UserKey) != 0 { panic(errors.Newf("pebble: virtual sstable %s lower range key bound is not tight", m.FileNum)) } // Check that upper bound is tight. - if d.cmp(rangeKeyIter.Last().LargestKey().UserKey, m.LargestRangeKey.UserKey) != 0 { + if s, err := rangeKeyIter.Last(); err != nil { + panic(err) + } else if d.cmp(s.LargestKey().UserKey, m.LargestRangeKey.UserKey) != 0 { panic(errors.Newf("pebble: virtual sstable %s upper range key bound is not tight", m.FileNum)) } - for key := rangeKeyIter.First(); key != nil; key = rangeKeyIter.Next() { - if d.cmp(key.SmallestKey().UserKey, m.SmallestRangeKey.UserKey) < 0 { - panic(errors.Newf("pebble: virtual sstable %s point key %s is not within bounds", m.FileNum, key.SmallestKey().UserKey)) + s, err := rangeKeyIter.First() + for ; s != nil; s, err = rangeKeyIter.Next() { + if d.cmp(s.SmallestKey().UserKey, m.SmallestRangeKey.UserKey) < 0 { + panic(errors.Newf("pebble: virtual sstable %s point key %s is not within bounds", m.FileNum, s.SmallestKey().UserKey)) } - if d.cmp(key.LargestKey().UserKey, m.LargestRangeKey.UserKey) > 0 { - panic(errors.Newf("pebble: virtual sstable %s point key %s is not within bounds", m.FileNum, key.LargestKey().UserKey)) + if d.cmp(s.LargestKey().UserKey, m.LargestRangeKey.UserKey) > 0 { + panic(errors.Newf("pebble: virtual sstable %s point key %s is not within bounds", m.FileNum, s.LargestKey().UserKey)) } } + if err != nil { + panic(err) + } } diff --git a/error_iter.go b/error_iter.go index 462309f2e7..30c442511a 100644 --- a/error_iter.go +++ b/error_iter.go @@ -75,13 +75,12 @@ type errorKeyspanIter struct { // errorKeyspanIter implements the keyspan.FragmentIterator interface. var _ keyspan.FragmentIterator = (*errorKeyspanIter)(nil) -func (*errorKeyspanIter) SeekGE(key []byte) *keyspan.Span { return nil } -func (*errorKeyspanIter) SeekLT(key []byte) *keyspan.Span { return nil } -func (*errorKeyspanIter) First() *keyspan.Span { return nil } -func (*errorKeyspanIter) Last() *keyspan.Span { return nil } -func (*errorKeyspanIter) Next() *keyspan.Span { return nil } -func (*errorKeyspanIter) Prev() *keyspan.Span { return nil } -func (i *errorKeyspanIter) Error() error { return i.err } -func (i *errorKeyspanIter) Close() error { return i.err } -func (*errorKeyspanIter) String() string { return "error" } -func (*errorKeyspanIter) WrapChildren(wrap keyspan.WrapFn) {} +func (i *errorKeyspanIter) SeekGE(key []byte) (*keyspan.Span, error) { return nil, i.err } +func (i *errorKeyspanIter) SeekLT(key []byte) (*keyspan.Span, error) { return nil, i.err } +func (i *errorKeyspanIter) First() (*keyspan.Span, error) { return nil, i.err } +func (i *errorKeyspanIter) Last() (*keyspan.Span, error) { return nil, i.err } +func (i *errorKeyspanIter) Next() (*keyspan.Span, error) { return nil, i.err } +func (i *errorKeyspanIter) Prev() (*keyspan.Span, error) { return nil, i.err } +func (i *errorKeyspanIter) Close() error { return i.err } +func (*errorKeyspanIter) String() string { return "error" } +func (*errorKeyspanIter) WrapChildren(wrap keyspan.WrapFn) {} diff --git a/flushable_test.go b/flushable_test.go index c5d1d9ca34..73b55e551a 100644 --- a/flushable_test.go +++ b/flushable_test.go @@ -133,22 +133,30 @@ func TestIngestedSSTFlushableAPI(t *testing.T) { iter := flushable.newRangeKeyIter(nil) var buf bytes.Buffer if iter != nil { - for span := iter.First(); span != nil; span = iter.Next() { + span, err := iter.First() + for ; span != nil; span, err = iter.Next() { buf.WriteString(span.String()) buf.WriteString("\n") } - iter.Close() + err = firstError(err, iter.Close()) + if err != nil { + fmt.Fprintf(&buf, "err=%q", err.Error()) + } } return buf.String() case "rangedelIter": iter := flushable.newRangeDelIter(nil) var buf bytes.Buffer if iter != nil { - for span := iter.First(); span != nil; span = iter.Next() { + span, err := iter.First() + for ; span != nil; span, err = iter.Next() { buf.WriteString(span.String()) buf.WriteString("\n") } - iter.Close() + err = firstError(err, iter.Close()) + if err != nil { + fmt.Fprintf(&buf, "err=%q", err.Error()) + } } return buf.String() case "readyForFlush": diff --git a/ingest.go b/ingest.go index 98ef20a47e..10513c07e1 100644 --- a/ingest.go +++ b/ingest.go @@ -326,17 +326,18 @@ func ingestLoad1( if iter != nil { defer iter.Close() var smallest InternalKey - if s := iter.First(); s != nil { + if s, err := iter.First(); err != nil { + return nil, err + } else if s != nil { key := s.SmallestKey() if err := ingestValidateKey(opts, &key); err != nil { return nil, err } smallest = key.Clone() } - if err := iter.Error(); err != nil { + if s, err := iter.Last(); err != nil { return nil, err - } - if s := iter.Last(); s != nil { + } else if s != nil { k := s.SmallestKey() if err := ingestValidateKey(opts, &k); err != nil { return nil, err @@ -355,17 +356,18 @@ func ingestLoad1( if iter != nil { defer iter.Close() var smallest InternalKey - if s := iter.First(); s != nil { + if s, err := iter.First(); err != nil { + return nil, err + } else if s != nil { key := s.SmallestKey() if err := ingestValidateKey(opts, &key); err != nil { return nil, err } smallest = key.Clone() } - if err := iter.Error(); err != nil { + if s, err := iter.Last(); err != nil { return nil, err - } - if s := iter.Last(); s != nil { + } else if s != nil { k := s.SmallestKey() if err := ingestValidateKey(opts, &k); err != nil { return nil, err @@ -375,9 +377,6 @@ func ingestLoad1( largest := s.LargestKey().Clone() meta.ExtendRangeKeyBounds(opts.Comparer.Compare, smallest, largest) } - if err := iter.Error(); err != nil { - return nil, err - } } } @@ -755,13 +754,18 @@ func overlapWithIterator( return true } - computeOverlapWithSpans := func(rIter keyspan.FragmentIterator) bool { + computeOverlapWithSpans := func(rIter keyspan.FragmentIterator) (bool, error) { // NB: The spans surfaced by the fragment iterator are non-overlapping. - span := rIter.SeekLT(keyRange.smallest.UserKey) - if span == nil { - span = rIter.Next() + span, err := rIter.SeekLT(keyRange.smallest.UserKey) + if err != nil { + return false, err + } else if span == nil { + span, err = rIter.Next() + if err != nil { + return false, err + } } - for ; span != nil; span = rIter.Next() { + for ; span != nil; span, err = rIter.Next() { if span.Empty() { continue } @@ -770,26 +774,26 @@ func overlapWithIterator( if c > 0 { // The start of the span is after the largest key in the // ingested table. - return false + return false, nil } if cmp(span.End, keyRange.smallest.UserKey) > 0 { // The end of the span is greater than the smallest in the // table. Note that the span end key is exclusive, thus ">0" // instead of ">=0". - return true + return true, nil } } - // Assume overlap if iterator errored. - if err := rIter.Error(); err != nil { - return true + if err != nil { + return false, err } - return false + return false, nil } // rkeyIter is either a range key level iter, or a range key iterator // over a single file. if rkeyIter != nil { - if computeOverlapWithSpans(rkeyIter) { + // If an error occurs, assume overlap. + if overlap, err := computeOverlapWithSpans(rkeyIter); overlap || err != nil { return true } } @@ -798,7 +802,9 @@ func overlapWithIterator( if rangeDelIter == nil || *rangeDelIter == nil { return false } - return computeOverlapWithSpans(*rangeDelIter) + overlap, err := computeOverlapWithSpans(*rangeDelIter) + // If an error occurs, assume overlap. + return overlap || err != nil } // ingestTargetLevel returns the target level for a file being ingested. @@ -1685,8 +1691,9 @@ func (d *DB) excise( var lastRangeDel []byte if rangeDelIter != nil { defer rangeDelIter.Close() - rdel := rangeDelIter.SeekLT(exciseSpan.Start) - if rdel != nil { + if rdel, err := rangeDelIter.SeekLT(exciseSpan.Start); err != nil { + return nil, err + } else if rdel != nil { lastRangeDel = append(lastRangeDel[:0], rdel.End...) if d.cmp(lastRangeDel, exciseSpan.Start) > 0 { lastRangeDel = exciseSpan.Start @@ -1712,8 +1719,9 @@ func (d *DB) excise( var lastRangeKey []byte var lastRangeKeyKind InternalKeyKind defer rangeKeyIter.Close() - rkey := rangeKeyIter.SeekLT(exciseSpan.Start) - if rkey != nil { + if rkey, err := rangeKeyIter.SeekLT(exciseSpan.Start); err != nil { + return nil, err + } else if rkey != nil { lastRangeKey = append(lastRangeKey[:0], rkey.End...) if d.cmp(lastRangeKey, exciseSpan.Start) > 0 { lastRangeKey = exciseSpan.Start @@ -1804,8 +1812,10 @@ func (d *DB) excise( // Store the max of (exciseSpan.End, rdel.Start) in firstRangeDel. This // needs to be a copy if the key is owned by the range del iter. var firstRangeDel []byte - rdel := rangeDelIter.SeekGE(exciseSpan.End) - if rdel != nil { + rdel, err := rangeDelIter.SeekGE(exciseSpan.End) + if err != nil { + return nil, err + } else if rdel != nil { firstRangeDel = append(firstRangeDel[:0], rdel.Start...) if d.cmp(firstRangeDel, exciseSpan.End) < 0 { firstRangeDel = exciseSpan.End @@ -1831,8 +1841,10 @@ func (d *DB) excise( // Store the max of (exciseSpan.End, rkey.Start) in firstRangeKey. This // needs to be a copy if the key is owned by the range key iter. var firstRangeKey []byte - rkey := rangeKeyIter.SeekGE(exciseSpan.End) - if rkey != nil { + rkey, err := rangeKeyIter.SeekGE(exciseSpan.End) + if err != nil { + return nil, err + } else if rkey != nil { firstRangeKey = append(firstRangeKey[:0], rkey.Start...) if d.cmp(firstRangeKey, exciseSpan.End) < 0 { firstRangeKey = exciseSpan.End diff --git a/internal/keyspan/assert_iter.go b/internal/keyspan/assert_iter.go index 5cd5312fad..3a56b680c1 100644 --- a/internal/keyspan/assert_iter.go +++ b/internal/keyspan/assert_iter.go @@ -109,62 +109,57 @@ func (i *assertIter) check(span *Span) { } // SeekGE implements FragmentIterator. -func (i *assertIter) SeekGE(key []byte) *Span { - span := i.iter.SeekGE(key) +func (i *assertIter) SeekGE(key []byte) (*Span, error) { + span, err := i.iter.SeekGE(key) if span != nil && i.cmp(span.End, key) <= 0 { i.panicf("incorrect SeekGE(%q) span %s", key, span) } i.check(span) - return span + return span, err } // SeekLT implements FragmentIterator. -func (i *assertIter) SeekLT(key []byte) *Span { - span := i.iter.SeekLT(key) +func (i *assertIter) SeekLT(key []byte) (*Span, error) { + span, err := i.iter.SeekLT(key) if span != nil && i.cmp(span.Start, key) >= 0 { i.panicf("incorrect SeekLT(%q) span %s", key, span) } i.check(span) - return span + return span, err } // First implements FragmentIterator. -func (i *assertIter) First() *Span { - span := i.iter.First() +func (i *assertIter) First() (*Span, error) { + span, err := i.iter.First() i.check(span) - return span + return span, err } // Last implements FragmentIterator. -func (i *assertIter) Last() *Span { - span := i.iter.Last() +func (i *assertIter) Last() (*Span, error) { + span, err := i.iter.Last() i.check(span) - return span + return span, err } // Next implements FragmentIterator. -func (i *assertIter) Next() *Span { - span := i.iter.Next() +func (i *assertIter) Next() (*Span, error) { + span, err := i.iter.Next() if span != nil && len(i.lastSpanEnd) > 0 && i.cmp(i.lastSpanEnd, span.Start) > 0 { i.panicf("Next span %s not after last span end %q", span, i.lastSpanEnd) } i.check(span) - return span + return span, err } // Prev implements FragmentIterator. -func (i *assertIter) Prev() *Span { - span := i.iter.Prev() +func (i *assertIter) Prev() (*Span, error) { + span, err := i.iter.Prev() if span != nil && len(i.lastSpanStart) > 0 && i.cmp(i.lastSpanStart, span.End) < 0 { i.panicf("Prev span %s not before last span start %q", span, i.lastSpanStart) } i.check(span) - return span -} - -// Error implements FragmentIterator. -func (i *assertIter) Error() error { - return i.iter.Error() + return span, err } // Close implements FragmentIterator. diff --git a/internal/keyspan/assert_iter_test.go b/internal/keyspan/assert_iter_test.go index 589bc82130..2bfc1be4ec 100644 --- a/internal/keyspan/assert_iter_test.go +++ b/internal/keyspan/assert_iter_test.go @@ -49,8 +49,11 @@ func TestAssertBoundsIter(t *testing.T) { res = fmt.Sprintf("%v", r) } }() - for span := iter.First(); span != nil; span = iter.Next() { + var span *Span + var err error + for span, err = iter.First(); span != nil; span, err = iter.Next() { } + require.NoError(t, err) return "OK" }() diff --git a/internal/keyspan/bounded.go b/internal/keyspan/bounded.go index 7646e0496e..3ca452ea60 100644 --- a/internal/keyspan/bounded.go +++ b/internal/keyspan/bounded.go @@ -100,37 +100,37 @@ var _ FragmentIterator = (*BoundedIter)(nil) // respect the prefix bounds. // SeekGE implements FragmentIterator. -func (i *BoundedIter) SeekGE(key []byte) *Span { - s := i.iter.SeekGE(key) - s = i.checkPrefixSpanStart(s) - s = i.checkPrefixSpanEnd(s) - return i.checkForwardBound(s) +func (i *BoundedIter) SeekGE(key []byte) (*Span, error) { + s, err := i.iter.SeekGE(key) + s, err = i.checkPrefixSpanStart(s, err) + s, err = i.checkPrefixSpanEnd(s, err) + return i.checkForwardBound(s, err) } // SeekLT implements FragmentIterator. -func (i *BoundedIter) SeekLT(key []byte) *Span { - s := i.iter.SeekLT(key) - s = i.checkPrefixSpanStart(s) - s = i.checkPrefixSpanEnd(s) - return i.checkBackwardBound(s) +func (i *BoundedIter) SeekLT(key []byte) (*Span, error) { + s, err := i.iter.SeekLT(key) + s, err = i.checkPrefixSpanStart(s, err) + s, err = i.checkPrefixSpanEnd(s, err) + return i.checkBackwardBound(s, err) } // First implements FragmentIterator. -func (i *BoundedIter) First() *Span { - s := i.iter.First() - s = i.checkPrefixSpanStart(s) - return i.checkForwardBound(s) +func (i *BoundedIter) First() (*Span, error) { + s, err := i.iter.First() + s, err = i.checkPrefixSpanStart(s, err) + return i.checkForwardBound(s, err) } // Last implements FragmentIterator. -func (i *BoundedIter) Last() *Span { - s := i.iter.Last() - s = i.checkPrefixSpanEnd(s) - return i.checkBackwardBound(s) +func (i *BoundedIter) Last() (*Span, error) { + s, err := i.iter.Last() + s, err = i.checkPrefixSpanEnd(s, err) + return i.checkBackwardBound(s, err) } // Next implements FragmentIterator. -func (i *BoundedIter) Next() *Span { +func (i *BoundedIter) Next() (*Span, error) { switch i.pos { case posAtLowerLimit: // The BoundedIter had previously returned nil, because it knew from @@ -138,14 +138,14 @@ func (i *BoundedIter) Next() *Span { // need to return the current iter span and reset i.pos to reflect that // we're no longer positioned at the limit. i.pos = posAtIterSpan - return i.iterSpan + return i.iterSpan, nil case posAtIterSpan: // If the span at the underlying iterator position extends to or beyond the // upper bound, we can avoid advancing because the next span is necessarily // out of bounds. if i.iterSpan != nil && i.upper != nil && i.cmp(i.iterSpan.End, i.upper) >= 0 { i.pos = posAtUpperLimit - return nil + return nil, nil } // Similarly, if the span extends to the next prefix and we're in prefix // iteration mode, we can avoid advancing. @@ -153,31 +153,31 @@ func (i *BoundedIter) Next() *Span { ei := i.split(i.iterSpan.End) if i.cmp(i.iterSpan.End[:ei], *i.prefix) > 0 { i.pos = posAtUpperLimit - return nil + return nil, nil } } return i.checkForwardBound(i.checkPrefixSpanStart(i.iter.Next())) case posAtUpperLimit: // Already exhausted. - return nil + return nil, nil default: panic("unreachable") } } // Prev implements FragmentIterator. -func (i *BoundedIter) Prev() *Span { +func (i *BoundedIter) Prev() (*Span, error) { switch i.pos { case posAtLowerLimit: // Already exhausted. - return nil + return nil, nil case posAtIterSpan: // If the span at the underlying iterator position extends to or beyond // the lower bound, we can avoid advancing because the previous span is // necessarily out of bounds. if i.iterSpan != nil && i.lower != nil && i.cmp(i.iterSpan.Start, i.lower) <= 0 { i.pos = posAtLowerLimit - return nil + return nil, nil } // Similarly, if the span extends to or beyond the current prefix and // we're in prefix iteration mode, we can avoid advancing. @@ -185,7 +185,7 @@ func (i *BoundedIter) Prev() *Span { si := i.split(i.iterSpan.Start) if i.cmp(i.iterSpan.Start[:si], *i.prefix) < 0 { i.pos = posAtLowerLimit - return nil + return nil, nil } } return i.checkBackwardBound(i.checkPrefixSpanEnd(i.iter.Prev())) @@ -195,17 +195,12 @@ func (i *BoundedIter) Prev() *Span { // need to return the current iter span and reset i.pos to reflect that // we're no longer positioned at the limit. i.pos = posAtIterSpan - return i.iterSpan + return i.iterSpan, nil default: panic("unreachable") } } -// Error implements FragmentIterator. -func (i *BoundedIter) Error() error { - return i.iter.Error() -} - // Close implements FragmentIterator. func (i *BoundedIter) Close() error { return i.iter.Close() @@ -216,7 +211,7 @@ func (i *BoundedIter) SetBounds(lower, upper []byte) { i.lower, i.upper = lower, upper } -func (i *BoundedIter) checkPrefixSpanStart(span *Span) *Span { +func (i *BoundedIter) checkPrefixSpanStart(span *Span, err error) (*Span, error) { // Compare to the prefix's bounds, if in prefix iteration mode. if span != nil && i.hasPrefix != nil && *i.hasPrefix { si := i.split(span.Start) @@ -225,13 +220,13 @@ func (i *BoundedIter) checkPrefixSpanStart(span *Span) *Span { span = nil } } - return span + return span, err } // checkForwardBound enforces the upper bound, returning nil if the provided // span is wholly outside the upper bound. It also updates i.pos and i.iterSpan // to reflect the new iterator position. -func (i *BoundedIter) checkForwardBound(span *Span) *Span { +func (i *BoundedIter) checkForwardBound(span *Span, err error) (*Span, error) { // Compare to the upper bound. if span != nil && i.upper != nil && i.cmp(span.Start, i.upper) >= 0 { span = nil @@ -240,22 +235,22 @@ func (i *BoundedIter) checkForwardBound(span *Span) *Span { if i.pos != posAtIterSpan { i.pos = posAtIterSpan } - return span + return span, err } -func (i *BoundedIter) checkPrefixSpanEnd(span *Span) *Span { +func (i *BoundedIter) checkPrefixSpanEnd(span *Span, err error) (*Span, error) { // Compare to the prefix's bounds, if in prefix iteration mode. if span != nil && i.hasPrefix != nil && *i.hasPrefix && i.cmp(span.End, *i.prefix) <= 0 { // This span ends before the current prefix. span = nil } - return span + return span, err } // checkBackward enforces the lower bound, returning nil if the provided span is // wholly outside the lower bound. It also updates i.pos and i.iterSpan to // reflect the new iterator position. -func (i *BoundedIter) checkBackwardBound(span *Span) *Span { +func (i *BoundedIter) checkBackwardBound(span *Span, err error) (*Span, error) { // Compare to the lower bound. if span != nil && i.lower != nil && i.cmp(span.End, i.lower) <= 0 { span = nil @@ -264,7 +259,7 @@ func (i *BoundedIter) checkBackwardBound(span *Span) *Span { if i.pos != posAtIterSpan { i.pos = posAtIterSpan } - return span + return span, err } // WrapChildren implements FragmentIterator. diff --git a/internal/keyspan/datadriven_test.go b/internal/keyspan/datadriven_test.go index a023b7bb98..55299a1970 100644 --- a/internal/keyspan/datadriven_test.go +++ b/internal/keyspan/datadriven_test.go @@ -286,7 +286,6 @@ func (s startKey) value(pctx *probeContext) any { type probeIterator struct { iter FragmentIterator - err error probe probe probeCtx probeContext } @@ -294,85 +293,73 @@ type probeIterator struct { // Assert that probeIterator implements the fragment iterator interface. var _ FragmentIterator = (*probeIterator)(nil) -func (p *probeIterator) handleOp(preProbeOp op) *Span { +func (p *probeIterator) handleOp(preProbeOp op) (*Span, error) { p.probeCtx.op = preProbeOp - if preProbeOp.Span == nil && p.iter != nil { - p.probeCtx.op.Err = p.iter.Error() - } - p.probe.probe(&p.probeCtx) - p.err = p.probeCtx.op.Err - return p.probeCtx.op.Span + return p.probeCtx.op.Span, p.probeCtx.op.Err } -func (p *probeIterator) SeekGE(key []byte) *Span { +func (p *probeIterator) SeekGE(key []byte) (*Span, error) { op := op{ Kind: OpSeekGE, SeekKey: key, } if p.iter != nil { - op.Span = p.iter.SeekGE(key) + op.Span, op.Err = p.iter.SeekGE(key) } return p.handleOp(op) } -func (p *probeIterator) SeekLT(key []byte) *Span { +func (p *probeIterator) SeekLT(key []byte) (*Span, error) { op := op{ Kind: OpSeekLT, SeekKey: key, } if p.iter != nil { - op.Span = p.iter.SeekLT(key) + op.Span, op.Err = p.iter.SeekLT(key) } return p.handleOp(op) } -func (p *probeIterator) First() *Span { +func (p *probeIterator) First() (*Span, error) { op := op{Kind: OpFirst} if p.iter != nil { - op.Span = p.iter.First() + op.Span, op.Err = p.iter.First() } return p.handleOp(op) } -func (p *probeIterator) Last() *Span { +func (p *probeIterator) Last() (*Span, error) { op := op{Kind: OpLast} if p.iter != nil { - op.Span = p.iter.Last() + op.Span, op.Err = p.iter.Last() } return p.handleOp(op) } -func (p *probeIterator) Next() *Span { +func (p *probeIterator) Next() (*Span, error) { op := op{Kind: OpNext} if p.iter != nil { - op.Span = p.iter.Next() + op.Span, op.Err = p.iter.Next() } return p.handleOp(op) } -func (p *probeIterator) Prev() *Span { +func (p *probeIterator) Prev() (*Span, error) { op := op{Kind: OpPrev} if p.iter != nil { - op.Span = p.iter.Prev() + op.Span, op.Err = p.iter.Prev() } return p.handleOp(op) } -func (p *probeIterator) Error() error { - return p.err -} - func (p *probeIterator) Close() error { op := op{Kind: OpClose} if p.iter != nil { op.Err = p.iter.Close() } - - p.probeCtx.op = op - p.probe.probe(&p.probeCtx) - p.err = p.probeCtx.op.Err - return p.err + _, err := p.handleOp(op) + return err } func (p *probeIterator) WrapChildren(wrap WrapFn) { @@ -403,34 +390,35 @@ var iterDelim = map[rune]bool{',': true, ' ': true, '(': true, ')': true, '"': t func runIterOp(w io.Writer, it FragmentIterator, op string) { fields := strings.FieldsFunc(op, func(r rune) bool { return iterDelim[r] }) var s *Span + var err error switch strings.ToLower(fields[0]) { case "first": - s = it.First() + s, err = it.First() case "last": - s = it.Last() + s, err = it.Last() case "seekge", "seek-ge": if len(fields) == 1 { panic(fmt.Sprintf("unable to parse iter op %q", op)) } - s = it.SeekGE([]byte(fields[1])) + s, err = it.SeekGE([]byte(fields[1])) case "seeklt", "seek-lt": if len(fields) == 1 { panic(fmt.Sprintf("unable to parse iter op %q", op)) } - s = it.SeekLT([]byte(fields[1])) + s, err = it.SeekLT([]byte(fields[1])) case "next": - s = it.Next() + s, err = it.Next() case "prev": - s = it.Prev() + s, err = it.Prev() default: panic(fmt.Sprintf("unrecognized iter op %q", fields[0])) } - if s == nil { + switch { + case err != nil: + fmt.Fprintf(w, " err=<%s>", err) + case s == nil: fmt.Fprint(w, "") - if err := it.Error(); err != nil { - fmt.Fprintf(w, " err=<%s>", it.Error()) - } - return + default: + fmt.Fprint(w, s) } - fmt.Fprint(w, s) } diff --git a/internal/keyspan/defragment.go b/internal/keyspan/defragment.go index 1271f0ae7a..6031006737 100644 --- a/internal/keyspan/defragment.go +++ b/internal/keyspan/defragment.go @@ -195,11 +195,6 @@ func (i *DefragmentingIter) Init( } } -// Error returns any accumulated error. -func (i *DefragmentingIter) Error() error { - return i.iter.Error() -} - // Close closes the underlying iterators. func (i *DefragmentingIter) Close() error { return i.iter.Close() @@ -208,14 +203,18 @@ func (i *DefragmentingIter) Close() error { // SeekGE moves the iterator to the first span covering a key greater than or // equal to the given key. This is equivalent to seeking to the first span with // an end key greater than the given key. -func (i *DefragmentingIter) SeekGE(key []byte) *Span { - i.iterSpan = i.iter.SeekGE(key) - if i.iterSpan == nil { +func (i *DefragmentingIter) SeekGE(key []byte) (*Span, error) { + var err error + i.iterSpan, err = i.iter.SeekGE(key) + switch { + case err != nil: + return nil, err + case i.iterSpan == nil: i.iterPos = iterPosCurr - return nil - } else if i.iterSpan.Empty() { + return nil, nil + case i.iterSpan.Empty(): i.iterPos = iterPosCurr - return i.iterSpan + return i.iterSpan, nil } // If the span starts strictly after key, we know there mustn't be an // earlier span that ends at i.iterSpan.Start, otherwise i.iter would've @@ -227,18 +226,16 @@ func (i *DefragmentingIter) SeekGE(key []byte) *Span { // The span we landed on has a Start bound ≤ key. There may be additional // fragments before this span. Defragment backward to find the start of the // defragmented span. - i.defragmentBackward() - - // Defragmenting backward may have stopped because it encountered an error. - // If so, we must not continue so that i.iter.Error() (and thus i.Error()) - // yields the error. - if i.iterSpan == nil && i.iter.Error() != nil { - return nil + if _, err := i.defragmentBackward(); err != nil { + return nil, err } - if i.iterPos == iterPosPrev { // Next once back onto the span. - i.iterSpan = i.iter.Next() + var err error + i.iterSpan, err = i.iter.Next() + if err != nil { + return nil, err + } } // Defragment the full span from its start. return i.defragmentForward() @@ -247,14 +244,18 @@ func (i *DefragmentingIter) SeekGE(key []byte) *Span { // SeekLT moves the iterator to the last span covering a key less than the // given key. This is equivalent to seeking to the last span with a start // key less than the given key. -func (i *DefragmentingIter) SeekLT(key []byte) *Span { - i.iterSpan = i.iter.SeekLT(key) - if i.iterSpan == nil { +func (i *DefragmentingIter) SeekLT(key []byte) (*Span, error) { + var err error + i.iterSpan, err = i.iter.SeekLT(key) + switch { + case err != nil: + return nil, err + case i.iterSpan == nil: i.iterPos = iterPosCurr - return nil - } else if i.iterSpan.Empty() { + return nil, nil + case i.iterSpan.Empty(): i.iterPos = iterPosCurr - return i.iterSpan + return i.iterSpan, nil } // If the span ends strictly before key, we know there mustn't be a later // span that starts at i.iterSpan.End, otherwise i.iter would've returned @@ -266,45 +267,54 @@ func (i *DefragmentingIter) SeekLT(key []byte) *Span { // The span we landed on has a End bound ≥ key. There may be additional // fragments after this span. Defragment forward to find the end of the // defragmented span. - i.defragmentForward() - - // Defragmenting forward may have stopped because it encountered an error. - // If so, we must not continue so that i.iter.Error() (and thus i.Error()) - // yields the error. - if i.iterSpan == nil && i.iter.Error() != nil { - return nil + if _, err := i.defragmentForward(); err != nil { + return nil, err } if i.iterPos == iterPosNext { // Prev once back onto the span. - i.iterSpan = i.iter.Prev() + var err error + i.iterSpan, err = i.iter.Prev() + if err != nil { + return nil, err + } } // Defragment the full span from its end. return i.defragmentBackward() } // First seeks the iterator to the first span and returns it. -func (i *DefragmentingIter) First() *Span { - i.iterSpan = i.iter.First() - if i.iterSpan == nil { +func (i *DefragmentingIter) First() (*Span, error) { + var err error + i.iterSpan, err = i.iter.First() + switch { + case err != nil: + return nil, err + case i.iterSpan == nil: i.iterPos = iterPosCurr - return nil + return nil, nil + default: + return i.defragmentForward() } - return i.defragmentForward() } // Last seeks the iterator to the last span and returns it. -func (i *DefragmentingIter) Last() *Span { - i.iterSpan = i.iter.Last() - if i.iterSpan == nil { +func (i *DefragmentingIter) Last() (*Span, error) { + var err error + i.iterSpan, err = i.iter.Last() + switch { + case err != nil: + return nil, err + case i.iterSpan == nil: i.iterPos = iterPosCurr - return nil + return nil, nil + default: + return i.defragmentBackward() } - return i.defragmentBackward() } // Next advances to the next span and returns it. -func (i *DefragmentingIter) Next() *Span { +func (i *DefragmentingIter) Next() (*Span, error) { switch i.iterPos { case iterPosPrev: // Switching directions; The iterator is currently positioned over the @@ -319,18 +329,23 @@ func (i *DefragmentingIter) Next() *Span { // // Next once to move onto y, defragment forward to land on the first z // position. - i.iterSpan = i.iter.Next() - if invariants.Enabled && i.iterSpan == nil && i.iter.Error() == nil { + var err error + i.iterSpan, err = i.iter.Next() + if err != nil { + return nil, err + } else if i.iterSpan == nil { panic("pebble: invariant violation: no next span while switching directions") } // We're now positioned on the first span that was defragmented into the // current iterator position. Skip over the rest of the current iterator // position's constitutent fragments. In the above example, this would // land on the first 'z'. - i.defragmentForward() + if _, err = i.defragmentForward(); err != nil { + return nil, err + } if i.iterSpan == nil { i.iterPos = iterPosCurr - return nil + return nil, nil } // Now that we're positioned over the first of the next set of @@ -343,16 +358,18 @@ func (i *DefragmentingIter) Next() *Span { panic("pebble: invariant violation: iterPosCurr with valid iterSpan") } - i.iterSpan = i.iter.Next() + var err error + i.iterSpan, err = i.iter.Next() if i.iterSpan == nil { - return nil + // NB: err may be nil or non-nil. + return nil, err } return i.defragmentForward() case iterPosNext: // Already at the next span. if i.iterSpan == nil { i.iterPos = iterPosCurr - return nil + return nil, nil } return i.defragmentForward() default: @@ -361,13 +378,13 @@ func (i *DefragmentingIter) Next() *Span { } // Prev steps back to the previous span and returns it. -func (i *DefragmentingIter) Prev() *Span { +func (i *DefragmentingIter) Prev() (*Span, error) { switch i.iterPos { case iterPosPrev: // Already at the previous span. if i.iterSpan == nil { i.iterPos = iterPosCurr - return nil + return nil, nil } return i.defragmentBackward() case iterPosCurr: @@ -377,9 +394,11 @@ func (i *DefragmentingIter) Prev() *Span { panic("pebble: invariant violation: iterPosCurr with valid iterSpan") } - i.iterSpan = i.iter.Prev() + var err error + i.iterSpan, err = i.iter.Prev() if i.iterSpan == nil { - return nil + // NB: err may be nil or non-nil. + return nil, err } return i.defragmentBackward() case iterPosNext: @@ -395,21 +414,26 @@ func (i *DefragmentingIter) Prev() *Span { // // Prev once to move onto y, defragment backward to land on the last x // position. - i.iterSpan = i.iter.Prev() - if invariants.Enabled && i.iterSpan == nil && i.iter.Error() == nil { + var err error + i.iterSpan, err = i.iter.Prev() + if err != nil { + return nil, err + } else if i.iterSpan == nil { panic("pebble: invariant violation: no previous span while switching directions") } // We're now positioned on the last span that was defragmented into the // current iterator position. Skip over the rest of the current iterator // position's constitutent fragments. In the above example, this would // land on the last 'x'. - i.defragmentBackward() + if _, err = i.defragmentBackward(); err != nil { + return nil, err + } // Now that we're positioned over the last of the prev set of // fragments, defragment backward. if i.iterSpan == nil { i.iterPos = iterPosCurr - return nil + return nil, nil } return i.defragmentBackward() default: @@ -427,18 +451,19 @@ func (i *DefragmentingIter) checkEqual(left, right *Span) bool { // defragmentForward defragments spans in the forward direction, starting from // i.iter's current position. The span at the current position must be non-nil, // but may be Empty(). -func (i *DefragmentingIter) defragmentForward() *Span { +func (i *DefragmentingIter) defragmentForward() (*Span, error) { if i.iterSpan.Empty() { // An empty span will never be equal to another span; see checkEqual for // why. To avoid loading non-empty range keys further ahead by calling Next, // return early. i.iterPos = iterPosCurr - return i.iterSpan + return i.iterSpan, nil } i.saveCurrent() + var err error i.iterPos = iterPosNext - i.iterSpan = i.iter.Next() + i.iterSpan, err = i.iter.Next() for i.iterSpan != nil { if !i.equal(i.curr.End, i.iterSpan.Start) { // Not a continuation. @@ -451,36 +476,32 @@ func (i *DefragmentingIter) defragmentForward() *Span { i.keyBuf = append(i.keyBuf[:0], i.iterSpan.End...) i.curr.End = i.keyBuf i.keysBuf = i.reduce(i.keysBuf, i.iterSpan.Keys) - i.iterSpan = i.iter.Next() + i.iterSpan, err = i.iter.Next() } // i.iterSpan == nil - // - // The inner iterator may return nil when it encounters an error. If there - // was an error, we don't know whether there is another span we should - // defragment or not. Return nil so that the caller knows they should check - // Error(). - if i.iter.Error() != nil { - return nil + if err != nil { + return nil, err } i.curr.Keys = i.keysBuf - return &i.curr + return &i.curr, nil } // defragmentBackward defragments spans in the backward direction, starting from // i.iter's current position. The span at the current position must be non-nil, // but may be Empty(). -func (i *DefragmentingIter) defragmentBackward() *Span { +func (i *DefragmentingIter) defragmentBackward() (*Span, error) { if i.iterSpan.Empty() { // An empty span will never be equal to another span; see checkEqual for // why. To avoid loading non-empty range keys further ahead by calling Next, // return early. i.iterPos = iterPosCurr - return i.iterSpan + return i.iterSpan, nil } i.saveCurrent() + var err error i.iterPos = iterPosPrev - i.iterSpan = i.iter.Prev() + i.iterSpan, err = i.iter.Prev() for i.iterSpan != nil { if !i.equal(i.curr.Start, i.iterSpan.End) { // Not a continuation. @@ -493,19 +514,14 @@ func (i *DefragmentingIter) defragmentBackward() *Span { i.keyBuf = append(i.keyBuf[:0], i.iterSpan.Start...) i.curr.Start = i.keyBuf i.keysBuf = i.reduce(i.keysBuf, i.iterSpan.Keys) - i.iterSpan = i.iter.Prev() + i.iterSpan, err = i.iter.Prev() } // i.iterSpan == nil - // - // The inner iterator may return nil when it encounters an error. If there - // was an error, we don't know whether there is another span we should - // defragment or not. Return nil so that the caller knows they should check - // Error(). - if i.iter.Error() != nil { - return nil + if err != nil { + return nil, err } i.curr.Keys = i.keysBuf - return &i.curr + return &i.curr, nil } func (i *DefragmentingIter) saveCurrent() { diff --git a/internal/keyspan/filter.go b/internal/keyspan/filter.go index ea60aacdaf..dc0eff5675 100644 --- a/internal/keyspan/filter.go +++ b/internal/keyspan/filter.go @@ -40,52 +40,77 @@ func Filter(iter FragmentIterator, filter FilterFunc, cmp base.Compare) Fragment } // SeekGE implements FragmentIterator. -func (i *filteringIter) SeekGE(key []byte) *Span { - span := i.filter(i.iter.SeekGE(key), +1) +func (i *filteringIter) SeekGE(key []byte) (*Span, error) { + s, err := i.iter.SeekGE(key) + if err != nil { + return nil, err + } + s, err = i.filter(s, +1) + if err != nil { + return nil, err + } // i.filter could return a span that's less than key, _if_ the filterFunc // (which has no knowledge of the seek key) mutated the span to end at a key // less than or equal to `key`. Detect this case and next/invalidate the iter. - if span != nil && i.cmp(span.End, key) <= 0 { + if s != nil && i.cmp(s.End, key) <= 0 { return i.Next() } - return span + return s, nil } // SeekLT implements FragmentIterator. -func (i *filteringIter) SeekLT(key []byte) *Span { - span := i.filter(i.iter.SeekLT(key), -1) +func (i *filteringIter) SeekLT(key []byte) (*Span, error) { + span, err := i.iter.SeekLT(key) + if err != nil { + return nil, err + } + span, err = i.filter(span, -1) + if err != nil { + return nil, err + } // i.filter could return a span that's >= key, _if_ the filterFunc (which has // no knowledge of the seek key) mutated the span to start at a key greater // than or equal to `key`. Detect this case and prev/invalidate the iter. if span != nil && i.cmp(span.Start, key) >= 0 { return i.Prev() } - return span + return span, nil } // First implements FragmentIterator. -func (i *filteringIter) First() *Span { - return i.filter(i.iter.First(), +1) +func (i *filteringIter) First() (*Span, error) { + s, err := i.iter.First() + if err != nil { + return nil, err + } + return i.filter(s, +1) } // Last implements FragmentIterator. -func (i *filteringIter) Last() *Span { - return i.filter(i.iter.Last(), -1) +func (i *filteringIter) Last() (*Span, error) { + s, err := i.iter.Last() + if err != nil { + return nil, err + } + return i.filter(s, -1) } // Next implements FragmentIterator. -func (i *filteringIter) Next() *Span { - return i.filter(i.iter.Next(), +1) +func (i *filteringIter) Next() (*Span, error) { + s, err := i.iter.Next() + if err != nil { + return nil, err + } + return i.filter(s, +1) } // Prev implements FragmentIterator. -func (i *filteringIter) Prev() *Span { - return i.filter(i.iter.Prev(), -1) -} - -// Error implements FragmentIterator. -func (i *filteringIter) Error() error { - return i.iter.Error() +func (i *filteringIter) Prev() (*Span, error) { + s, err := i.iter.Prev() + if err != nil { + return nil, err + } + return i.filter(s, -1) } // Close implements FragmentIterator. @@ -97,21 +122,23 @@ func (i *filteringIter) Close() error { // given Span. If the current Span is to be skipped, the iterator continues // iterating in the given direction until it lands on a Span that should be // returned, or the iterator becomes invalid. -func (i *filteringIter) filter(span *Span, dir int8) *Span { +func (i *filteringIter) filter(span *Span, dir int8) (*Span, error) { if i.filterFn == nil { - return span + return span, nil } - for i.Error() == nil && span != nil { + var err error + for span != nil { if keep := i.filterFn(span, &i.span); keep { - return &i.span + return &i.span, nil } if dir == +1 { - span = i.iter.Next() + span, err = i.iter.Next() } else { - span = i.iter.Prev() + span, err = i.iter.Prev() } } - return span + // NB: err may be nil or non-nil. + return span, err } // WrapChildren implements FragmentIterator. diff --git a/internal/keyspan/get.go b/internal/keyspan/get.go index 962e5f5b6e..4561dc89fe 100644 --- a/internal/keyspan/get.go +++ b/internal/keyspan/get.go @@ -16,11 +16,11 @@ func Get(cmp base.Compare, iter FragmentIterator, key []byte) (*Span, error) { // NB: FragmentIterator.SeekGE moves the iterator to the first span covering // a key greater than or equal to the given key. This is equivalent to // seeking to the first span with an end key greater than the given key. - iterSpan := iter.SeekGE(key) + iterSpan, err := iter.SeekGE(key) switch { - case iterSpan == nil: - return nil, iter.Error() - case cmp(iterSpan.Start, key) > 0: + case err != nil: + return nil, err + case iterSpan != nil && cmp(iterSpan.Start, key) > 0: return nil, nil default: return iterSpan, nil diff --git a/internal/keyspan/interleaving_iter.go b/internal/keyspan/interleaving_iter.go index 0ce56e11ad..0f33bda848 100644 --- a/internal/keyspan/interleaving_iter.go +++ b/internal/keyspan/interleaving_iter.go @@ -289,7 +289,7 @@ func (i *InterleavingIter) SeekGE( if i.span != nil && i.cmp(key, i.span.End) < 0 && i.cmp(key, i.span.Start) >= 0 { // We're seeking within the existing span's bounds. We still might need // truncate the span to the iterator's bounds. - i.saveSpanForward(i.span) + i.saveSpanForward(i.span, nil) i.savedKeyspan() } else { i.keyspanSeekGE(key, nil /* prefix */) @@ -346,7 +346,7 @@ func (i *InterleavingIter) SeekPrefixGE( if ei := i.comparer.Split(i.span.End); i.cmp(prefix, i.span.End[:ei]) < 0 { // We're seeking within the existing span's bounds. We still might need // truncate the span to the iterator's bounds. - i.saveSpanForward(i.span) + i.saveSpanForward(i.span, nil) i.savedKeyspan() seekKeyspanIter = false } @@ -375,7 +375,7 @@ func (i *InterleavingIter) SeekLT( if i.span != nil && i.cmp(key, i.span.Start) > 0 && i.cmp(key, i.span.End) < 0 { // We're seeking within the existing span's bounds. We still might need // truncate the span to the iterator's bounds. - i.saveSpanBackward(i.span) + i.saveSpanBackward(i.span, nil) // The span's start key is still not guaranteed to be less than key, // because of the bounds enforcement. Consider the following example: // @@ -845,21 +845,14 @@ func (i *InterleavingIter) keyspanSeekLT(k []byte) { i.savedKeyspan() } -func (i *InterleavingIter) saveSpanForward(span *Span) { +func (i *InterleavingIter) saveSpanForward(span *Span, err error) { i.span = span + i.err = firstError(i.err, err) i.truncated = false i.truncatedSpan = Span{} if i.span == nil { - i.err = firstError(i.err, i.keyspanIter.Error()) return } - if invariants.Enabled { - if err := i.keyspanIter.Error(); err != nil { - panic(errors.WithSecondaryError( - errors.AssertionFailedf("pebble: %T keyspan iterator returned non-nil span %s while iter has error", i.keyspanIter, i.span), - err)) - } - } // Check the upper bound if we have one. if i.upper != nil && i.cmp(i.span.Start, i.upper) >= 0 { i.span = nil @@ -907,21 +900,14 @@ func (i *InterleavingIter) saveSpanForward(span *Span) { } } -func (i *InterleavingIter) saveSpanBackward(span *Span) { +func (i *InterleavingIter) saveSpanBackward(span *Span, err error) { i.span = span + i.err = firstError(i.err, err) i.truncated = false i.truncatedSpan = Span{} if i.span == nil { - i.err = firstError(i.err, i.keyspanIter.Error()) return } - if invariants.Enabled { - if err := i.keyspanIter.Error(); err != nil { - panic(errors.WithSecondaryError( - errors.AssertionFailedf("pebble: %T keyspan iterator returned non-nil span %s while iter has error", i.keyspanIter, i.span), - err)) - } - } // Check the lower bound if we have one. if i.lower != nil && i.cmp(i.span.End, i.lower) <= 0 { @@ -1029,8 +1015,6 @@ func (i *InterleavingIter) verify( panic("pebble: invariant violation: accumulated error swallowed") case i.err == nil && i.pointIter.Error() != nil: panic("pebble: invariant violation: pointIter swallowed") - case i.err == nil && i.keyspanIter.Error() != nil: - panic("pebble: invariant violation: keyspanIter error swallowed") } } return k, v diff --git a/internal/keyspan/iter.go b/internal/keyspan/iter.go index 86f6d76f14..8cc065ee01 100644 --- a/internal/keyspan/iter.go +++ b/internal/keyspan/iter.go @@ -22,22 +22,25 @@ import ( // positioned at an exhausted position in the direction of iteration. For // example, a caller than finds SeekGE(k)=nil may call Prev to move the iterator // to the last span. +// +// If an error occurs during any positioning method, the method returns a nil +// span and a non-nil error. type FragmentIterator interface { // SeekGE moves the iterator to the first span covering a key greater than // or equal to the given key. This is equivalent to seeking to the first // span with an end key greater than the given key. - SeekGE(key []byte) *Span + SeekGE(key []byte) (*Span, error) // SeekLT moves the iterator to the last span covering a key less than the // given key. This is equivalent to seeking to the last span with a start // key less than the given key. - SeekLT(key []byte) *Span + SeekLT(key []byte) (*Span, error) // First moves the iterator to the first span. - First() *Span + First() (*Span, error) // Last moves the iterator to the last span. - Last() *Span + Last() (*Span, error) // Next moves the iterator to the next span. // @@ -45,7 +48,7 @@ type FragmentIterator interface { // key/value pair due to either a prior call to SeekLT or Prev which // returned an invalid span. It is not allowed to call Next when the // previous call to SeekGE, SeekPrefixGE or Next returned an invalid span. - Next() *Span + Next() (*Span, error) // Prev moves the iterator to the previous span. // @@ -53,12 +56,7 @@ type FragmentIterator interface { // key/value pair due to either a prior call to SeekGE or Next which // returned an invalid span. It is not allowed to call Prev when the // previous call to SeekLT or Prev returned an invalid span. - Prev() *Span - - // Error returns any accumulated error. - // - // TODO(jackson): Lift errors into return values on the positioning methods. - Error() error + Prev() (*Span, error) // Close closes the iterator and returns any accumulated error. Exhausting // the iterator is not considered to be an error. It is valid to call Close @@ -116,7 +114,7 @@ func (i *Iter) Init(cmp base.Compare, spans []Span) { } // SeekGE implements FragmentIterator.SeekGE. -func (i *Iter) SeekGE(key []byte) *Span { +func (i *Iter) SeekGE(key []byte) (*Span, error) { // NB: manually inlined sort.Search is ~5% faster. // // Define f(j) = false iff the span i.spans[j] is strictly before `key` @@ -139,13 +137,13 @@ func (i *Iter) SeekGE(key []byte) *Span { // i.index == upper, f(i.index-1) == false, and f(upper) (= f(i.index)) == // true => answer is i.index. if i.index >= len(i.spans) { - return nil + return nil, nil } - return &i.spans[i.index] + return &i.spans[i.index], nil } // SeekLT implements FragmentIterator.SeekLT. -func (i *Iter) SeekLT(key []byte) *Span { +func (i *Iter) SeekLT(key []byte) (*Span, error) { // NB: manually inlined sort.Search is ~5% faster. // // Define f(-1) == false and f(n) == true. @@ -168,56 +166,51 @@ func (i *Iter) SeekLT(key []byte) *Span { // the largest whose key is < the key sought. i.index-- if i.index < 0 { - return nil + return nil, nil } - return &i.spans[i.index] + return &i.spans[i.index], nil } // First implements FragmentIterator.First. -func (i *Iter) First() *Span { +func (i *Iter) First() (*Span, error) { if len(i.spans) == 0 { - return nil + return nil, nil } i.index = 0 - return &i.spans[i.index] + return &i.spans[i.index], nil } // Last implements FragmentIterator.Last. -func (i *Iter) Last() *Span { +func (i *Iter) Last() (*Span, error) { if len(i.spans) == 0 { - return nil + return nil, nil } i.index = len(i.spans) - 1 - return &i.spans[i.index] + return &i.spans[i.index], nil } // Next implements FragmentIterator.Next. -func (i *Iter) Next() *Span { +func (i *Iter) Next() (*Span, error) { if i.index >= len(i.spans) { - return nil + return nil, nil } i.index++ if i.index >= len(i.spans) { - return nil + return nil, nil } - return &i.spans[i.index] + return &i.spans[i.index], nil } // Prev implements FragmentIterator.Prev. -func (i *Iter) Prev() *Span { +func (i *Iter) Prev() (*Span, error) { if i.index < 0 { - return nil + return nil, nil } i.index-- if i.index < 0 { - return nil + return nil, nil } - return &i.spans[i.index] -} - -// Error implements FragmentIterator.Error. -func (i *Iter) Error() error { - return nil + return &i.spans[i.index], nil } // Close implements FragmentIterator.Close. diff --git a/internal/keyspan/iter_test.go b/internal/keyspan/iter_test.go index daff12aeab..fd861d2a89 100644 --- a/internal/keyspan/iter_test.go +++ b/internal/keyspan/iter_test.go @@ -22,38 +22,40 @@ func runFragmentIteratorCmd(iter FragmentIterator, input string, extraInfo func( continue } var span *Span + var err error switch parts[0] { case "seek-ge": if len(parts) != 2 { return "seek-ge \n" } - span = iter.SeekGE([]byte(strings.TrimSpace(parts[1]))) + span, err = iter.SeekGE([]byte(strings.TrimSpace(parts[1]))) case "seek-lt": if len(parts) != 2 { return "seek-lt \n" } - span = iter.SeekLT([]byte(strings.TrimSpace(parts[1]))) + span, err = iter.SeekLT([]byte(strings.TrimSpace(parts[1]))) case "first": - span = iter.First() + span, err = iter.First() case "last": - span = iter.Last() + span, err = iter.Last() case "next": - span = iter.Next() + span, err = iter.Next() case "prev": - span = iter.Prev() + span, err = iter.Prev() default: return fmt.Sprintf("unknown op: %s", parts[0]) } - if span != nil { + switch { + case err != nil: + fmt.Fprintf(&b, "err=%v\n", err) + case span == nil: + fmt.Fprintf(&b, ".\n") + default: fmt.Fprintf(&b, "%s", span) if extraInfo != nil { fmt.Fprintf(&b, " (%s)", extraInfo()) } b.WriteByte('\n') - } else if err := iter.Error(); err != nil { - fmt.Fprintf(&b, "err=%v\n", err) - } else { - fmt.Fprintf(&b, ".\n") } } return b.String() @@ -94,7 +96,7 @@ type invalidatingIter struct { // invalidatingIter implements FragmentIterator. var _ FragmentIterator = (*invalidatingIter)(nil) -func (i *invalidatingIter) invalidate(s *Span) *Span { +func (i *invalidatingIter) invalidate(s *Span, err error) (*Span, error) { // Zero the entirety of the byte bufs and the keys slice. for j := range i.bufs { for k := range i.bufs[j] { @@ -106,7 +108,7 @@ func (i *invalidatingIter) invalidate(s *Span) *Span { i.keys[j] = Key{} } if s == nil { - return nil + return nil, err } // Copy all of the span's slices into slices owned by the invalidating iter @@ -125,7 +127,7 @@ func (i *invalidatingIter) invalidate(s *Span) *Span { }) } i.span.Keys = i.keys - return &i.span + return &i.span, err } func (i *invalidatingIter) saveBytes(b []byte) []byte { @@ -137,14 +139,13 @@ func (i *invalidatingIter) saveBytes(b []byte) []byte { return saved } -func (i *invalidatingIter) SeekGE(key []byte) *Span { return i.invalidate(i.iter.SeekGE(key)) } -func (i *invalidatingIter) SeekLT(key []byte) *Span { return i.invalidate(i.iter.SeekLT(key)) } -func (i *invalidatingIter) First() *Span { return i.invalidate(i.iter.First()) } -func (i *invalidatingIter) Last() *Span { return i.invalidate(i.iter.Last()) } -func (i *invalidatingIter) Next() *Span { return i.invalidate(i.iter.Next()) } -func (i *invalidatingIter) Prev() *Span { return i.invalidate(i.iter.Prev()) } -func (i *invalidatingIter) Close() error { return i.iter.Close() } -func (i *invalidatingIter) Error() error { return i.iter.Error() } +func (i *invalidatingIter) SeekGE(key []byte) (*Span, error) { return i.invalidate(i.iter.SeekGE(key)) } +func (i *invalidatingIter) SeekLT(key []byte) (*Span, error) { return i.invalidate(i.iter.SeekLT(key)) } +func (i *invalidatingIter) First() (*Span, error) { return i.invalidate(i.iter.First()) } +func (i *invalidatingIter) Last() (*Span, error) { return i.invalidate(i.iter.Last()) } +func (i *invalidatingIter) Next() (*Span, error) { return i.invalidate(i.iter.Next()) } +func (i *invalidatingIter) Prev() (*Span, error) { return i.invalidate(i.iter.Prev()) } +func (i *invalidatingIter) Close() error { return i.iter.Close() } func (i *invalidatingIter) WrapChildren(wrap WrapFn) { i.iter = wrap(i.iter) } diff --git a/internal/keyspan/level_iter.go b/internal/keyspan/level_iter.go index 362079bbfc..110be7c07b 100644 --- a/internal/keyspan/level_iter.go +++ b/internal/keyspan/level_iter.go @@ -7,6 +7,7 @@ package keyspan import ( "fmt" + "github.com/cockroachdb/errors" "github.com/cockroachdb/pebble/internal/base" "github.com/cockroachdb/pebble/internal/invariants" "github.com/cockroachdb/pebble/internal/manifest" @@ -194,7 +195,7 @@ func (l *LevelIter) loadFile(file *manifest.FileMetadata, dir int) loadFileRetur } // SeekGE implements keyspan.FragmentIterator. -func (l *LevelIter) SeekGE(key []byte) *Span { +func (l *LevelIter) SeekGE(key []byte) (*Span, error) { l.dir = +1 l.straddle = Span{} l.straddleDir = 0 @@ -222,8 +223,8 @@ func (l *LevelIter) SeekGE(key []byte) *Span { // cases similar to the above, while still retaining correctness. // Return a straddling key instead of loading the file. l.iterFile = f - if err := l.Close(); err != nil { - return l.verify(nil) + if l.err = l.Close(); l.err != nil { + return l.verify(nil, l.err) } l.straddleDir = +1 l.straddle = Span{ @@ -231,21 +232,23 @@ func (l *LevelIter) SeekGE(key []byte) *Span { End: f.SmallestRangeKey.UserKey, Keys: nil, } - return l.verify(&l.straddle) + return l.verify(&l.straddle, nil) } } loadFileIndicator := l.loadFile(f, +1) if loadFileIndicator == noFileLoaded { - return l.verify(nil) + return l.verify(nil, l.err) } - if span := l.iter.SeekGE(key); span != nil { - return l.verify(span) + if span, err := l.iter.SeekGE(key); err != nil { + return l.verify(nil, err) + } else if span != nil { + return l.verify(span, nil) } return l.skipEmptyFileForward() } // SeekLT implements keyspan.FragmentIterator. -func (l *LevelIter) SeekLT(key []byte) *Span { +func (l *LevelIter) SeekLT(key []byte) (*Span, error) { l.dir = -1 l.straddle = Span{} l.straddleDir = 0 @@ -273,8 +276,8 @@ func (l *LevelIter) SeekLT(key []byte) *Span { // cases similar to the above, while still retaining correctness. // Return a straddling key instead of loading the file. l.iterFile = f - if err := l.Close(); err != nil { - return l.verify(nil) + if l.err = l.Close(); l.err != nil { + return l.verify(nil, l.err) } l.straddleDir = -1 l.straddle = Span{ @@ -282,54 +285,60 @@ func (l *LevelIter) SeekLT(key []byte) *Span { End: nextFile.SmallestRangeKey.UserKey, Keys: nil, } - return l.verify(&l.straddle) + return l.verify(&l.straddle, nil) } } if l.loadFile(f, -1) == noFileLoaded { - return l.verify(nil) + return l.verify(nil, l.err) } - if span := l.iter.SeekLT(key); span != nil { - return l.verify(span) + if span, err := l.iter.SeekLT(key); err != nil { + return l.verify(nil, err) + } else if span != nil { + return l.verify(span, nil) } return l.skipEmptyFileBackward() } // First implements keyspan.FragmentIterator. -func (l *LevelIter) First() *Span { +func (l *LevelIter) First() (*Span, error) { l.dir = +1 l.straddle = Span{} l.straddleDir = 0 l.err = nil // clear cached iteration error if l.loadFile(l.files.First(), +1) == noFileLoaded { - return l.verify(nil) + return l.verify(nil, l.err) } - if span := l.iter.First(); span != nil { - return l.verify(span) + if span, err := l.iter.First(); err != nil { + return l.verify(nil, err) + } else if span != nil { + return l.verify(span, nil) } return l.skipEmptyFileForward() } // Last implements keyspan.FragmentIterator. -func (l *LevelIter) Last() *Span { +func (l *LevelIter) Last() (*Span, error) { l.dir = -1 l.straddle = Span{} l.straddleDir = 0 l.err = nil // clear cached iteration error if l.loadFile(l.files.Last(), -1) == noFileLoaded { - return l.verify(nil) + return l.verify(nil, l.err) } - if span := l.iter.Last(); span != nil { - return l.verify(span) + if span, err := l.iter.Last(); err != nil { + return l.verify(nil, err) + } else if span != nil { + return l.verify(span, nil) } return l.skipEmptyFileBackward() } // Next implements keyspan.FragmentIterator. -func (l *LevelIter) Next() *Span { +func (l *LevelIter) Next() (*Span, error) { if l.err != nil || (l.iter == nil && l.iterFile == nil && l.dir > 0) { - return l.verify(nil) + return l.verify(nil, l.err) } if l.iter == nil && l.iterFile == nil { // l.dir <= 0 @@ -338,17 +347,19 @@ func (l *LevelIter) Next() *Span { l.dir = +1 if l.iter != nil { - if span := l.iter.Next(); span != nil { - return l.verify(span) + if span, err := l.iter.Next(); err != nil { + return l.verify(nil, err) + } else if span != nil { + return l.verify(span, nil) } } return l.skipEmptyFileForward() } // Prev implements keyspan.FragmentIterator. -func (l *LevelIter) Prev() *Span { +func (l *LevelIter) Prev() (*Span, error) { if l.err != nil || (l.iter == nil && l.iterFile == nil && l.dir < 0) { - return l.verify(nil) + return l.verify(nil, l.err) } if l.iter == nil && l.iterFile == nil { // l.dir >= 0 @@ -357,14 +368,16 @@ func (l *LevelIter) Prev() *Span { l.dir = -1 if l.iter != nil { - if span := l.iter.Prev(); span != nil { - return l.verify(span) + if span, err := l.iter.Prev(); err != nil { + return nil, err + } else if span != nil { + return l.verify(span, nil) } } return l.skipEmptyFileBackward() } -func (l *LevelIter) skipEmptyFileForward() *Span { +func (l *LevelIter) skipEmptyFileForward() (*Span, error) { if l.straddleDir == 0 && l.keyType == manifest.KeyTypeRange && l.iterFile != nil && l.iter != nil { // We were at a file that had spans. Check if the next file that has @@ -373,9 +386,8 @@ func (l *LevelIter) skipEmptyFileForward() *Span { // a "straddle span" in l.straddle and return that. // // Straddle spans are not created in rangedel mode. - if err := l.Close(); err != nil { - l.err = err - return l.verify(nil) + if l.err = l.Close(); l.err != nil { + return l.verify(nil, l.err) } startKey := l.iterFile.LargestRangeKey.UserKey // Resetting l.iterFile without loading the file into l.iter is okay and @@ -383,7 +395,7 @@ func (l *LevelIter) skipEmptyFileForward() *Span { // which it should be due to the Close() call above. l.iterFile = l.files.Next() if l.iterFile == nil { - return l.verify(nil) + return l.verify(nil, nil) } endKey := l.iterFile.SmallestRangeKey.UserKey if l.cmp(startKey, endKey) < 0 { @@ -394,7 +406,7 @@ func (l *LevelIter) skipEmptyFileForward() *Span { End: endKey, } l.straddleDir = +1 - return l.verify(&l.straddle) + return l.verify(&l.straddle, nil) } } else if l.straddleDir < 0 { // We were at a straddle key, but are now changing directions. l.iterFile @@ -413,19 +425,22 @@ func (l *LevelIter) skipEmptyFileForward() *Span { fileToLoad = l.files.Next() } if l.loadFile(fileToLoad, +1) == noFileLoaded { - return l.verify(nil) + return l.verify(nil, l.err) + } + span, l.err = l.iter.First() + if l.err != nil { + return l.verify(nil, l.err) } - span = l.iter.First() // In rangedel mode, we can expect to get empty files that we'd need to // skip over, but not in range key mode. if l.keyType == manifest.KeyTypeRange { break } } - return l.verify(span) + return l.verify(span, l.err) } -func (l *LevelIter) skipEmptyFileBackward() *Span { +func (l *LevelIter) skipEmptyFileBackward() (*Span, error) { // We were at a file that had spans. Check if the previous file that has // spans is not directly adjacent to the current file i.e. there is a // gap in the span keyspace between the two files. In that case, synthesize @@ -434,9 +449,8 @@ func (l *LevelIter) skipEmptyFileBackward() *Span { // Straddle spans are not created in rangedel mode. if l.straddleDir == 0 && l.keyType == manifest.KeyTypeRange && l.iterFile != nil && l.iter != nil { - if err := l.Close(); err != nil { - l.err = err - return l.verify(nil) + if l.err = l.Close(); l.err != nil { + return l.verify(nil, l.err) } endKey := l.iterFile.SmallestRangeKey.UserKey // Resetting l.iterFile without loading the file into l.iter is okay and @@ -444,7 +458,7 @@ func (l *LevelIter) skipEmptyFileBackward() *Span { // which it should be due to the Close() call above. l.iterFile = l.files.Prev() if l.iterFile == nil { - return l.verify(nil) + return l.verify(nil, nil) } startKey := l.iterFile.LargestRangeKey.UserKey if l.cmp(startKey, endKey) < 0 { @@ -455,7 +469,7 @@ func (l *LevelIter) skipEmptyFileBackward() *Span { End: endKey, } l.straddleDir = -1 - return l.verify(&l.straddle) + return l.verify(&l.straddle, nil) } } else if l.straddleDir > 0 { // We were at a straddle key, but are now changing directions. l.iterFile @@ -472,9 +486,12 @@ func (l *LevelIter) skipEmptyFileBackward() *Span { fileToLoad = l.files.Prev() } if l.loadFile(fileToLoad, -1) == noFileLoaded { - return l.verify(nil) + return l.verify(nil, l.err) + } + span, l.err = l.iter.Last() + if l.err != nil { + return l.verify(span, l.err) } - span = l.iter.Last() // In rangedel mode, we can expect to get empty files that we'd need to // skip over, but not in range key mode as the filter on the FileMetadata // should guarantee we always get a non-empty file. @@ -482,30 +499,33 @@ func (l *LevelIter) skipEmptyFileBackward() *Span { break } } - return l.verify(span) + return l.verify(span, l.err) } // verify is invoked whenever a span is returned from an iterator positioning // method to a caller. During invariant builds, it asserts invariants to the // caller. -func (l *LevelIter) verify(s *Span) *Span { +func (l *LevelIter) verify(s *Span, err error) (*Span, error) { // NB: Do not add any logic outside the invariants.Enabled conditional to // ensure that verify is always compiled away in production builds. if invariants.Enabled { + if err != l.err { + panic(errors.AssertionFailedf("LevelIter.err (%v) != returned error (%v)", l.err, err)) + } + if err != nil && s != nil { + panic(errors.AssertionFailedf("non-nil error returned alongside non-nil span")) + } if f := l.files.Current(); f != l.iterFile { panic(fmt.Sprintf("LevelIter.files.Current (%s) and l.iterFile (%s) diverged", f, l.iterFile)) } } - return s + return s, err } // Error implements keyspan.FragmentIterator. func (l *LevelIter) Error() error { - if l.err != nil || l.iter == nil { - return l.err - } - return l.iter.Error() + return l.err } // Close implements keyspan.FragmentIterator. diff --git a/internal/keyspan/level_iter_test.go b/internal/keyspan/level_iter_test.go index d9897dc28b..cac4378b1b 100644 --- a/internal/keyspan/level_iter_test.go +++ b/internal/keyspan/level_iter_test.go @@ -320,24 +320,29 @@ func TestLevelIterEquivalence(t *testing.T) { iter1.Init(base.DefaultComparer.Compare, VisibleTransform(base.InternalKeySeqNumMax), new(MergingBuffers), fileIters...) iter2.Init(base.DefaultComparer.Compare, VisibleTransform(base.InternalKeySeqNumMax), new(MergingBuffers), levelIters...) - // Check iter1 and iter2 for equivalence. - require.Equal(t, iter1.First(), iter2.First(), "failed on test case %q", tc.name) + // Check iter1 and iter2 for equivalence. + s1, err1 := iter1.First() + s2, err2 := iter2.First() + require.NoError(t, err1) + require.NoError(t, err2) + require.Equal(t, s1, s2, "failed on test case %q", tc.name) valid := true for valid { - f1 := iter1.Next() - var f2 *Span + s1, err1 = iter1.Next() + require.NoError(t, err1) for { - f2 = iter2.Next() + s2, err2 = iter2.Next() + require.NoError(t, err2) // The level iter could produce empty spans that straddle between // files. Ignore those. - if f2 == nil || !f2.Empty() { + if s2 == nil || !s2.Empty() { break } } - require.Equal(t, f1, f2, "failed on test case %q", tc.name) - valid = f1 != nil && f2 != nil + require.Equal(t, s1, s2, "failed on test case %q", tc.name) + valid = s1 != nil && s2 != nil } } } diff --git a/internal/keyspan/logging_iter.go b/internal/keyspan/logging_iter.go index 10c1753da9..074f4f97f5 100644 --- a/internal/keyspan/logging_iter.go +++ b/internal/keyspan/logging_iter.go @@ -85,61 +85,51 @@ func (i *loggingIter) opStartf(format string, args ...any) func(results ...any) var _ FragmentIterator = (*loggingIter)(nil) // SeekGE implements FragmentIterator. -func (i *loggingIter) SeekGE(key []byte) *Span { +func (i *loggingIter) SeekGE(key []byte) (*Span, error) { opEnd := i.opStartf("SeekGE(%q)", key) - span := i.iter.SeekGE(key) - opEnd(span) - return span + span, err := i.iter.SeekGE(key) + opEnd(span, err) + return span, err } // SeekLT implements FragmentIterator. -func (i *loggingIter) SeekLT(key []byte) *Span { +func (i *loggingIter) SeekLT(key []byte) (*Span, error) { opEnd := i.opStartf("SeekLT(%q)", key) - span := i.iter.SeekLT(key) - opEnd(span) - return span + span, err := i.iter.SeekLT(key) + opEnd(span, err) + return span, err } // First implements FragmentIterator. -func (i *loggingIter) First() *Span { +func (i *loggingIter) First() (*Span, error) { opEnd := i.opStartf("First()") - span := i.iter.First() - opEnd(span) - return span + span, err := i.iter.First() + opEnd(span, err) + return span, err } // Last implements FragmentIterator. -func (i *loggingIter) Last() *Span { +func (i *loggingIter) Last() (*Span, error) { opEnd := i.opStartf("Last()") - span := i.iter.Last() - opEnd(span) - return span + span, err := i.iter.Last() + opEnd(span, err) + return span, err } // Next implements FragmentIterator. -func (i *loggingIter) Next() *Span { +func (i *loggingIter) Next() (*Span, error) { opEnd := i.opStartf("Next()") - span := i.iter.Next() - opEnd(span) - return span + span, err := i.iter.Next() + opEnd(span, err) + return span, err } // Prev implements FragmentIterator. -func (i *loggingIter) Prev() *Span { +func (i *loggingIter) Prev() (*Span, error) { opEnd := i.opStartf("Prev()") - span := i.iter.Prev() - opEnd(span) - return span -} - -// Error implements FragmentIterator. -func (i *loggingIter) Error() error { - err := i.iter.Error() - if err != nil { - opEnd := i.opStartf("Error()") - opEnd(err) - } - return err + span, err := i.iter.Prev() + opEnd(span, err) + return span, err } // Close implements FragmentIterator. diff --git a/internal/keyspan/merging_iter.go b/internal/keyspan/merging_iter.go index 1ba0aeeefc..5d2ee53afc 100644 --- a/internal/keyspan/merging_iter.go +++ b/internal/keyspan/merging_iter.go @@ -239,43 +239,55 @@ type mergingIterLevel struct { heapKey boundKey } -func (l *mergingIterLevel) next() { +func (l *mergingIterLevel) next() error { if l.heapKey.kind == boundKindFragmentStart { l.heapKey = boundKey{ kind: boundKindFragmentEnd, key: l.heapKey.span.End, span: l.heapKey.span, } - return + return nil } - if s := l.iter.Next(); s == nil { + s, err := l.iter.Next() + switch { + case err != nil: + return err + case s == nil: l.heapKey = boundKey{kind: boundKindInvalid} - } else { + return nil + default: l.heapKey = boundKey{ kind: boundKindFragmentStart, key: s.Start, span: s, } + return nil } } -func (l *mergingIterLevel) prev() { +func (l *mergingIterLevel) prev() error { if l.heapKey.kind == boundKindFragmentEnd { l.heapKey = boundKey{ kind: boundKindFragmentStart, key: l.heapKey.span.Start, span: l.heapKey.span, } - return + return nil } - if s := l.iter.Prev(); s == nil { + s, err := l.iter.Prev() + switch { + case err != nil: + return err + case s == nil: l.heapKey = boundKey{kind: boundKindInvalid} - } else { + return nil + default: l.heapKey = boundKey{ kind: boundKindFragmentEnd, key: s.End, span: s, } + return nil } } @@ -325,9 +337,7 @@ func (m *MergingIter) AddLevel(iter FragmentIterator) { // SeekGE moves the iterator to the first span covering a key greater than // or equal to the given key. This is equivalent to seeking to the first // span with an end key greater than the given key. -func (m *MergingIter) SeekGE(key []byte) *Span { - m.invalidate() // clear state about current position - +func (m *MergingIter) SeekGE(key []byte) (*Span, error) { // SeekGE(k) seeks to the first span with an end key greater than the given // key. The merged span M that we're searching for might straddle the seek // `key`. In this case, the M.Start may be a key ≤ the seek key. @@ -373,16 +383,19 @@ func (m *MergingIter) SeekGE(key []byte) *Span { // root of the max heap is a preliminary value for `M.Start`. for i := range m.levels { l := &m.levels[i] - s := l.iter.SeekLT(key) - if s == nil { + s, err := l.iter.SeekLT(key) + switch { + case err != nil: + return nil, err + case s == nil: l.heapKey = boundKey{kind: boundKindInvalid} - } else if m.cmp(s.End, key) <= 0 { + case m.cmp(s.End, key) <= 0: l.heapKey = boundKey{ kind: boundKindFragmentEnd, key: s.End, span: s, } - } else { + default: // s.End > key && s.Start < key // We need to use this span's start bound, since that's the largest // bound ≤ key. @@ -394,13 +407,13 @@ func (m *MergingIter) SeekGE(key []byte) *Span { } } m.initMaxHeap() - if m.err != nil { - return nil - } else if len(m.heap.items) == 0 { + if len(m.heap.items) == 0 { // There are no spans covering any key < `key`. There is no span that // straddles the seek key. Reorient the heap into a min heap and return // the first span we find in the forward direction. - m.switchToMinHeap() + if err := m.switchToMinHeap(); err != nil { + return nil, err + } return m.findNextFragmentSet() } @@ -431,11 +444,10 @@ func (m *MergingIter) SeekGE(key []byte) *Span { // every level, and then establish a min heap. This allows us to obtain the // smallest boundary key > `key`, which will serve as our candidate end // bound. - m.switchToMinHeap() - if m.err != nil { - return nil + if err := m.switchToMinHeap(); err != nil { + return nil, err } else if len(m.heap.items) == 0 { - return nil + return nil, nil } // Check for the case 3 described above. It's possible that when we switch @@ -450,19 +462,18 @@ func (m *MergingIter) SeekGE(key []byte) *Span { } m.end = m.heap.items[0].boundKey.key - if found, s := m.synthesizeKeys(+1); found && s != nil { - return s + if found, s, err := m.synthesizeKeys(+1); err != nil { + return nil, err + } else if found && s != nil { + return s, nil } return m.findNextFragmentSet() - } // SeekLT moves the iterator to the last span covering a key less than the // given key. This is equivalent to seeking to the last span with a start // key less than the given key. -func (m *MergingIter) SeekLT(key []byte) *Span { - m.invalidate() // clear state about current position - +func (m *MergingIter) SeekLT(key []byte) (*Span, error) { // SeekLT(k) seeks to the last span with a start key less than the given // key. The merged span M that we're searching for might straddle the seek // `key`. In this case, the M.End may be a key ≥ the seek key. @@ -508,16 +519,19 @@ func (m *MergingIter) SeekLT(key []byte) *Span { // root of the min heap is a preliminary value for `M.End`. for i := range m.levels { l := &m.levels[i] - s := l.iter.SeekGE(key) - if s == nil { + s, err := l.iter.SeekGE(key) + switch { + case err != nil: + return nil, err + case s == nil: l.heapKey = boundKey{kind: boundKindInvalid} - } else if m.cmp(s.Start, key) >= 0 { + case m.cmp(s.Start, key) >= 0: l.heapKey = boundKey{ kind: boundKindFragmentStart, key: s.Start, span: s, } - } else { + default: // s.Start < key // We need to use this span's end bound, since that's the smallest // bound > key. @@ -529,13 +543,13 @@ func (m *MergingIter) SeekLT(key []byte) *Span { } } m.initMinHeap() - if m.err != nil { - return nil - } else if len(m.heap.items) == 0 { + if len(m.heap.items) == 0 { // There are no spans covering any key ≥ `key`. There is no span that // straddles the seek key. Reorient the heap into a max heap and return // the first span we find in the reverse direction. - m.switchToMaxHeap() + if err := m.switchToMaxHeap(); err != nil { + return nil, err + } return m.findPrevFragmentSet() } @@ -566,11 +580,10 @@ func (m *MergingIter) SeekLT(key []byte) *Span { // every level, and then establish a max heap. This allows us to obtain the // largest boundary key < `key`, which will serve as our candidate start // bound. - m.switchToMaxHeap() - if m.err != nil { - return nil + if err := m.switchToMaxHeap(); err != nil { + return nil, err } else if len(m.heap.items) == 0 { - return nil + return nil, nil } // Check for the case 3 described above. It's possible that when we switch // heap directions, we discover an end boundary of some child span that is @@ -584,19 +597,24 @@ func (m *MergingIter) SeekLT(key []byte) *Span { } m.start = m.heap.items[0].boundKey.key - if found, s := m.synthesizeKeys(-1); found && s != nil { - return s + if found, s, err := m.synthesizeKeys(-1); err != nil { + return nil, err + } else if found && s != nil { + return s, nil } return m.findPrevFragmentSet() } // First seeks the iterator to the first span. -func (m *MergingIter) First() *Span { - m.invalidate() // clear state about current position +func (m *MergingIter) First() (*Span, error) { for i := range m.levels { - if s := m.levels[i].iter.First(); s == nil { + s, err := m.levels[i].iter.First() + switch { + case err != nil: + return nil, err + case s == nil: m.levels[i].heapKey = boundKey{kind: boundKindInvalid} - } else { + default: m.levels[i].heapKey = boundKey{ kind: boundKindFragmentStart, key: s.Start, @@ -609,12 +627,15 @@ func (m *MergingIter) First() *Span { } // Last seeks the iterator to the last span. -func (m *MergingIter) Last() *Span { - m.invalidate() // clear state about current position +func (m *MergingIter) Last() (*Span, error) { for i := range m.levels { - if s := m.levels[i].iter.Last(); s == nil { + s, err := m.levels[i].iter.Last() + switch { + case err != nil: + return nil, err + case s == nil: m.levels[i].heapKey = boundKey{kind: boundKindInvalid} - } else { + default: m.levels[i].heapKey = boundKey{ kind: boundKindFragmentEnd, key: s.End, @@ -627,51 +648,40 @@ func (m *MergingIter) Last() *Span { } // Next advances the iterator to the next span. -func (m *MergingIter) Next() *Span { - if m.err != nil { - return nil - } +func (m *MergingIter) Next() (*Span, error) { if m.dir == +1 && (m.end == nil || m.start == nil) { - return nil + return nil, nil } if m.dir != +1 { - m.switchToMinHeap() + if err := m.switchToMinHeap(); err != nil { + return nil, err + } } return m.findNextFragmentSet() } // Prev advances the iterator to the previous span. -func (m *MergingIter) Prev() *Span { - if m.err != nil { - return nil - } +func (m *MergingIter) Prev() (*Span, error) { if m.dir == -1 && (m.end == nil || m.start == nil) { - return nil + return nil, nil } if m.dir != -1 { - m.switchToMaxHeap() + if err := m.switchToMaxHeap(); err != nil { + return nil, err + } } return m.findPrevFragmentSet() } -// Error returns any accumulated error. -func (m *MergingIter) Error() error { - if m.heap.len() == 0 || m.err != nil { - return m.err - } - return m.levels[m.heap.items[0].index].iter.Error() -} - // Close closes the iterator, releasing all acquired resources. func (m *MergingIter) Close() error { + err := m.err for i := range m.levels { - if err := m.levels[i].iter.Close(); err != nil && m.err == nil { - m.err = err - } + err = firstError(err, m.levels[i].iter.Close()) } m.levels = nil m.heap.items = m.heap.items[:0] - return m.err + return err } // String implements fmt.Stringer. @@ -699,17 +709,12 @@ func (m *MergingIter) initHeap() { index: i, boundKey: &l.heapKey, }) - } else { - m.err = firstError(m.err, l.iter.Error()) - if m.err != nil { - return - } } } m.heap.init() } -func (m *MergingIter) switchToMinHeap() { +func (m *MergingIter) switchToMinHeap() error { // switchToMinHeap reorients the heap for forward iteration, without moving // the current MergingIter position. @@ -755,12 +760,15 @@ func (m *MergingIter) switchToMinHeap() { } for i := range m.levels { - m.levels[i].next() + if err := m.levels[i].next(); err != nil { + return err + } } m.initMinHeap() + return nil } -func (m *MergingIter) switchToMaxHeap() { +func (m *MergingIter) switchToMaxHeap() error { // switchToMaxHeap reorients the heap for reverse iteration, without moving // the current MergingIter position. @@ -807,16 +815,19 @@ func (m *MergingIter) switchToMaxHeap() { } for i := range m.levels { - m.levels[i].prev() + if err := m.levels[i].prev(); err != nil { + return err + } } m.initMaxHeap() + return nil } func (m *MergingIter) cmp(a, b []byte) int { return m.heap.cmp(a, b) } -func (m *MergingIter) findNextFragmentSet() *Span { +func (m *MergingIter) findNextFragmentSet() (*Span, error) { // Each iteration of this loop considers a new merged span between unique // user keys. An iteration may find that there exists no overlap for a given // span, (eg, if the spans [a,b), [d, e) exist within level iterators, the @@ -862,9 +873,13 @@ func (m *MergingIter) findNextFragmentSet() *Span { // L2: [c, e) // If we're positioned at L1's end(c) end boundary, we want to advance // to the first bound > c. - m.nextEntry() + if err := m.nextEntry(); err != nil { + return nil, err + } for len(m.heap.items) > 0 && m.err == nil && m.cmp(m.heapRoot(), m.start) == 0 { - m.nextEntry() + if err := m.nextEntry(); err != nil { + return nil, err + } } if len(m.heap.items) == 0 || m.err != nil { break @@ -884,16 +899,18 @@ func (m *MergingIter) findNextFragmentSet() *Span { // we elide empty spans created by the mergingIter itself that don't overlap // with any child iterator returned spans (i.e. empty spans that bridge two // distinct child-iterator-defined spans). - if found, s := m.synthesizeKeys(+1); found && s != nil { - return s + if found, s, err := m.synthesizeKeys(+1); err != nil { + return nil, err + } else if found && s != nil { + return s, nil } } // Exhausted. m.clear() - return nil + return nil, nil } -func (m *MergingIter) findPrevFragmentSet() *Span { +func (m *MergingIter) findPrevFragmentSet() (*Span, error) { // Each iteration of this loop considers a new merged span between unique // user keys. An iteration may find that there exists no overlap for a given // span, (eg, if the spans [a,b), [d, e) exist within level iterators, the @@ -938,11 +955,13 @@ func (m *MergingIter) findPrevFragmentSet() *Span { // L2: [c, e) // If we're positioned at L1's start(c) start boundary, we want to prev // to move to the first bound < c. - m.prevEntry() + m.err = m.prevEntry() for len(m.heap.items) > 0 && m.err == nil && m.cmp(m.heapRoot(), m.end) == 0 { - m.prevEntry() + m.err = m.prevEntry() } - if len(m.heap.items) == 0 || m.err != nil { + if m.err != nil { + return nil, m.err + } else if len(m.heap.items) == 0 { break } @@ -960,13 +979,15 @@ func (m *MergingIter) findPrevFragmentSet() *Span { // we elide empty spans created by the mergingIter itself that don't overlap // with any child iterator returned spans (i.e. empty spans that bridge two // distinct child-iterator-defined spans). - if found, s := m.synthesizeKeys(-1); found && s != nil { - return s + if found, s, err := m.synthesizeKeys(-1); err != nil { + return nil, err + } else if found && s != nil { + return s, nil } } // Exhausted. m.clear() - return nil + return nil, nil } func (m *MergingIter) heapRoot() []byte { @@ -986,7 +1007,7 @@ func (m *MergingIter) heapRoot() []byte { // // The boolean return value, `found`, is true if the returned span overlaps // with a span returned by a child iterator. -func (m *MergingIter) synthesizeKeys(dir int8) (bool, *Span) { +func (m *MergingIter) synthesizeKeys(dir int8) (bool, *Span, error) { if invariants.Enabled { if m.cmp(m.start, m.end) >= 0 { panic(fmt.Sprintf("pebble: invariant violation: span start ≥ end: %s >= %s", m.start, m.end)) @@ -1018,14 +1039,9 @@ func (m *MergingIter) synthesizeKeys(dir int8) (bool, *Span) { // NB: m.heap.cmp is a base.Compare, whereas m.cmp is a method on // MergingIter. if err := m.transformer.Transform(m.heap.cmp, m.span, &m.span); err != nil { - m.err = err - return false, nil + return false, nil, err } - return found, &m.span -} - -func (m *MergingIter) invalidate() { - m.err = nil + return found, &m.span, nil } func (m *MergingIter) clear() { @@ -1036,39 +1052,39 @@ func (m *MergingIter) clear() { } // nextEntry steps to the next entry. -func (m *MergingIter) nextEntry() { +func (m *MergingIter) nextEntry() error { l := &m.levels[m.heap.items[0].index] - l.next() + if err := l.next(); err != nil { + return err + } if !l.heapKey.valid() { // l.iter is exhausted. - m.err = l.iter.Error() - if m.err == nil { - m.heap.pop() - } - return + m.heap.pop() + return nil } if m.heap.len() > 1 { m.heap.fix(0) } + return nil } // prevEntry steps to the previous entry. -func (m *MergingIter) prevEntry() { +func (m *MergingIter) prevEntry() error { l := &m.levels[m.heap.items[0].index] - l.prev() + if err := l.prev(); err != nil { + return err + } if !l.heapKey.valid() { // l.iter is exhausted. - m.err = l.iter.Error() - if m.err == nil { - m.heap.pop() - } - return + m.heap.pop() + return nil } if m.heap.len() > 1 { m.heap.fix(0) } + return nil } // DebugString returns a string representing the current internal state of the diff --git a/internal/keyspan/merging_iter_test.go b/internal/keyspan/merging_iter_test.go index 18650bb409..e422f8a6ab 100644 --- a/internal/keyspan/merging_iter_test.go +++ b/internal/keyspan/merging_iter_test.go @@ -192,30 +192,34 @@ func testFragmenterEquivalenceOnce(t *testing.T, seed int64) { weight int fn func() (str string, f *Span, m *Span) } + must := func(s *Span, err error) *Span { + require.NoError(t, err) + return s + } ops := []opKind{ {weight: 2, fn: func() (string, *Span, *Span) { - return "First()", fragmenterIter.First(), mergingIter.First() + return "First()", must(fragmenterIter.First()), must(mergingIter.First()) }}, {weight: 2, fn: func() (string, *Span, *Span) { - return "Last()", fragmenterIter.Last(), mergingIter.Last() + return "Last()", must(fragmenterIter.Last()), must(mergingIter.Last()) }}, {weight: 5, fn: func() (string, *Span, *Span) { k := testkeys.Key(ks, rng.Int63n(ks.Count())) return fmt.Sprintf("SeekGE(%q)", k), - fragmenterIter.SeekGE(k), - mergingIter.SeekGE(k) + must(fragmenterIter.SeekGE(k)), + must(mergingIter.SeekGE(k)) }}, {weight: 5, fn: func() (string, *Span, *Span) { k := testkeys.Key(ks, rng.Int63n(ks.Count())) return fmt.Sprintf("SeekLT(%q)", k), - fragmenterIter.SeekLT(k), - mergingIter.SeekLT(k) + must(fragmenterIter.SeekLT(k)), + must(mergingIter.SeekLT(k)) }}, {weight: 50, fn: func() (string, *Span, *Span) { - return "Next()", fragmenterIter.Next(), mergingIter.Next() + return "Next()", must(fragmenterIter.Next()), must(mergingIter.Next()) }}, {weight: 50, fn: func() (string, *Span, *Span) { - return "Prev()", fragmenterIter.Prev(), mergingIter.Prev() + return "Prev()", must(fragmenterIter.Prev()), must(mergingIter.Prev()) }}, } var totalWeight int diff --git a/internal/keyspan/seek.go b/internal/keyspan/seek.go index 334a66220e..aef293f4d1 100644 --- a/internal/keyspan/seek.go +++ b/internal/keyspan/seek.go @@ -11,22 +11,14 @@ import "github.com/cockroachdb/pebble/internal/base" func SeekLE(cmp base.Compare, iter FragmentIterator, key []byte) (*Span, error) { // Seek to the smallest span that contains a key ≥ key. If some span // contains the key `key`, SeekGE will return it. - iterSpan := iter.SeekGE(key) - if iterSpan == nil { - if err := iter.Error(); err != nil { - return nil, err - } - // Fallthrough to Prev()-ing. - } else if cmp(key, iterSpan.Start) >= 0 { + iterSpan, err := iter.SeekGE(key) + if err != nil { + return nil, err + } + if iterSpan != nil && cmp(key, iterSpan.Start) >= 0 { return iterSpan, nil } - // No span covers exactly `key`. Step backwards to move onto the largest // span < key. - iterSpan = iter.Prev() - if iterSpan == nil { - // NB: iter.Error() may be nil or non-nil. - return iterSpan, iter.Error() - } - return iterSpan, nil + return iter.Prev() } diff --git a/internal/keyspan/seek_test.go b/internal/keyspan/seek_test.go index 49a680701d..331475c336 100644 --- a/internal/keyspan/seek_test.go +++ b/internal/keyspan/seek_test.go @@ -40,10 +40,7 @@ func TestSeek(t *testing.T) { seek := SeekLE if d.Cmd == "seek-ge" { seek = func(_ base.Compare, iter FragmentIterator, key []byte) (*Span, error) { - if s := iter.SeekGE(key); s != nil { - return s, nil - } - return nil, iter.Error() + return iter.SeekGE(key) } } diff --git a/internal/keyspan/testdata/logging_iter b/internal/keyspan/testdata/logging_iter index d581f882f8..66bcf70689 100644 --- a/internal/keyspan/testdata/logging_iter +++ b/internal/keyspan/testdata/logging_iter @@ -16,36 +16,36 @@ prev ---- *keyspan.assertIter: SeekGE("a") ├── *keyspan.Iter: SeekGE("a") - │ └── a-b:{(#2,SET) (#1,SET)} - └── a-b:{(#2,SET) (#1,SET)} + │ └── a-b:{(#2,SET) (#1,SET)} + └── a-b:{(#2,SET) (#1,SET)} *keyspan.assertIter: Next() ├── *keyspan.Iter: Next() - │ └── b-c:{(#2,SET) (#1,SET)} - └── b-c:{(#2,SET) (#1,SET)} + │ └── b-c:{(#2,SET) (#1,SET)} + └── b-c:{(#2,SET) (#1,SET)} *keyspan.assertIter: Next() ├── *keyspan.Iter: Next() - │ └── c-d:{(#2,SET) (#1,SET)} - └── c-d:{(#2,SET) (#1,SET)} + │ └── c-d:{(#2,SET) (#1,SET)} + └── c-d:{(#2,SET) (#1,SET)} *keyspan.assertIter: Prev() ├── *keyspan.Iter: Prev() - │ └── b-c:{(#2,SET) (#1,SET)} - └── b-c:{(#2,SET) (#1,SET)} + │ └── b-c:{(#2,SET) (#1,SET)} + └── b-c:{(#2,SET) (#1,SET)} *keyspan.assertIter: First() ├── *keyspan.Iter: First() - │ └── a-b:{(#2,SET) (#1,SET)} - └── a-b:{(#2,SET) (#1,SET)} + │ └── a-b:{(#2,SET) (#1,SET)} + └── a-b:{(#2,SET) (#1,SET)} *keyspan.assertIter: Next() ├── *keyspan.Iter: Next() - │ └── b-c:{(#2,SET) (#1,SET)} - └── b-c:{(#2,SET) (#1,SET)} + │ └── b-c:{(#2,SET) (#1,SET)} + └── b-c:{(#2,SET) (#1,SET)} *keyspan.assertIter: Last() ├── *keyspan.Iter: Last() - │ └── c-d:{(#2,SET) (#1,SET)} - └── c-d:{(#2,SET) (#1,SET)} + │ └── c-d:{(#2,SET) (#1,SET)} + └── c-d:{(#2,SET) (#1,SET)} *keyspan.assertIter: Prev() ├── *keyspan.Iter: Prev() - │ └── b-c:{(#2,SET) (#1,SET)} - └── b-c:{(#2,SET) (#1,SET)} + │ └── b-c:{(#2,SET) (#1,SET)} + └── b-c:{(#2,SET) (#1,SET)} *keyspan.assertIter: Close() └── *keyspan.Iter: Close() @@ -57,19 +57,19 @@ next ---- *keyspan.assertIter: SeekLT("a") ├── *keyspan.Iter: SeekLT("a") - │ └── - └── + │ └── + └── *keyspan.assertIter: SeekLT("c") ├── *keyspan.Iter: SeekLT("c") - │ └── b-c:{(#2,SET) (#1,SET)} - └── b-c:{(#2,SET) (#1,SET)} + │ └── b-c:{(#2,SET) (#1,SET)} + └── b-c:{(#2,SET) (#1,SET)} *keyspan.assertIter: Next() ├── *keyspan.Iter: Next() - │ └── c-d:{(#2,SET) (#1,SET)} - └── c-d:{(#2,SET) (#1,SET)} + │ └── c-d:{(#2,SET) (#1,SET)} + └── c-d:{(#2,SET) (#1,SET)} *keyspan.assertIter: Next() ├── *keyspan.Iter: Next() - │ └── - └── + │ └── + └── *keyspan.assertIter: Close() └── *keyspan.Iter: Close() diff --git a/internal/keyspan/testdata/merging_iter b/internal/keyspan/testdata/merging_iter index aa309e2083..06530a856c 100644 --- a/internal/keyspan/testdata/merging_iter +++ b/internal/keyspan/testdata/merging_iter @@ -368,40 +368,28 @@ seek-ge k seek-ge z ---- # a.SeekLT("a") = nil -# b.SeekLT("a") = nil err= # a.SeekLT("b") = nil -# b.SeekLT("b") = a-c:{(#3,RANGEKEYUNSET,@1)} err= # a.SeekLT("c") = nil -# b.SeekLT("c") = a-c:{(#3,RANGEKEYUNSET,@1)} err= # a.SeekLT("d") = nil -# b.SeekLT("d") = a-c:{(#3,RANGEKEYUNSET,@1)} err= # a.SeekLT("e") = nil -# b.SeekLT("e") = a-c:{(#3,RANGEKEYUNSET,@1)} err= # a.SeekLT("f") = nil -# b.SeekLT("f") = a-c:{(#3,RANGEKEYUNSET,@1)} err= # a.SeekLT("g") = nil -# b.SeekLT("g") = a-c:{(#3,RANGEKEYUNSET,@1)} err= # a.SeekLT("h") = nil -# b.SeekLT("h") = a-c:{(#3,RANGEKEYUNSET,@1)} err= # a.SeekLT("i") = nil -# b.SeekLT("i") = h-k:{(#5,RANGEKEYDEL)} err= # a.SeekLT("j") = nil -# b.SeekLT("j") = h-k:{(#5,RANGEKEYDEL)} err= # a.SeekLT("k") = nil -# b.SeekLT("k") = h-k:{(#5,RANGEKEYDEL)} err= # a.SeekLT("z") = nil -# b.SeekLT("z") = h-k:{(#5,RANGEKEYDEL)} err= # Test the same as above, but with errors injected on the second iterator. @@ -474,40 +462,28 @@ seek-lt k seek-lt z ---- # a.SeekGE("a") = nil -# b.SeekGE("a") = a-c:{(#3,RANGEKEYUNSET,@1)} err= # a.SeekGE("b") = nil -# b.SeekGE("b") = a-c:{(#3,RANGEKEYUNSET,@1)} err= # a.SeekGE("c") = nil -# b.SeekGE("c") = h-k:{(#5,RANGEKEYDEL)} err= # a.SeekGE("d") = nil -# b.SeekGE("d") = h-k:{(#5,RANGEKEYDEL)} err= # a.SeekGE("e") = nil -# b.SeekGE("e") = h-k:{(#5,RANGEKEYDEL)} err= # a.SeekGE("f") = nil -# b.SeekGE("f") = h-k:{(#5,RANGEKEYDEL)} err= # a.SeekGE("g") = nil -# b.SeekGE("g") = h-k:{(#5,RANGEKEYDEL)} err= # a.SeekGE("h") = nil -# b.SeekGE("h") = h-k:{(#5,RANGEKEYDEL)} err= # a.SeekGE("i") = nil -# b.SeekGE("i") = h-k:{(#5,RANGEKEYDEL)} err= # a.SeekGE("j") = nil -# b.SeekGE("j") = h-k:{(#5,RANGEKEYDEL)} err= # a.SeekGE("k") = nil -# b.SeekGE("k") = nil err= # a.SeekGE("z") = nil -# b.SeekGE("z") = nil err= # Test SeekLTs with errors injected on the second iterator. diff --git a/internal/keyspan/transformer.go b/internal/keyspan/transformer.go index e886c90c2f..b85d29fe00 100644 --- a/internal/keyspan/transformer.go +++ b/internal/keyspan/transformer.go @@ -62,89 +62,78 @@ type TransformerIter struct { Compare base.Compare span Span - err error } -func (t *TransformerIter) applyTransform(span *Span) *Span { +func (t *TransformerIter) applyTransform(span *Span) (*Span, error) { + if span == nil { + return nil, nil + } t.span = Span{ Start: t.span.Start[:0], End: t.span.End[:0], Keys: t.span.Keys[:0], } if err := t.Transformer.Transform(t.Compare, *span, &t.span); err != nil { - t.err = err - return nil + return nil, err } - return &t.span + return &t.span, nil } // SeekGE implements the FragmentIterator interface. -func (t *TransformerIter) SeekGE(key []byte) *Span { - span := t.FragmentIterator.SeekGE(key) - if span == nil { - return nil +func (t *TransformerIter) SeekGE(key []byte) (*Span, error) { + span, err := t.FragmentIterator.SeekGE(key) + if err != nil { + return nil, err } return t.applyTransform(span) } // SeekLT implements the FragmentIterator interface. -func (t *TransformerIter) SeekLT(key []byte) *Span { - span := t.FragmentIterator.SeekLT(key) - if span == nil { - return nil +func (t *TransformerIter) SeekLT(key []byte) (*Span, error) { + span, err := t.FragmentIterator.SeekLT(key) + if err != nil { + return nil, err } return t.applyTransform(span) } // First implements the FragmentIterator interface. -func (t *TransformerIter) First() *Span { - span := t.FragmentIterator.First() - if span == nil { - return nil +func (t *TransformerIter) First() (*Span, error) { + span, err := t.FragmentIterator.First() + if err != nil { + return nil, err } return t.applyTransform(span) } // Last implements the FragmentIterator interface. -func (t *TransformerIter) Last() *Span { - span := t.FragmentIterator.Last() - if span == nil { - return nil +func (t *TransformerIter) Last() (*Span, error) { + span, err := t.FragmentIterator.Last() + if err != nil { + return nil, err } return t.applyTransform(span) } // Next implements the FragmentIterator interface. -func (t *TransformerIter) Next() *Span { - span := t.FragmentIterator.Next() - if span == nil { - return nil +func (t *TransformerIter) Next() (*Span, error) { + span, err := t.FragmentIterator.Next() + if err != nil { + return nil, err } return t.applyTransform(span) } // Prev implements the FragmentIterator interface. -func (t *TransformerIter) Prev() *Span { - span := t.FragmentIterator.Prev() - if span == nil { - return nil +func (t *TransformerIter) Prev() (*Span, error) { + span, err := t.FragmentIterator.Prev() + if err != nil { + return nil, err } return t.applyTransform(span) } -// Error implements the FragmentIterator interface. -func (t *TransformerIter) Error() error { - if t.err != nil { - return t.err - } - return t.FragmentIterator.Error() -} - // Close implements the FragmentIterator interface. func (t *TransformerIter) Close() error { - err := t.FragmentIterator.Close() - if err != nil { - return err - } - return t.err + return t.FragmentIterator.Close() } diff --git a/internal/keyspan/truncate_test.go b/internal/keyspan/truncate_test.go index f2b2793309..6c45fee4de 100644 --- a/internal/keyspan/truncate_test.go +++ b/internal/keyspan/truncate_test.go @@ -12,6 +12,7 @@ import ( "github.com/cockroachdb/datadriven" "github.com/cockroachdb/pebble/internal/base" + "github.com/stretchr/testify/require" ) func TestTruncate(t *testing.T) { @@ -70,9 +71,12 @@ func TestTruncate(t *testing.T) { tIter := doTruncate() defer tIter.Close() var truncated []Span - for s := tIter.First(); s != nil; s = tIter.Next() { + var s *Span + var err error + for s, err = tIter.First(); s != nil; s, err = tIter.Next() { truncated = append(truncated, s.ShallowClone()) } + require.NoError(t, err) return formatAlphabeticSpans(truncated) case "truncate-and-save-iter": diff --git a/internal/rangekey/coalesce_test.go b/internal/rangekey/coalesce_test.go index 2ba63f7826..d07937b9b4 100644 --- a/internal/rangekey/coalesce_test.go +++ b/internal/rangekey/coalesce_test.go @@ -101,23 +101,24 @@ func TestIter(t *testing.T) { iterCmd = string(line[:i]) } var s *keyspan.Span + var err error switch iterCmd { case "first": - s = iter.First() + s, err = iter.First() case "last": - s = iter.Last() + s, err = iter.Last() case "next": - s = iter.Next() + s, err = iter.Next() case "prev": - s = iter.Prev() + s, err = iter.Prev() case "seek-ge": - s = iter.SeekGE([]byte(strings.TrimSpace(line[i:]))) + s, err = iter.SeekGE([]byte(strings.TrimSpace(line[i:]))) case "seek-lt": - s = iter.SeekLT([]byte(strings.TrimSpace(line[i:]))) + s, err = iter.SeekLT([]byte(strings.TrimSpace(line[i:]))) default: return fmt.Sprintf("unrecognized iter command %q", iterCmd) } - require.NoError(t, iter.Error()) + require.NoError(t, err) fmt.Fprint(&buf, s) if buf.Len() > 0 { fmt.Fprintln(&buf) @@ -341,28 +342,32 @@ var iterDelim = map[rune]bool{',': true, ' ': true, '(': true, ')': true, '"': t func runIterOp(w io.Writer, it keyspan.FragmentIterator, op string) { fields := strings.FieldsFunc(op, func(r rune) bool { return iterDelim[r] }) var s *keyspan.Span + var err error switch strings.ToLower(fields[0]) { case "first": - s = it.First() + s, err = it.First() case "last": - s = it.Last() + s, err = it.Last() case "seekge": - s = it.SeekGE([]byte(fields[1])) + s, err = it.SeekGE([]byte(fields[1])) case "seeklt": - s = it.SeekLT([]byte(fields[1])) + s, err = it.SeekLT([]byte(fields[1])) case "next": - s = it.Next() + s, err = it.Next() case "prev": - s = it.Prev() + s, err = it.Prev() default: panic(fmt.Sprintf("unrecognized iter op %q", fields[0])) } fmt.Fprintf(w, "%-10s", op) - if s == nil { + switch { + case err != nil: + fmt.Fprintf(w, "", err) + case s == nil: fmt.Fprintln(w, ".") - return + default: + fmt.Fprintln(w, s) } - fmt.Fprintln(w, s) } func BenchmarkTransform(b *testing.B) { diff --git a/keyspan_probe_test.go b/keyspan_probe_test.go index 347e90eccc..7ce39009a3 100644 --- a/keyspan_probe_test.go +++ b/keyspan_probe_test.go @@ -297,7 +297,6 @@ func (s seekKey) value(pctx *keyspanProbeContext) any { type probeKeyspanIterator struct { iter keyspan.FragmentIterator - err error probe keyspanProbe probeCtx keyspanProbeContext } @@ -305,67 +304,62 @@ type probeKeyspanIterator struct { // Assert that probeIterator implements the fragment iterator interface. var _ keyspan.FragmentIterator = (*probeKeyspanIterator)(nil) -func (p *probeKeyspanIterator) handleOp(preProbeOp keyspanOp) *keyspan.Span { +func (p *probeKeyspanIterator) handleOp(preProbeOp keyspanOp) (*keyspan.Span, error) { p.probeCtx.keyspanOp = preProbeOp - if preProbeOp.Span == nil && p.iter != nil { - p.probeCtx.keyspanOp.Err = p.iter.Error() - } - p.probe.probe(&p.probeCtx) - p.err = p.probeCtx.keyspanOp.Err - return p.probeCtx.keyspanOp.Span + return p.probeCtx.keyspanOp.Span, p.probeCtx.keyspanOp.Err } -func (p *probeKeyspanIterator) SeekGE(key []byte) *keyspan.Span { +func (p *probeKeyspanIterator) SeekGE(key []byte) (*keyspan.Span, error) { op := keyspanOp{ Kind: opSpanSeekGE, SeekKey: key, } if p.iter != nil { - op.Span = p.iter.SeekGE(key) + op.Span, op.Err = p.iter.SeekGE(key) } return p.handleOp(op) } -func (p *probeKeyspanIterator) SeekLT(key []byte) *keyspan.Span { +func (p *probeKeyspanIterator) SeekLT(key []byte) (*keyspan.Span, error) { op := keyspanOp{ Kind: opSpanSeekLT, SeekKey: key, } if p.iter != nil { - op.Span = p.iter.SeekLT(key) + op.Span, op.Err = p.iter.SeekLT(key) } return p.handleOp(op) } -func (p *probeKeyspanIterator) First() *keyspan.Span { +func (p *probeKeyspanIterator) First() (*keyspan.Span, error) { op := keyspanOp{Kind: opSpanFirst} if p.iter != nil { - op.Span = p.iter.First() + op.Span, op.Err = p.iter.First() } return p.handleOp(op) } -func (p *probeKeyspanIterator) Last() *keyspan.Span { +func (p *probeKeyspanIterator) Last() (*keyspan.Span, error) { op := keyspanOp{Kind: opSpanLast} if p.iter != nil { - op.Span = p.iter.Last() + op.Span, op.Err = p.iter.Last() } return p.handleOp(op) } -func (p *probeKeyspanIterator) Next() *keyspan.Span { +func (p *probeKeyspanIterator) Next() (*keyspan.Span, error) { op := keyspanOp{Kind: opSpanNext} if p.iter != nil { - op.Span = p.iter.Next() + op.Span, op.Err = p.iter.Next() } return p.handleOp(op) } -func (p *probeKeyspanIterator) Prev() *keyspan.Span { +func (p *probeKeyspanIterator) Prev() (*keyspan.Span, error) { op := keyspanOp{Kind: opSpanPrev} if p.iter != nil { - op.Span = p.iter.Prev() + op.Span, op.Err = p.iter.Prev() } return p.handleOp(op) } @@ -374,18 +368,11 @@ func (p *probeKeyspanIterator) WrapChildren(wrap keyspan.WrapFn) { p.iter = wrap(p.iter) } -func (p *probeKeyspanIterator) Error() error { - return p.err -} - func (p *probeKeyspanIterator) Close() error { op := keyspanOp{Kind: opSpanClose} if p.iter != nil { op.Err = p.iter.Close() } - - p.probeCtx.keyspanOp = op - p.probe.probe(&p.probeCtx) - p.err = p.probeCtx.keyspanOp.Err - return p.err + _, err := p.handleOp(op) + return err } diff --git a/level_checker.go b/level_checker.go index 97be00ee74..7da59c9e81 100644 --- a/level_checker.go +++ b/level_checker.go @@ -117,7 +117,9 @@ func (m *simpleMergingIter) positionRangeDels() { if l.rangeDelIter == nil { continue } - l.tombstone = l.rangeDelIter.SeekGE(item.key.UserKey) + t, err := l.rangeDelIter.SeekGE(item.key.UserKey) + m.err = firstError(m.err, err) + l.tombstone = t } } @@ -448,7 +450,8 @@ func addTombstonesFromIter( }() var prevTombstone keyspan.Span - for tomb := iter.First(); tomb != nil; tomb = iter.Next() { + tomb, err := iter.First() + for ; tomb != nil; tomb, err = iter.Next() { t := tomb.Visible(seqNum) if t.Empty() { continue @@ -472,6 +475,9 @@ func addTombstonesFromIter( }) } } + if err != nil { + return nil, err + } return tombstones, nil } diff --git a/level_iter_test.go b/level_iter_test.go index 2fa12560d3..0ec3d6e96a 100644 --- a/level_iter_test.go +++ b/level_iter_test.go @@ -355,24 +355,28 @@ type levelIterTestIter struct { rangeDelIter keyspan.FragmentIterator } +func must(err error) { + if err != nil { + panic(err) + } +} + func (i *levelIterTestIter) rangeDelSeek( key []byte, ikey *InternalKey, val base.LazyValue, dir int, ) (*InternalKey, base.LazyValue) { var tombstone keyspan.Span if i.rangeDelIter != nil { var t *keyspan.Span + var err error if dir < 0 { - var err error t, err = keyspan.SeekLE(i.levelIter.cmp, i.rangeDelIter, key) - // TODO(jackson): Clean this up when the FragmentIterator interface - // is refactored to return an error return value from all - // positioning methods. - if err != nil { - panic(err) - } } else { - t = i.rangeDelIter.SeekGE(key) + t, err = i.rangeDelIter.SeekGE(key) } + // TODO(jackson): Clean this up when the InternalIterator interface + // is refactored to return an error return value from all + // positioning methods. + must(err) if t != nil { tombstone = t.Visible(1000) } diff --git a/mem_table_test.go b/mem_table_test.go index ae555e5ec0..2ab5045499 100644 --- a/mem_table_test.go +++ b/mem_table_test.go @@ -367,12 +367,16 @@ func TestMemTableConcurrentDeleteRange(t *testing.T) { var count int it := m.newRangeDelIter(nil) - for s := it.SeekGE(start); s != nil; s = it.Next() { + s, err := it.SeekGE(start) + for ; s != nil; s, err = it.Next() { if m.cmp(s.Start, end) >= 0 { break } count += len(s.Keys) } + if err != nil { + return err + } if j+1 != count { return errors.Errorf("%d: expected %d tombstones, but found %d", i, j+1, count) } diff --git a/merging_iter.go b/merging_iter.go index bedb92cdd4..ce08647d88 100644 --- a/merging_iter.go +++ b/merging_iter.go @@ -373,11 +373,10 @@ func (m *mergingIter) initMinRangeDelIters(oldTopLevel int) error { if l.rangeDelIter == nil { continue } - l.tombstone = l.rangeDelIter.SeekGE(item.iterKey.UserKey) - if l.tombstone == nil { - if err := l.rangeDelIter.Error(); err != nil { - return err - } + var err error + l.tombstone, err = l.rangeDelIter.SeekGE(item.iterKey.UserKey) + if err != nil { + return err } } return nil @@ -728,11 +727,10 @@ func (m *mergingIter) isNextEntryDeleted(item *mergingIterLevel) (bool, error) { // levelIter in the future cannot contain item.iterKey). Also, it is possible that we // will encounter parts of the range delete that should be ignored -- we handle that // below. - l.tombstone = l.rangeDelIter.SeekGE(item.iterKey.UserKey) - if l.tombstone == nil { - if err := l.rangeDelIter.Error(); err != nil { - return false, err - } + var err error + l.tombstone, err = l.rangeDelIter.SeekGE(item.iterKey.UserKey) + if err != nil { + return false, err } } if l.tombstone == nil { @@ -1174,11 +1172,10 @@ func (m *mergingIter) seekGE(key []byte, level int, flags base.SeekGEFlags) erro // so we can have a sstable with bounds [c#8, i#InternalRangeDelSentinel], and the // tombstone is [b, k)#8 and the seek key is i: levelIter.SeekGE(i) will move past // this sstable since it realizes the largest key is a InternalRangeDelSentinel. - l.tombstone = rangeDelIter.SeekGE(key) - if l.tombstone == nil { - if err := rangeDelIter.Error(); err != nil { - return err - } + var err error + l.tombstone, err = rangeDelIter.SeekGE(key) + if err != nil { + return err } if l.tombstone != nil && l.tombstone.VisibleAt(m.snapshot) && l.tombstone.Contains(m.heap.cmp, key) && (l.smallestUserKey == nil || m.heap.cmp(l.smallestUserKey, key) <= 0) { diff --git a/metamorphic/ops.go b/metamorphic/ops.go index 2be5f6b714..ed4c8c64a6 100644 --- a/metamorphic/ops.go +++ b/metamorphic/ops.go @@ -687,13 +687,17 @@ func buildForIngest( if rangeDelIter != nil { // NB: The range tombstones have already been fragmented by the Batch. - for t := rangeDelIter.First(); t != nil; t = rangeDelIter.Next() { + t, err := rangeDelIter.First() + for ; t != nil; t, err = rangeDelIter.Next() { // NB: We don't have to copy the key or value since we're reading from a // batch which doesn't do prefix compression. if err := w.DeleteRange(t.Start, t.End); err != nil { return "", nil, err } } + if err != nil { + return "", nil, err + } if err := rangeDelIter.Close(); err != nil { return "", nil, err } @@ -701,7 +705,8 @@ func buildForIngest( } if rangeKeyIter != nil { - for span := rangeKeyIter.First(); span != nil; span = rangeKeyIter.Next() { + span, err := rangeKeyIter.First() + for ; span != nil; span, err = rangeKeyIter.Next() { // Coalesce the keys of this span and then zero the sequence // numbers. This is necessary in order to make the range keys within // the ingested sstable internally consistent at the sequence number @@ -728,7 +733,7 @@ func buildForIngest( return "", nil, err } } - if err := rangeKeyIter.Error(); err != nil { + if err != nil { return "", nil, err } if err := rangeKeyIter.Close(); err != nil { @@ -800,13 +805,17 @@ func (o *ingestOp) collapseBatch( if rangeDelIter != nil { // NB: The range tombstones have already been fragmented by the Batch. - for t := rangeDelIter.First(); t != nil; t = rangeDelIter.Next() { + t, err := rangeDelIter.First() + for ; t != nil; t, err = rangeDelIter.Next() { // NB: We don't have to copy the key or value since we're reading from a // batch which doesn't do prefix compression. if err := collapsed.DeleteRange(t.Start, t.End, nil); err != nil { return nil, err } } + if err != nil { + return nil, err + } if err := rangeDelIter.Close(); err != nil { return nil, err } diff --git a/open.go b/open.go index 9b7154fde7..48785e9d6a 100644 --- a/open.go +++ b/open.go @@ -812,8 +812,11 @@ func (d *DB) replayWAL( updateVE := func() error { // TODO(bananabrick): See if we can use the actual base level here, // instead of using 1. - c := newFlush(d.opts, d.mu.versions.currentVersion(), + c, err := newFlush(d.opts, d.mu.versions.currentVersion(), 1 /* base level */, toFlush, d.timeNow()) + if err != nil { + return err + } newVE, _, _, err := d.runCompaction(jobID, c) if err != nil { return errors.Wrapf(err, "running compaction during WAL replay") @@ -970,12 +973,15 @@ func (d *DB) replayWAL( // the application of ve to the manifest into chunks and is // not pretty w/o a refactor to this function and how it's // used. - c := newFlush( + c, err := newFlush( d.opts, d.mu.versions.currentVersion(), 1, /* base level */ []*flushableEntry{entry}, d.timeNow(), ) + if err != nil { + return nil, 0, err + } for _, file := range c.flushing[0].flushable.(*ingestedFlushable).files { ve.NewFiles = append(ve.NewFiles, newFileEntry{Level: 0, Meta: file.FileMetadata}) } diff --git a/replay/replay.go b/replay/replay.go index e2111ebf20..7b6011b405 100644 --- a/replay/replay.go +++ b/replay/replay.go @@ -1023,7 +1023,8 @@ func loadFlushedSSTableKeys( return err } else if iter != nil { defer iter.Close() - for s := iter.First(); s != nil; s = iter.Next() { + s, err := iter.First() + for ; s != nil; s, err = iter.Next() { if err := rangedel.Encode(s, func(k base.InternalKey, v []byte) error { var key flushedKey key.Trailer = k.Trailer @@ -1035,6 +1036,9 @@ func loadFlushedSSTableKeys( return err } } + if err != nil { + return err + } } // Load all the range keys. @@ -1042,7 +1046,8 @@ func loadFlushedSSTableKeys( return err } else if iter != nil { defer iter.Close() - for s := iter.First(); s != nil; s = iter.Next() { + s, err := iter.First() + for ; s != nil; s, err = iter.Next() { if err := rangekey.Encode(s, func(k base.InternalKey, v []byte) error { var key flushedKey key.Trailer = k.Trailer @@ -1054,6 +1059,9 @@ func loadFlushedSSTableKeys( return err } } + if err != nil { + return err + } } return nil }(); err != nil { diff --git a/scan_internal.go b/scan_internal.go index 62bb58e9f8..3fdb148e44 100644 --- a/scan_internal.go +++ b/scan_internal.go @@ -502,8 +502,9 @@ func (d *DB) truncateSharedFile( sst.SmallestPointKey.CopyFrom(*key) } if rangeDelIter != nil { - span := rangeDelIter.SeekGE(lower) - if span != nil && (len(sst.SmallestPointKey.UserKey) == 0 || base.InternalCompare(cmp, span.SmallestKey(), sst.SmallestPointKey) < 0) { + if span, err := rangeDelIter.SeekGE(lower); err != nil { + return nil, false, err + } else if span != nil && (len(sst.SmallestPointKey.UserKey) == 0 || base.InternalCompare(cmp, span.SmallestKey(), sst.SmallestPointKey) < 0) { sst.SmallestPointKey.CopyFrom(span.SmallestKey()) foundPointKey = true } @@ -516,10 +517,13 @@ func (d *DB) truncateSharedFile( sst.SmallestRangeKey.UserKey = sst.SmallestRangeKey.UserKey[:0] sst.SmallestRangeKey.Trailer = 0 if rangeKeyIter != nil { - span := rangeKeyIter.SeekGE(lower) - if span != nil { + span, err := rangeKeyIter.SeekGE(lower) + switch { + case err != nil: + return nil, false, err + case span != nil: sst.SmallestRangeKey.CopyFrom(span.SmallestKey()) - } else { + default: // There are no range keys in the span we're interested in. sst.SmallestRangeKey = InternalKey{} sst.LargestRangeKey = InternalKey{} @@ -537,8 +541,9 @@ func (d *DB) truncateSharedFile( sst.LargestPointKey.CopyFrom(*key) } if rangeDelIter != nil { - span := rangeDelIter.SeekLT(upper) - if span != nil && (len(sst.LargestPointKey.UserKey) == 0 || base.InternalCompare(cmp, span.LargestKey(), sst.LargestPointKey) > 0) { + if span, err := rangeDelIter.SeekLT(upper); err != nil { + return nil, false, err + } else if span != nil && (len(sst.LargestPointKey.UserKey) == 0 || base.InternalCompare(cmp, span.LargestKey(), sst.LargestPointKey) > 0) { sst.LargestPointKey.CopyFrom(span.LargestKey()) foundPointKey = true } @@ -551,10 +556,13 @@ func (d *DB) truncateSharedFile( sst.LargestRangeKey.UserKey = sst.LargestRangeKey.UserKey[:0] sst.LargestRangeKey.Trailer = 0 if rangeKeyIter != nil { - span := rangeKeyIter.SeekLT(upper) - if span != nil { + span, err := rangeKeyIter.SeekLT(upper) + switch { + case err != nil: + return nil, false, err + case span != nil: sst.LargestRangeKey.CopyFrom(span.LargestKey()) - } else { + default: // There are no range keys in the span we're interested in. sst.SmallestRangeKey = InternalKey{} sst.LargestRangeKey = InternalKey{} diff --git a/scan_internal_test.go b/scan_internal_test.go index 6087b2e01a..9360ab5e35 100644 --- a/scan_internal_test.go +++ b/scan_internal_test.go @@ -404,21 +404,29 @@ func TestScanInternal(t *testing.T) { file, err := d.opts.FS.Create("temp0.sst") require.NoError(t, err) w := sstable.NewWriter(objstorageprovider.NewFileWritable(file), d.opts.MakeWriterOptions(0, sstable.TableFormatPebblev4)) - for span := rangeDels.First(); span != nil; span = rangeDels.Next() { - require.NoError(t, w.DeleteRange(span.Start, span.End)) + { + span, err := rangeDels.First() + for ; span != nil; span, err = rangeDels.Next() { + require.NoError(t, w.DeleteRange(span.Start, span.End)) + } + require.NoError(t, err) + require.NoError(t, rangeDels.Close()) } - rangeDels.Close() - for span := rangeKeys.First(); span != nil; span = rangeKeys.Next() { - keys := []keyspan.Key{} - for i := range span.Keys { - keys = append(keys, span.Keys[i]) - keys[i].Trailer = base.MakeTrailer(0, keys[i].Kind()) + { + span, err := rangeKeys.First() + for ; span != nil; span, err = rangeKeys.Next() { + keys := []keyspan.Key{} + for i := range span.Keys { + keys = append(keys, span.Keys[i]) + keys[i].Trailer = base.MakeTrailer(0, keys[i].Kind()) + } + keyspan.SortKeysByTrailer(&keys) + newSpan := &keyspan.Span{Start: span.Start, End: span.End, Keys: keys} + require.NoError(t, rangekey.Encode(newSpan, w.AddRangeKey)) } - keyspan.SortKeysByTrailer(&keys) - newSpan := &keyspan.Span{Start: span.Start, End: span.End, Keys: keys} - rangekey.Encode(newSpan, w.AddRangeKey) + require.NoError(t, err) } - rangeKeys.Close() + require.NoError(t, rangeKeys.Close()) for key, val := points.First(); key != nil; key, val = points.Next() { var value []byte value, _, err = val.Value(value) diff --git a/sstable/block.go b/sstable/block.go index 6cfea806ac..c0abd4c2c8 100644 --- a/sstable/block.go +++ b/sstable/block.go @@ -1570,7 +1570,6 @@ type fragmentBlockIter struct { blockIter blockIter keyBuf [2]keyspan.Key span keyspan.Span - err error dir int8 closeHook func(i keyspan.FragmentIterator) error @@ -1583,7 +1582,7 @@ func (i *fragmentBlockIter) resetForReuse() fragmentBlockIter { return fragmentBlockIter{blockIter: i.blockIter.resetForReuse()} } -func (i *fragmentBlockIter) decodeSpanKeys(k *InternalKey, internalValue []byte) { +func (i *fragmentBlockIter) decodeSpanKeys(k *InternalKey, internalValue []byte) error { // TODO(jackson): The use of i.span.Keys to accumulate keys across multiple // calls to Decode is too confusing and subtle. Refactor to make it // explicit. @@ -1593,16 +1592,17 @@ func (i *fragmentBlockIter) decodeSpanKeys(k *InternalKey, internalValue []byte) // whereas the various range key kinds store are more complicated. The // details of the range key internal value format are documented within the // internal/rangekey package. + var err error switch k.Kind() { case base.InternalKeyKindRangeDelete: i.span = rangedel.Decode(*k, internalValue, i.span.Keys) - i.err = nil case base.InternalKeyKindRangeKeySet, base.InternalKeyKindRangeKeyUnset, base.InternalKeyKindRangeKeyDelete: - i.span, i.err = rangekey.Decode(*k, internalValue, i.span.Keys) + i.span, err = rangekey.Decode(*k, internalValue, i.span.Keys) default: i.span = keyspan.Span{} - i.err = base.CorruptionErrorf("pebble: corrupt keyspan fragment of kind %d", k.Kind()) + err = base.CorruptionErrorf("pebble: corrupt keyspan fragment of kind %d", k.Kind()) } + return err } func (i *fragmentBlockIter) elideKeysOfSameSeqNum() { @@ -1631,21 +1631,21 @@ func (i *fragmentBlockIter) elideKeysOfSameSeqNum() { // // gatherForward iterates forward, re-combining the fragmented internal keys to // reconstruct a keyspan.Span that holds all the keys defined over the span. -func (i *fragmentBlockIter) gatherForward(k *InternalKey, lazyValue base.LazyValue) *keyspan.Span { +func (i *fragmentBlockIter) gatherForward( + k *InternalKey, lazyValue base.LazyValue, +) (*keyspan.Span, error) { i.span = keyspan.Span{} if k == nil || !i.blockIter.valid() { - return nil + return nil, nil } - i.err = nil // Use the i.keyBuf array to back the Keys slice to prevent an allocation // when a span contains few keys. i.span.Keys = i.keyBuf[:0] // Decode the span's end key and individual keys from the value. internalValue := lazyValue.InPlaceValue() - i.decodeSpanKeys(k, internalValue) - if i.err != nil { - return nil + if err := i.decodeSpanKeys(k, internalValue); err != nil { + return nil, err } prevEnd := i.span.End @@ -1655,9 +1655,8 @@ func (i *fragmentBlockIter) gatherForward(k *InternalKey, lazyValue base.LazyVal k, lazyValue = i.blockIter.Next() internalValue = lazyValue.InPlaceValue() for k != nil && i.blockIter.cmp(k.UserKey, i.span.Start) == 0 { - i.decodeSpanKeys(k, internalValue) - if i.err != nil { - return nil + if err := i.decodeSpanKeys(k, internalValue); err != nil { + return nil, err } // Since k indicates an equal start key, the encoded end key must @@ -1665,9 +1664,8 @@ func (i *fragmentBlockIter) gatherForward(k *InternalKey, lazyValue base.LazyVal // Overlapping fragments are required to have exactly equal start and // end bounds. if i.blockIter.cmp(prevEnd, i.span.End) != 0 { - i.err = base.CorruptionErrorf("pebble: corrupt keyspan fragmentation") i.span = keyspan.Span{} - return nil + return nil, base.CorruptionErrorf("pebble: corrupt keyspan fragmentation") } k, lazyValue = i.blockIter.Next() internalValue = lazyValue.InPlaceValue() @@ -1676,7 +1674,7 @@ func (i *fragmentBlockIter) gatherForward(k *InternalKey, lazyValue base.LazyVal i.elideKeysOfSameSeqNum() } // i.blockIter is positioned over the first internal key for the next span. - return &i.span + return &i.span, nil } // gatherBackward gathers internal keys with identical bounds. Keys defined over @@ -1687,21 +1685,21 @@ func (i *fragmentBlockIter) gatherForward(k *InternalKey, lazyValue base.LazyVal // // gatherBackward iterates backwards, re-combining the fragmented internal keys // to reconstruct a keyspan.Span that holds all the keys defined over the span. -func (i *fragmentBlockIter) gatherBackward(k *InternalKey, lazyValue base.LazyValue) *keyspan.Span { +func (i *fragmentBlockIter) gatherBackward( + k *InternalKey, lazyValue base.LazyValue, +) (*keyspan.Span, error) { i.span = keyspan.Span{} if k == nil || !i.blockIter.valid() { - return nil + return nil, nil } - i.err = nil // Use the i.keyBuf array to back the Keys slice to prevent an allocation // when a span contains few keys. i.span.Keys = i.keyBuf[:0] // Decode the span's end key and individual keys from the value. internalValue := lazyValue.InPlaceValue() - i.decodeSpanKeys(k, internalValue) - if i.err != nil { - return nil + if err := i.decodeSpanKeys(k, internalValue); err != nil { + return nil, err } prevEnd := i.span.End @@ -1711,9 +1709,8 @@ func (i *fragmentBlockIter) gatherBackward(k *InternalKey, lazyValue base.LazyVa k, lazyValue = i.blockIter.Prev() internalValue = lazyValue.InPlaceValue() for k != nil && i.blockIter.cmp(k.UserKey, i.span.Start) == 0 { - i.decodeSpanKeys(k, internalValue) - if i.err != nil { - return nil + if err := i.decodeSpanKeys(k, internalValue); err != nil { + return nil, err } // Since k indicates an equal start key, the encoded end key must @@ -1721,9 +1718,8 @@ func (i *fragmentBlockIter) gatherBackward(k *InternalKey, lazyValue base.LazyVa // Overlapping fragments are required to have exactly equal start and // end bounds. if i.blockIter.cmp(prevEnd, i.span.End) != 0 { - i.err = base.CorruptionErrorf("pebble: corrupt keyspan fragmentation") i.span = keyspan.Span{} - return nil + return nil, base.CorruptionErrorf("pebble: corrupt keyspan fragmentation") } k, lazyValue = i.blockIter.Prev() internalValue = lazyValue.InPlaceValue() @@ -1737,12 +1733,7 @@ func (i *fragmentBlockIter) gatherBackward(k *InternalKey, lazyValue base.LazyVa if i.elideSameSeqnum && len(i.span.Keys) > 0 { i.elideKeysOfSameSeqNum() } - return &i.span -} - -// Error implements (keyspan.FragmentIterator).Error. -func (i *fragmentBlockIter) Error() error { - return i.err + return &i.span, nil } // Close implements (keyspan.FragmentIterator).Close. @@ -1756,19 +1747,19 @@ func (i *fragmentBlockIter) Close() error { } // First implements (keyspan.FragmentIterator).First -func (i *fragmentBlockIter) First() *keyspan.Span { +func (i *fragmentBlockIter) First() (*keyspan.Span, error) { i.dir = +1 return i.gatherForward(i.blockIter.First()) } // Last implements (keyspan.FragmentIterator).Last. -func (i *fragmentBlockIter) Last() *keyspan.Span { +func (i *fragmentBlockIter) Last() (*keyspan.Span, error) { i.dir = -1 return i.gatherBackward(i.blockIter.Last()) } // Next implements (keyspan.FragmentIterator).Next. -func (i *fragmentBlockIter) Next() *keyspan.Span { +func (i *fragmentBlockIter) Next() (*keyspan.Span, error) { switch { case i.dir == -1 && !i.span.Valid(): // Switching directions. @@ -1792,7 +1783,9 @@ func (i *fragmentBlockIter) Next() *keyspan.Span { // ... [a,b) [b,c) [b,c) [b,c) [d,e) ... // ^ ^ // i.blockIter want - if x := i.gatherForward(i.blockIter.Next()); invariants.Enabled && !x.Valid() { + if x, err := i.gatherForward(i.blockIter.Next()); err != nil { + return nil, err + } else if invariants.Enabled && !x.Valid() { panic("pebble: invariant violation: next entry unexpectedly invalid") } i.dir = +1 @@ -1802,7 +1795,7 @@ func (i *fragmentBlockIter) Next() *keyspan.Span { } // Prev implements (keyspan.FragmentIterator).Prev. -func (i *fragmentBlockIter) Prev() *keyspan.Span { +func (i *fragmentBlockIter) Prev() (*keyspan.Span, error) { switch { case i.dir == +1 && !i.span.Valid(): // Switching directions. @@ -1826,7 +1819,9 @@ func (i *fragmentBlockIter) Prev() *keyspan.Span { // ... [a,b) [b,c) [b,c) [b,c) [d,e) ... // ^ ^ // want i.blockIter - if x := i.gatherBackward(i.blockIter.Prev()); invariants.Enabled && !x.Valid() { + if x, err := i.gatherBackward(i.blockIter.Prev()); err != nil { + return nil, err + } else if invariants.Enabled && !x.Valid() { panic("pebble: invariant violation: previous entry unexpectedly invalid") } i.dir = -1 @@ -1836,9 +1831,11 @@ func (i *fragmentBlockIter) Prev() *keyspan.Span { } // SeekGE implements (keyspan.FragmentIterator).SeekGE. -func (i *fragmentBlockIter) SeekGE(k []byte) *keyspan.Span { - if s := i.SeekLT(k); s != nil && i.blockIter.cmp(k, s.End) < 0 { - return s +func (i *fragmentBlockIter) SeekGE(k []byte) (*keyspan.Span, error) { + if s, err := i.SeekLT(k); err != nil { + return nil, err + } else if s != nil && i.blockIter.cmp(k, s.End) < 0 { + return s, nil } // TODO(jackson): If the above i.SeekLT(k) discovers a span but the span // doesn't meet the k < s.End comparison, then there's no need for the @@ -1847,7 +1844,7 @@ func (i *fragmentBlockIter) SeekGE(k []byte) *keyspan.Span { } // SeekLT implements (keyspan.FragmentIterator).SeekLT. -func (i *fragmentBlockIter) SeekLT(k []byte) *keyspan.Span { +func (i *fragmentBlockIter) SeekLT(k []byte) (*keyspan.Span, error) { i.dir = -1 return i.gatherBackward(i.blockIter.SeekLT(k, base.SeekLTFlagsNone)) } diff --git a/sstable/prefix_replacing_iterator.go b/sstable/prefix_replacing_iterator.go index e2cee14cf6..caab3ebb13 100644 --- a/sstable/prefix_replacing_iterator.go +++ b/sstable/prefix_replacing_iterator.go @@ -187,7 +187,6 @@ func (p *prefixReplacingIterator) SetCloseHook(fn func(i Iterator) error) { type prefixReplacingFragmentIterator struct { i keyspan.FragmentIterator - err error src, dst []byte arg []byte out1, out2 []byte @@ -208,63 +207,63 @@ func newPrefixReplacingFragmentIterator( } } -func (p *prefixReplacingFragmentIterator) rewriteArg(key []byte) []byte { +func (p *prefixReplacingFragmentIterator) rewriteArg(key []byte) ([]byte, error) { if !bytes.HasPrefix(key, p.dst) { - p.err = errInputPrefixMismatch - return key + return nil, errInputPrefixMismatch } p.arg = append(p.arg[:len(p.src)], key[len(p.dst):]...) - return p.arg + return p.arg, nil } -func (p *prefixReplacingFragmentIterator) rewriteSpan(sp *keyspan.Span) *keyspan.Span { +func (p *prefixReplacingFragmentIterator) rewriteSpan( + sp *keyspan.Span, err error, +) (*keyspan.Span, error) { if !bytes.HasPrefix(sp.Start, p.src) || !bytes.HasPrefix(sp.End, p.src) { - p.err = errInputPrefixMismatch - return sp + return nil, errInputPrefixMismatch } sp.Start = append(p.out1[:len(p.dst)], sp.Start[len(p.src):]...) sp.End = append(p.out2[:len(p.dst)], sp.End[len(p.src):]...) - return sp + return sp, nil } // SeekGE implements the FragmentIterator interface. -func (p *prefixReplacingFragmentIterator) SeekGE(key []byte) *keyspan.Span { - return p.rewriteSpan(p.i.SeekGE(p.rewriteArg(key))) +func (p *prefixReplacingFragmentIterator) SeekGE(key []byte) (*keyspan.Span, error) { + rewrittenKey, err := p.rewriteArg(key) + if err != nil { + return nil, err + } + return p.rewriteSpan(p.i.SeekGE(rewrittenKey)) } // SeekLT implements the FragmentIterator interface. -func (p *prefixReplacingFragmentIterator) SeekLT(key []byte) *keyspan.Span { - return p.rewriteSpan(p.i.SeekLT(p.rewriteArg(key))) +func (p *prefixReplacingFragmentIterator) SeekLT(key []byte) (*keyspan.Span, error) { + rewrittenKey, err := p.rewriteArg(key) + if err != nil { + return nil, err + } + return p.rewriteSpan(p.i.SeekLT(rewrittenKey)) } // First implements the FragmentIterator interface. -func (p *prefixReplacingFragmentIterator) First() *keyspan.Span { +func (p *prefixReplacingFragmentIterator) First() (*keyspan.Span, error) { return p.rewriteSpan(p.i.First()) } // Last implements the FragmentIterator interface. -func (p *prefixReplacingFragmentIterator) Last() *keyspan.Span { +func (p *prefixReplacingFragmentIterator) Last() (*keyspan.Span, error) { return p.rewriteSpan(p.i.Last()) } // Close implements the FragmentIterator interface. -func (p *prefixReplacingFragmentIterator) Next() *keyspan.Span { +func (p *prefixReplacingFragmentIterator) Next() (*keyspan.Span, error) { return p.rewriteSpan(p.i.Next()) } // Prev implements the FragmentIterator interface. -func (p *prefixReplacingFragmentIterator) Prev() *keyspan.Span { +func (p *prefixReplacingFragmentIterator) Prev() (*keyspan.Span, error) { return p.rewriteSpan(p.i.Prev()) } -// Error implements the FragmentIterator interface. -func (p *prefixReplacingFragmentIterator) Error() error { - if p.err != nil { - return p.err - } - return p.i.Error() -} - // Close implements the FragmentIterator interface. func (p *prefixReplacingFragmentIterator) Close() error { return p.i.Close() diff --git a/sstable/reader_test.go b/sstable/reader_test.go index cc20f8abc3..c94083eca8 100644 --- a/sstable/reader_test.go +++ b/sstable/reader_test.go @@ -407,9 +407,13 @@ func TestVirtualReader(t *testing.T) { defer iter.Close() var buf bytes.Buffer - for s := iter.First(); s != nil; s = iter.Next() { + s, err := iter.First() + for ; s != nil; s, err = iter.Next() { fmt.Fprintf(&buf, "%s\n", s) } + if err != nil { + return err.Error() + } return buf.String() case "scan-range-key": @@ -426,9 +430,13 @@ func TestVirtualReader(t *testing.T) { defer iter.Close() var buf bytes.Buffer - for s := iter.First(); s != nil; s = iter.Next() { + s, err := iter.First() + for ; s != nil; s, err = iter.Next() { fmt.Fprintf(&buf, "%s\n", s) } + if err != nil { + return err.Error() + } return buf.String() case "iter": diff --git a/sstable/suffix_rewriter.go b/sstable/suffix_rewriter.go index a226e8eb9c..359d2e5d5a 100644 --- a/sstable/suffix_rewriter.go +++ b/sstable/suffix_rewriter.go @@ -402,7 +402,8 @@ func rewriteRangeKeyBlockToWriter(r *Reader, w *Writer, from, to []byte) error { } defer iter.Close() - for s := iter.First(); s != nil; s = iter.Next() { + s, err := iter.First() + for ; s != nil; s, err = iter.Next() { if !s.Valid() { break } @@ -416,7 +417,7 @@ func rewriteRangeKeyBlockToWriter(r *Reader, w *Writer, from, to []byte) error { s.Keys[i].Suffix = to } - err := rangekey.Encode(s, func(k base.InternalKey, v []byte) error { + err = rangekey.Encode(s, func(k base.InternalKey, v []byte) error { // Calling AddRangeKey instead of addRangeKeySpan bypasses the fragmenter. // This is okay because the raw fragments off of `iter` are already // fragmented, and suffix replacement should not affect fragmentation. @@ -426,8 +427,7 @@ func rewriteRangeKeyBlockToWriter(r *Reader, w *Writer, from, to []byte) error { return err } } - - return nil + return err } type copyFilterWriter struct { diff --git a/sstable/writer_rangekey_test.go b/sstable/writer_rangekey_test.go index bbe6946a1c..1968059651 100644 --- a/sstable/writer_rangekey_test.go +++ b/sstable/writer_rangekey_test.go @@ -112,9 +112,13 @@ func TestWriter_RangeKeys(t *testing.T) { defer iter.Close() var buf bytes.Buffer - for s := iter.First(); s != nil; s = iter.Next() { + s, err := iter.First() + for ; s != nil; s, err = iter.Next() { _, _ = fmt.Fprintf(&buf, "%s\n", s) } + if err != nil { + return err.Error() + } return buf.String() default: diff --git a/sstable/writer_test.go b/sstable/writer_test.go index 60adf6d6a4..2a5e9dad0e 100644 --- a/sstable/writer_test.go +++ b/sstable/writer_test.go @@ -177,9 +177,13 @@ func runDataDriven(t *testing.T, file string, tableFormat TableFormat, paralleli defer iter.Close() var buf bytes.Buffer - for s := iter.First(); s != nil; s = iter.Next() { + s, err := iter.First() + for ; s != nil; s, err = iter.Next() { fmt.Fprintf(&buf, "%s\n", s) } + if err != nil { + return err.Error() + } return buf.String() case "scan-range-key": @@ -193,9 +197,13 @@ func runDataDriven(t *testing.T, file string, tableFormat TableFormat, paralleli defer iter.Close() var buf bytes.Buffer - for s := iter.First(); s != nil; s = iter.Next() { + s, err := iter.First() + for ; s != nil; s, err = iter.Next() { fmt.Fprintf(&buf, "%s\n", s) } + if err != nil { + return err.Error() + } return buf.String() case "layout": diff --git a/table_stats.go b/table_stats.go index 3c0994d45d..f2afbd848c 100644 --- a/table_stats.go +++ b/table_stats.go @@ -371,7 +371,8 @@ func (d *DB) loadTableRangeDelStats( // numbers. Also, merging abutting tombstones reduces the number of calls to // estimateReclaimedSizeBeneath which is costly, and improves the accuracy of // our overall estimate. - for s := iter.First(); s != nil; s = iter.Next() { + s, err := iter.First() + for ; s != nil; s, err = iter.Next() { start, end := s.Start, s.End // We only need to consider deletion size estimates for tables that contain // RANGEDELs. @@ -459,7 +460,10 @@ func (d *DB) loadTableRangeDelStats( copy(hint.end, end) compactionHints = append(compactionHints, hint) } - return compactionHints, err + if err != nil { + return nil, err + } + return compactionHints, nil } func (d *DB) estimateSizesBeneath( diff --git a/table_stats_test.go b/table_stats_test.go index 8122d77e1c..27db47ac11 100644 --- a/table_stats_test.go +++ b/table_stats_test.go @@ -263,9 +263,13 @@ func TestTableRangeDeletionIter(t *testing.T) { } defer iter.Close() var buf bytes.Buffer - for s := iter.First(); s != nil; s = iter.Next() { + s, err := iter.First() + for ; s != nil; s, err = iter.Next() { buf.WriteString(s.String() + "\n") } + if err != nil { + return err.Error() + } if buf.Len() == 0 { return "(none)" } diff --git a/tool/find.go b/tool/find.go index 0521d985e2..beac0a028e 100644 --- a/tool/find.go +++ b/tool/find.go @@ -469,12 +469,16 @@ func (f *findT) searchTables(stdout io.Writer, searchKey []byte, refs []findRef) defer iter.Close() var tombstones []keyspan.Span - for t := iter.First(); t != nil; t = iter.Next() { + t, err := iter.First() + for ; t != nil; t, err = iter.Next() { if !t.Contains(r.Compare, searchKey) { continue } tombstones = append(tombstones, t.ShallowClone()) } + if err != nil { + return nil, err + } slices.SortFunc(tombstones, func(a, b keyspan.Span) int { return r.Compare(a.Start, b.Start) @@ -486,7 +490,10 @@ func (f *findT) searchTables(stdout io.Writer, searchKey []byte, refs []findRef) } defer rangeDelIter.Close() - rangeDel := rangeDelIter.First() + rangeDel, err := rangeDelIter.First() + if err != nil { + return err + } foundRef := false for key != nil || rangeDel != nil { @@ -520,7 +527,10 @@ func (f *findT) searchTables(stdout io.Writer, searchKey []byte, refs []findRef) if err != nil { return err } - rangeDel = rangeDelIter.Next() + rangeDel, err = rangeDelIter.Next() + if err != nil { + return err + } } foundRef = true } diff --git a/tool/sstable.go b/tool/sstable.go index 1e348776ac..7b59f724d0 100644 --- a/tool/sstable.go +++ b/tool/sstable.go @@ -408,7 +408,8 @@ func (s *sstableT) runScan(cmd *cobra.Command, args []string) { defer iter.Close() var tombstones []keyspan.Span - for t := iter.First(); t != nil; t = iter.Next() { + t, err := iter.First() + for ; t != nil; t, err = iter.Next() { if s.end != nil && r.Compare(s.end, t.Start) <= 0 { // The range tombstone lies after the scan range. continue @@ -419,6 +420,9 @@ func (s *sstableT) runScan(cmd *cobra.Command, args []string) { } tombstones = append(tombstones, t.ShallowClone()) } + if err != nil { + return nil, err + } slices.SortFunc(tombstones, func(a, b keyspan.Span) int { return r.Compare(a.Start, b.Start) @@ -431,7 +435,11 @@ func (s *sstableT) runScan(cmd *cobra.Command, args []string) { } defer rangeDelIter.Close() - rangeDel := rangeDelIter.First() + rangeDel, err := rangeDelIter.First() + if err != nil { + fmt.Fprintf(stdout, "%s%s\n", prefix, err) + return + } count := s.count var lastKey base.InternalKey @@ -476,7 +484,11 @@ func (s *sstableT) runScan(cmd *cobra.Command, args []string) { os.Exit(1) } } - rangeDel = rangeDelIter.Next() + rangeDel, err = rangeDelIter.Next() + if err != nil { + fmt.Fprintf(stdout, "%s\n", err) + os.Exit(1) + } } if count > 0 { @@ -495,7 +507,8 @@ func (s *sstableT) runScan(cmd *cobra.Command, args []string) { } if rkIter != nil { defer rkIter.Close() - for span := rkIter.SeekGE(s.start); span != nil; span = rkIter.Next() { + span, err := rkIter.SeekGE(s.start) + for ; span != nil; span, err = rkIter.Next() { // By default, emit the key, unless there is a filter. emit := s.filter == nil // Skip spans that start after the end key (if provided). End keys are @@ -513,6 +526,10 @@ func (s *sstableT) runScan(cmd *cobra.Command, args []string) { formatSpan(stdout, s.fmtKey, s.fmtValue, span) } } + if err != nil { + fmt.Fprintf(stdout, "%s\n", err) + os.Exit(1) + } } if err := iter.Close(); err != nil {