From be542c718b1062525a55d606f85bf27bdb972124 Mon Sep 17 00:00:00 2001 From: Bilal Akhtar Date: Wed, 16 Aug 2023 15:25:12 -0400 Subject: [PATCH] *: add error return values to New{MVCC,Engine}Iterator Now that the iterator creation return paths can return errors from Pebble, thread that through the interface and all callers. Mechanical but very large and widespread change. Epic: none Release note: None --- .../full_cluster_backup_restore_test.go | 3 +- .../backupccl/restore_data_processor_test.go | 3 +- pkg/ccl/storageccl/engineccl/bench_test.go | 11 +- .../replication_random_client_test.go | 6 +- .../stream_ingestion_processor_test.go | 5 +- pkg/cli/debug.go | 5 +- pkg/kv/kvnemesis/engine.go | 11 +- pkg/kv/kvnemesis/validator.go | 10 +- pkg/kv/kvserver/batch_spanset_test.go | 37 +++- pkg/kv/kvserver/batcheval/cmd_add_sstable.go | 5 +- pkg/kv/kvserver/batcheval/cmd_clear_range.go | 5 +- .../batcheval/cmd_clear_range_test.go | 5 +- .../batcheval/cmd_delete_range_test.go | 10 +- .../kvserver/batcheval/cmd_end_transaction.go | 5 +- pkg/kv/kvserver/batcheval/cmd_export_test.go | 2 +- .../batcheval/cmd_query_resolved_timestamp.go | 5 +- .../kvserver/batcheval/cmd_refresh_range.go | 2 +- pkg/kv/kvserver/batcheval/cmd_revert_range.go | 5 +- pkg/kv/kvserver/batcheval/intent_test.go | 2 +- .../kvserver/client_manual_proposal_test.go | 3 +- pkg/kv/kvserver/client_raft_test.go | 5 +- pkg/kv/kvserver/client_split_test.go | 5 +- pkg/kv/kvserver/gc/gc_iterator_test.go | 5 +- pkg/kv/kvserver/gc/gc_random_test.go | 15 +- pkg/kv/kvserver/gc/gc_test.go | 18 +- pkg/kv/kvserver/kvstorage/init.go | 10 +- pkg/kv/kvserver/logstore/stateloader.go | 5 +- pkg/kv/kvserver/loqrecovery/record.go | 5 +- pkg/kv/kvserver/raft_log_truncator_test.go | 5 +- pkg/kv/kvserver/raftlog/iter_bench_test.go | 14 +- pkg/kv/kvserver/raftlog/iter_test.go | 6 +- pkg/kv/kvserver/raftlog/iterator.go | 21 ++- pkg/kv/kvserver/rangefeed/catchup_scan.go | 2 +- pkg/kv/kvserver/rangefeed/task_test.go | 10 +- pkg/kv/kvserver/rditer/replica_data_iter.go | 17 +- pkg/kv/kvserver/replica_evaluate.go | 20 ++- pkg/kv/kvserver/replica_raft.go | 5 +- pkg/kv/kvserver/replica_rangefeed.go | 6 +- pkg/kv/kvserver/replica_test.go | 9 +- pkg/kv/kvserver/spanset/batch.go | 36 +++- pkg/server/init.go | 5 +- pkg/sql/row/fetcher_mvcc_test.go | 5 +- pkg/sql/tests/data.go | 5 +- pkg/storage/batch_test.go | 22 ++- pkg/storage/bench_test.go | 20 ++- pkg/storage/col_mvcc.go | 5 +- pkg/storage/disk_map.go | 15 +- pkg/storage/disk_map_test.go | 5 +- pkg/storage/engine.go | 61 +++++-- pkg/storage/engine_test.go | 144 +++++++++------ pkg/storage/intent_interleaving_iter.go | 16 +- pkg/storage/intent_interleaving_iter_test.go | 90 ++++++++-- pkg/storage/intent_reader_writer.go | 2 +- pkg/storage/intent_reader_writer_test.go | 6 +- pkg/storage/metamorphic/operations.go | 5 +- pkg/storage/mvcc.go | 166 ++++++++++++++---- pkg/storage/mvcc_history_test.go | 18 +- pkg/storage/mvcc_incremental_iterator.go | 10 +- pkg/storage/mvcc_incremental_iterator_test.go | 8 +- pkg/storage/mvcc_stats_test.go | 5 +- pkg/storage/mvcc_test.go | 55 ++++-- pkg/storage/pebble.go | 144 ++++++++++++--- pkg/storage/pebble_batch.go | 71 ++++++-- pkg/storage/pebble_iterator.go | 23 ++- pkg/storage/pebble_iterator_test.go | 3 +- pkg/storage/pebble_mvcc_scanner_test.go | 11 +- pkg/storage/pebble_test.go | 77 +++++--- pkg/storage/read_as_of_iterator_test.go | 6 +- pkg/storage/sst.go | 15 +- pkg/testutils/storageutils/mvcc.go | 5 +- pkg/testutils/storageutils/scan.go | 5 +- pkg/ts/pruning.go | 5 +- 72 files changed, 1061 insertions(+), 331 deletions(-) diff --git a/pkg/ccl/backupccl/full_cluster_backup_restore_test.go b/pkg/ccl/backupccl/full_cluster_backup_restore_test.go index c01d1a92cd54..8314e49698fa 100644 --- a/pkg/ccl/backupccl/full_cluster_backup_restore_test.go +++ b/pkg/ccl/backupccl/full_cluster_backup_restore_test.go @@ -223,9 +223,10 @@ CREATE TABLE data2.foo (a int); store := tcRestore.GetFirstStoreFromServer(t, 0) startKey := keys.SystemSQLCodec.TablePrefix(uint32(id)) endKey := startKey.PrefixEnd() - it := store.TODOEngine().NewMVCCIterator(storage.MVCCKeyAndIntentsIterKind, storage.IterOptions{ + it, err := store.TODOEngine().NewMVCCIterator(storage.MVCCKeyAndIntentsIterKind, storage.IterOptions{ UpperBound: endKey, }) + require.NoError(t, err) defer it.Close() it.SeekGE(storage.MVCCKey{Key: startKey}) hasKey, err := it.Valid() diff --git a/pkg/ccl/backupccl/restore_data_processor_test.go b/pkg/ccl/backupccl/restore_data_processor_test.go index 0d935714090d..b11d0ff7b727 100644 --- a/pkg/ccl/backupccl/restore_data_processor_test.go +++ b/pkg/ccl/backupccl/restore_data_processor_test.go @@ -117,7 +117,8 @@ func slurpSSTablesLatestKey( } var kvs []storage.MVCCKeyValue - it := batch.NewMVCCIterator(storage.MVCCKeyAndIntentsIterKind, storage.IterOptions{UpperBound: roachpb.KeyMax}) + it, err := batch.NewMVCCIterator(storage.MVCCKeyAndIntentsIterKind, storage.IterOptions{UpperBound: roachpb.KeyMax}) + require.NoError(t, err) defer it.Close() for it.SeekGE(start); ; it.NextKey() { if ok, err := it.Valid(); err != nil { diff --git a/pkg/ccl/storageccl/engineccl/bench_test.go b/pkg/ccl/storageccl/engineccl/bench_test.go index 6c3f8d5ab110..290c87c110d7 100644 --- a/pkg/ccl/storageccl/engineccl/bench_test.go +++ b/pkg/ccl/storageccl/engineccl/bench_test.go @@ -127,7 +127,7 @@ func loadTestData( func runIterate( b *testing.B, loadFactor float32, - makeIterator func(storage.Engine, hlc.Timestamp, hlc.Timestamp) storage.MVCCIterator, + makeIterator func(storage.Engine, hlc.Timestamp, hlc.Timestamp) (storage.MVCCIterator, error), ) { const numKeys = 100000 const numBatches = 100 @@ -149,7 +149,10 @@ func runIterate( if endTime.IsEmpty() { endTime = endTime.Next() } - it := makeIterator(eng, startTime, endTime) + it, err := makeIterator(eng, startTime, endTime) + if err != nil { + b.Fatal(err) + } defer it.Close() for it.SeekGE(storage.MVCCKey{Key: keys.LocalMax}); ; it.Next() { if ok, err := it.Valid(); !ok { @@ -172,12 +175,12 @@ func BenchmarkTimeBoundIterate(b *testing.B) { for _, loadFactor := range []float32{1.0, 0.5, 0.1, 0.05, 0.0} { b.Run(fmt.Sprintf("LoadFactor=%.2f", loadFactor), func(b *testing.B) { b.Run("NormalIterator", func(b *testing.B) { - runIterate(b, loadFactor, func(e storage.Engine, _, _ hlc.Timestamp) storage.MVCCIterator { + runIterate(b, loadFactor, func(e storage.Engine, _, _ hlc.Timestamp) (storage.MVCCIterator, error) { return e.NewMVCCIterator(storage.MVCCKeyAndIntentsIterKind, storage.IterOptions{UpperBound: roachpb.KeyMax}) }) }) b.Run("TimeBoundIterator", func(b *testing.B) { - runIterate(b, loadFactor, func(e storage.Engine, startTime, endTime hlc.Timestamp) storage.MVCCIterator { + runIterate(b, loadFactor, func(e storage.Engine, startTime, endTime hlc.Timestamp) (storage.MVCCIterator, error) { return e.NewMVCCIterator(storage.MVCCKeyIterKind, storage.IterOptions{ MinTimestampHint: startTime, MaxTimestampHint: endTime, diff --git a/pkg/ccl/streamingccl/streamingest/replication_random_client_test.go b/pkg/ccl/streamingccl/streamingest/replication_random_client_test.go index 734e399a573e..2828a2b499da 100644 --- a/pkg/ccl/streamingccl/streamingest/replication_random_client_test.go +++ b/pkg/ccl/streamingccl/streamingest/replication_random_client_test.go @@ -297,14 +297,16 @@ func assertExactlyEqualKVs( ) hlc.Timestamp { // Iterate over the store. store := tc.GetFirstStoreFromServer(t, 0) - it := store.TODOEngine().NewMVCCIterator(storage.MVCCKeyIterKind, storage.IterOptions{ + it, err := store.TODOEngine().NewMVCCIterator(storage.MVCCKeyIterKind, storage.IterOptions{ LowerBound: tenantPrefix, UpperBound: tenantPrefix.PrefixEnd(), }) + if err != nil { + t.Fatal(err) + } defer it.Close() var prevKey roachpb.Key var valueTimestampTuples []roachpb.KeyValue - var err error var maxKVTimestampSeen hlc.Timestamp var matchingKVs int for it.SeekGE(storage.MVCCKey{}); ; it.Next() { diff --git a/pkg/ccl/streamingccl/streamingest/stream_ingestion_processor_test.go b/pkg/ccl/streamingccl/streamingest/stream_ingestion_processor_test.go index 91a7f08656d3..b806c548b475 100644 --- a/pkg/ccl/streamingccl/streamingest/stream_ingestion_processor_test.go +++ b/pkg/ccl/streamingccl/streamingest/stream_ingestion_processor_test.go @@ -544,10 +544,13 @@ func assertEqualKVs( // Iterate over the store. store, err := srv.GetStores().(*kvserver.Stores).GetStore(srv.GetFirstStoreID()) require.NoError(t, err) - it := store.TODOEngine().NewMVCCIterator(storage.MVCCKeyAndIntentsIterKind, storage.IterOptions{ + it, err := store.TODOEngine().NewMVCCIterator(storage.MVCCKeyAndIntentsIterKind, storage.IterOptions{ LowerBound: targetSpan.Key, UpperBound: targetSpan.EndKey, }) + if err != nil { + t.Fatal(err) + } defer it.Close() var prevKey roachpb.Key var valueTimestampTuples []roachpb.KeyValue diff --git a/pkg/cli/debug.go b/pkg/cli/debug.go index ead75e464a67..fd16ecb4ed30 100644 --- a/pkg/cli/debug.go +++ b/pkg/cli/debug.go @@ -1272,10 +1272,13 @@ func runDebugIntentCount(cmd *cobra.Command, args []string) error { } }) - iter := db.NewEngineIterator(storage.IterOptions{ + iter, err := db.NewEngineIterator(storage.IterOptions{ LowerBound: keys.LockTableSingleKeyStart, UpperBound: keys.LockTableSingleKeyEnd, }) + if err != nil { + return err + } defer iter.Close() seekKey := storage.EngineKey{Key: keys.LockTableSingleKeyStart} diff --git a/pkg/kv/kvnemesis/engine.go b/pkg/kv/kvnemesis/engine.go index 2fa72432217c..ae489a86b11e 100644 --- a/pkg/kv/kvnemesis/engine.go +++ b/pkg/kv/kvnemesis/engine.go @@ -62,7 +62,10 @@ func (e *Engine) Get(key roachpb.Key, ts hlc.Timestamp) roachpb.Value { Suffix: storage.EncodeMVCCTimestampSuffix(ts), }, } - iter := e.kvs.NewIter(&opts) + iter, err := e.kvs.NewIter(&opts) + if err != nil { + panic(err) + } defer func() { _ = iter.Close() }() iter.SeekGE(storage.EncodeMVCCKey(storage.MVCCKey{Key: key, Timestamp: ts})) for iter.Valid() { @@ -126,7 +129,11 @@ func (e *Engine) DeleteRange(from, to roachpb.Key, ts hlc.Timestamp, val []byte) func (e *Engine) Iterate( fn func(key, endKey roachpb.Key, ts hlc.Timestamp, value []byte, err error), ) { - iter := e.kvs.NewIter(&pebble.IterOptions{KeyTypes: pebble.IterKeyTypePointsAndRanges}) + iter, err := e.kvs.NewIter(&pebble.IterOptions{KeyTypes: pebble.IterKeyTypePointsAndRanges}) + if err != nil { + fn(nil, nil, hlc.Timestamp{}, nil, err) + return + } defer func() { _ = iter.Close() }() for iter.First(); iter.Valid(); iter.Next() { hasPoint, _ := iter.HasPointAndRange() diff --git a/pkg/kv/kvnemesis/validator.go b/pkg/kv/kvnemesis/validator.go index 0cc4cb34fd81..3bd4ef073492 100644 --- a/pkg/kv/kvnemesis/validator.go +++ b/pkg/kv/kvnemesis/validator.go @@ -1308,11 +1308,14 @@ func validReadTimes(b *pebble.Batch, key roachpb.Key, value []byte) disjointTime var hist []storage.MVCCValue lowerBound := storage.EncodeMVCCKey(storage.MVCCKey{Key: key}) upperBound := storage.EncodeMVCCKey(storage.MVCCKey{Key: key.Next()}) - iter := b.NewIter(&pebble.IterOptions{ + iter, err := b.NewIter(&pebble.IterOptions{ KeyTypes: storage.IterKeyTypePointsAndRanges, LowerBound: lowerBound, UpperBound: upperBound, }) + if err != nil { + panic(err) + } defer func() { _ = iter.Close() }() iter.SeekGE(lowerBound) @@ -1436,7 +1439,10 @@ func validScanTime(b *pebble.Batch, span roachpb.Span, kvs []roachpb.KeyValue) m // Note that this iterator ignores MVCC range deletions. We use this iterator // only to *discover* point keys; we then invoke validReadTimes for each of // them which *does* take into account MVCC range deletions. - iter := b.NewIter(nil) + iter, err := b.NewIter(nil) + if err != nil { + panic(err) + } defer func() { _ = iter.Close() }() iter.SeekGE(storage.EncodeMVCCKey(storage.MVCCKey{Key: span.Key})) diff --git a/pkg/kv/kvserver/batch_spanset_test.go b/pkg/kv/kvserver/batch_spanset_test.go index 91c77e645760..88fff8ebf652 100644 --- a/pkg/kv/kvserver/batch_spanset_test.go +++ b/pkg/kv/kvserver/batch_spanset_test.go @@ -154,7 +154,10 @@ func TestSpanSetBatchBoundaries(t *testing.T) { }) t.Run("forward scans", func(t *testing.T) { - iter := batch.NewMVCCIterator(storage.MVCCKeyAndIntentsIterKind, storage.IterOptions{UpperBound: roachpb.KeyMax}) + iter, err := batch.NewMVCCIterator(storage.MVCCKeyAndIntentsIterKind, storage.IterOptions{UpperBound: roachpb.KeyMax}) + if err != nil { + t.Fatal(err) + } defer iter.Close() // MVCCIterators check boundaries on seek and next/prev @@ -199,7 +202,12 @@ func TestSpanSetBatchBoundaries(t *testing.T) { } t.Run("reverse scans", func(t *testing.T) { - iter := spanset.NewIterator(eng.NewMVCCIterator(storage.MVCCKeyAndIntentsIterKind, storage.IterOptions{UpperBound: roachpb.KeyMax}), &ss) + innerIter, err := eng.NewMVCCIterator(storage.MVCCKeyAndIntentsIterKind, storage.IterOptions{UpperBound: roachpb.KeyMax}) + if err != nil { + t.Fatal(err) + } + iter := spanset.NewIterator(innerIter, &ss) + defer iter.Close() iter.SeekLT(outsideKey4) if _, err := iter.Valid(); !isReadSpanErr(err) { @@ -383,7 +391,10 @@ func TestSpanSetIteratorTimestamps(t *testing.T) { func() { // When accessing at t=1, we're able to read through latches declared at t=1 and t=2. - iter := batchAt1.NewMVCCIterator(storage.MVCCKeyAndIntentsIterKind, storage.IterOptions{UpperBound: roachpb.KeyMax}) + iter, err := batchAt1.NewMVCCIterator(storage.MVCCKeyAndIntentsIterKind, storage.IterOptions{UpperBound: roachpb.KeyMax}) + if err != nil { + t.Fatal(err) + } defer iter.Close() iter.SeekGE(k1) @@ -405,7 +416,10 @@ func TestSpanSetIteratorTimestamps(t *testing.T) { func() { // When accessing at t=2, we're only able to read through the latch declared at t=2. - iter := batchAt2.NewMVCCIterator(storage.MVCCKeyAndIntentsIterKind, storage.IterOptions{UpperBound: roachpb.KeyMax}) + iter, err := batchAt2.NewMVCCIterator(storage.MVCCKeyAndIntentsIterKind, storage.IterOptions{UpperBound: roachpb.KeyMax}) + if err != nil { + t.Fatal(err) + } defer iter.Close() iter.SeekGE(k1) @@ -425,7 +439,10 @@ func TestSpanSetIteratorTimestamps(t *testing.T) { for _, batch := range []storage.Batch{batchAt3, batchNonMVCC} { // When accessing at t=3, we're unable to read through any of the declared latches. // Same is true when accessing without a timestamp. - iter := batch.NewMVCCIterator(storage.MVCCKeyAndIntentsIterKind, storage.IterOptions{UpperBound: roachpb.KeyMax}) + iter, err := batch.NewMVCCIterator(storage.MVCCKeyAndIntentsIterKind, storage.IterOptions{UpperBound: roachpb.KeyMax}) + if err != nil { + t.Fatal(err) + } defer iter.Close() iter.SeekGE(k1) @@ -442,7 +459,10 @@ func TestSpanSetIteratorTimestamps(t *testing.T) { // The behavior is the same as above for an EngineIterator. func() { // When accessing at t=1, we're able to read through latches declared at t=1 and t=2. - iter := batchAt1.NewEngineIterator(storage.IterOptions{UpperBound: roachpb.KeyMax}) + iter, err := batchAt1.NewEngineIterator(storage.IterOptions{UpperBound: roachpb.KeyMax}) + if err != nil { + t.Fatal(err) + } defer iter.Close() if ok, err := iter.SeekEngineKeyGE(k1e); !ok { @@ -470,7 +490,10 @@ func TestSpanSetIteratorTimestamps(t *testing.T) { func() { // When accessing at t=2, we're only able to read through the latch declared at t=2. - iter := batchAt2.NewEngineIterator(storage.IterOptions{UpperBound: roachpb.KeyMax}) + iter, err := batchAt2.NewEngineIterator(storage.IterOptions{UpperBound: roachpb.KeyMax}) + if err != nil { + t.Fatal(err) + } defer iter.Close() if ok, _ := iter.SeekEngineKeyGE(k1e); ok { diff --git a/pkg/kv/kvserver/batcheval/cmd_add_sstable.go b/pkg/kv/kvserver/batcheval/cmd_add_sstable.go index a2a7062d3bd2..814263daae76 100644 --- a/pkg/kv/kvserver/batcheval/cmd_add_sstable.go +++ b/pkg/kv/kvserver/batcheval/cmd_add_sstable.go @@ -404,13 +404,16 @@ func EvalAddSSTable( // addition, and instead just use this key-only iterator. If a caller actually // needs to know what data is there, it must issue its own real Scan. if args.ReturnFollowingLikelyNonEmptySpanStart { - existingIter := spanset.DisableReaderAssertions(readWriter).NewMVCCIterator( + existingIter, err := spanset.DisableReaderAssertions(readWriter).NewMVCCIterator( storage.MVCCKeyIterKind, // don't care if it is committed or not, just that it isn't empty. storage.IterOptions{ KeyTypes: storage.IterKeyTypePointsAndRanges, UpperBound: reply.RangeSpan.EndKey, }, ) + if err != nil { + return result.Result{}, errors.Wrap(err, "error when creating iterator for non-empty span") + } defer existingIter.Close() existingIter.SeekGE(end) if ok, err := existingIter.Valid(); err != nil { diff --git a/pkg/kv/kvserver/batcheval/cmd_clear_range.go b/pkg/kv/kvserver/batcheval/cmd_clear_range.go index dfdbdcb0a76f..6b83267dae67 100644 --- a/pkg/kv/kvserver/batcheval/cmd_clear_range.go +++ b/pkg/kv/kvserver/batcheval/cmd_clear_range.go @@ -213,11 +213,14 @@ func computeStatsDelta( if !entireRange { leftPeekBound, rightPeekBound := rangeTombstonePeekBounds( from, to, desc.StartKey.AsRawKey(), desc.EndKey.AsRawKey()) - rkIter := readWriter.NewMVCCIterator(storage.MVCCKeyIterKind, storage.IterOptions{ + rkIter, err := readWriter.NewMVCCIterator(storage.MVCCKeyIterKind, storage.IterOptions{ KeyTypes: storage.IterKeyTypeRangesOnly, LowerBound: leftPeekBound, UpperBound: rightPeekBound, }) + if err != nil { + return enginepb.MVCCStats{}, err + } defer rkIter.Close() if cmp, lhs, err := storage.PeekRangeKeysLeft(rkIter, from); err != nil { diff --git a/pkg/kv/kvserver/batcheval/cmd_clear_range_test.go b/pkg/kv/kvserver/batcheval/cmd_clear_range_test.go index 0f8f2f640d6d..a6d6c955b29f 100644 --- a/pkg/kv/kvserver/batcheval/cmd_clear_range_test.go +++ b/pkg/kv/kvserver/batcheval/cmd_clear_range_test.go @@ -205,11 +205,14 @@ func TestCmdClearRange(t *testing.T) { require.Equal(t, tc.expClearIter, batch.clearIterCount == 1) // Ensure that the data is gone. - iter := eng.NewMVCCIterator(storage.MVCCKeyAndIntentsIterKind, storage.IterOptions{ + iter, err := eng.NewMVCCIterator(storage.MVCCKeyAndIntentsIterKind, storage.IterOptions{ KeyTypes: storage.IterKeyTypePointsAndRanges, LowerBound: startKey, UpperBound: endKey, }) + if err != nil { + t.Fatal(err) + } defer iter.Close() iter.SeekGE(storage.MVCCKey{Key: keys.LocalMax}) ok, err := iter.Valid() diff --git a/pkg/kv/kvserver/batcheval/cmd_delete_range_test.go b/pkg/kv/kvserver/batcheval/cmd_delete_range_test.go index 42620de94a6c..2bb06d1b7638 100644 --- a/pkg/kv/kvserver/batcheval/cmd_delete_range_test.go +++ b/pkg/kv/kvserver/batcheval/cmd_delete_range_test.go @@ -308,11 +308,14 @@ func TestDeleteRangeTombstone(t *testing.T) { // operated on. The command should not have written an actual rangekey! func checkPredicateDeleteRange(t *testing.T, engine storage.Reader, rKeyInfo storage.MVCCRangeKey) { - iter := engine.NewMVCCIterator(storage.MVCCKeyAndIntentsIterKind, storage.IterOptions{ + iter, err := engine.NewMVCCIterator(storage.MVCCKeyAndIntentsIterKind, storage.IterOptions{ KeyTypes: storage.IterKeyTypePointsAndRanges, LowerBound: rKeyInfo.StartKey, UpperBound: rKeyInfo.EndKey, }) + if err != nil { + t.Fatal(err) + } defer iter.Close() for iter.SeekGE(storage.MVCCKey{Key: rKeyInfo.StartKey}); ; iter.NextKey() { @@ -345,11 +348,14 @@ func checkDeleteRangeTombstone( written bool, now hlc.ClockTimestamp, ) { - iter := engine.NewMVCCIterator(storage.MVCCKeyAndIntentsIterKind, storage.IterOptions{ + iter, err := engine.NewMVCCIterator(storage.MVCCKeyAndIntentsIterKind, storage.IterOptions{ KeyTypes: storage.IterKeyTypeRangesOnly, LowerBound: rangeKey.StartKey, UpperBound: rangeKey.EndKey, }) + if err != nil { + t.Fatal(err) + } defer iter.Close() iter.SeekGE(storage.MVCCKey{Key: rangeKey.StartKey}) diff --git a/pkg/kv/kvserver/batcheval/cmd_end_transaction.go b/pkg/kv/kvserver/batcheval/cmd_end_transaction.go index e8b0b1deecaa..241058185746 100644 --- a/pkg/kv/kvserver/batcheval/cmd_end_transaction.go +++ b/pkg/kv/kvserver/batcheval/cmd_end_transaction.go @@ -1410,11 +1410,14 @@ func computeSplitRangeKeyStatsDelta( splitKey.Prevish(roachpb.PrevishKeyLength), splitKey.Next(), lhs.StartKey.AsRawKey(), rhs.EndKey.AsRawKey()) - iter := r.NewMVCCIterator(storage.MVCCKeyIterKind, storage.IterOptions{ + iter, err := r.NewMVCCIterator(storage.MVCCKeyIterKind, storage.IterOptions{ KeyTypes: storage.IterKeyTypeRangesOnly, LowerBound: leftPeekBound, UpperBound: rightPeekBound, }) + if err != nil { + return ms, err + } defer iter.Close() if cmp, rangeKeys, err := storage.PeekRangeKeysRight(iter, splitKey); err != nil { diff --git a/pkg/kv/kvserver/batcheval/cmd_export_test.go b/pkg/kv/kvserver/batcheval/cmd_export_test.go index cb678f0ca05c..df075d980c88 100644 --- a/pkg/kv/kvserver/batcheval/cmd_export_test.go +++ b/pkg/kv/kvserver/batcheval/cmd_export_test.go @@ -530,7 +530,7 @@ func exportUsingGoIterator( return nil, nil } - iter := storage.NewMVCCIncrementalIterator(reader, storage.MVCCIncrementalIterOptions{ + iter := storage.NewMVCCIncrementalIterator(reader.(storage.ReaderWithMustIterators), storage.MVCCIncrementalIterOptions{ EndKey: endKey, StartTime: startTime, EndTime: endTime, diff --git a/pkg/kv/kvserver/batcheval/cmd_query_resolved_timestamp.go b/pkg/kv/kvserver/batcheval/cmd_query_resolved_timestamp.go index 943906906ad4..fbc56d39b33d 100644 --- a/pkg/kv/kvserver/batcheval/cmd_query_resolved_timestamp.go +++ b/pkg/kv/kvserver/batcheval/cmd_query_resolved_timestamp.go @@ -104,7 +104,10 @@ func computeMinIntentTimestamp( ) (hlc.Timestamp, []roachpb.Intent, error) { ltStart, _ := keys.LockTableSingleKey(span.Key, nil) ltEnd, _ := keys.LockTableSingleKey(span.EndKey, nil) - iter := reader.NewEngineIterator(storage.IterOptions{LowerBound: ltStart, UpperBound: ltEnd}) + iter, err := reader.NewEngineIterator(storage.IterOptions{LowerBound: ltStart, UpperBound: ltEnd}) + if err != nil { + return hlc.Timestamp{}, nil, err + } defer iter.Close() var meta enginepb.MVCCMetadata diff --git a/pkg/kv/kvserver/batcheval/cmd_refresh_range.go b/pkg/kv/kvserver/batcheval/cmd_refresh_range.go index 7f70ee798eaa..a3d3bf22562d 100644 --- a/pkg/kv/kvserver/batcheval/cmd_refresh_range.go +++ b/pkg/kv/kvserver/batcheval/cmd_refresh_range.go @@ -77,7 +77,7 @@ func refreshRange( // Construct an incremental iterator with the desired time bounds. Incremental // iterators will emit MVCC tombstones by default and will emit intents when // configured to do so (see IntentPolicy). - iter := storage.NewMVCCIncrementalIterator(reader, storage.MVCCIncrementalIterOptions{ + iter := storage.NewMVCCIncrementalIterator(reader.(storage.ReaderWithMustIterators), storage.MVCCIncrementalIterOptions{ KeyTypes: storage.IterKeyTypePointsAndRanges, StartKey: span.Key, EndKey: span.EndKey, diff --git a/pkg/kv/kvserver/batcheval/cmd_revert_range.go b/pkg/kv/kvserver/batcheval/cmd_revert_range.go index d9169f211ac5..22a087e8db7d 100644 --- a/pkg/kv/kvserver/batcheval/cmd_revert_range.go +++ b/pkg/kv/kvserver/batcheval/cmd_revert_range.go @@ -78,13 +78,16 @@ func isEmptyKeyTimeRange( // may not be in the time range but the fact the TBI found any key indicates // that there is *a* key in the SST that is in the time range. Thus we should // proceed to iteration that actually checks timestamps on each key. - iter := readWriter.NewMVCCIterator(storage.MVCCKeyIterKind, storage.IterOptions{ + iter, err := readWriter.NewMVCCIterator(storage.MVCCKeyIterKind, storage.IterOptions{ KeyTypes: storage.IterKeyTypePointsAndRanges, LowerBound: from, UpperBound: to, MinTimestampHint: since.Next(), // make exclusive MaxTimestampHint: until, }) + if err != nil { + return false, err + } defer iter.Close() iter.SeekGE(storage.MVCCKey{Key: from}) ok, err := iter.Valid() diff --git a/pkg/kv/kvserver/batcheval/intent_test.go b/pkg/kv/kvserver/batcheval/intent_test.go index f50ed9dfb896..f25c842cfa0c 100644 --- a/pkg/kv/kvserver/batcheval/intent_test.go +++ b/pkg/kv/kvserver/batcheval/intent_test.go @@ -37,7 +37,7 @@ type instrumentedEngine struct { func (ie *instrumentedEngine) NewMVCCIterator( iterKind storage.MVCCIterKind, opts storage.IterOptions, -) storage.MVCCIterator { +) (storage.MVCCIterator, error) { if ie.onNewIterator != nil { ie.onNewIterator(opts) } diff --git a/pkg/kv/kvserver/client_manual_proposal_test.go b/pkg/kv/kvserver/client_manual_proposal_test.go index eeb6f3e6a0c6..477005e5e66b 100644 --- a/pkg/kv/kvserver/client_manual_proposal_test.go +++ b/pkg/kv/kvserver/client_manual_proposal_test.go @@ -117,7 +117,8 @@ LIMIT // Determine LastIndex, LastTerm, and next MaxLeaseIndex by scanning // existing log. - it := raftlog.NewIterator(rangeID, eng, raftlog.IterOptions{}) + it, err := raftlog.NewIterator(rangeID, eng, raftlog.IterOptions{}) + require.NoError(t, err) defer it.Close() rsl := logstore.NewStateLoader(rangeID) lastIndex, err := rsl.LoadLastIndex(ctx, eng) diff --git a/pkg/kv/kvserver/client_raft_test.go b/pkg/kv/kvserver/client_raft_test.go index 42c93ef28506..ab011604d2e5 100644 --- a/pkg/kv/kvserver/client_raft_test.go +++ b/pkg/kv/kvserver/client_raft_test.go @@ -5934,11 +5934,14 @@ func TestRaftSnapshotsWithMVCCRangeKeysEverywhere(t *testing.T) { for _, span := range rditer.MakeReplicatedKeySpans(&desc) { prefix := append(span.Key.Clone(), ':') - iter := e.NewEngineIterator(storage.IterOptions{ + iter, err := e.NewEngineIterator(storage.IterOptions{ KeyTypes: storage.IterKeyTypeRangesOnly, LowerBound: span.Key, UpperBound: span.EndKey, }) + if err != nil { + t.Fatal(err) + } defer iter.Close() ok, err := iter.SeekEngineKeyGE(storage.EngineKey{Key: span.Key}) diff --git a/pkg/kv/kvserver/client_split_test.go b/pkg/kv/kvserver/client_split_test.go index c2f548d6fc72..54f005e05009 100644 --- a/pkg/kv/kvserver/client_split_test.go +++ b/pkg/kv/kvserver/client_split_test.go @@ -386,7 +386,10 @@ func TestStoreRangeSplitIntents(t *testing.T) { // Verify the transaction record is gone. start := storage.MakeMVCCMetadataKey(keys.MakeRangeKeyPrefix(roachpb.RKeyMin)) end := storage.MakeMVCCMetadataKey(keys.MakeRangeKeyPrefix(roachpb.RKeyMax)) - iter := store.TODOEngine().NewMVCCIterator(storage.MVCCKeyAndIntentsIterKind, storage.IterOptions{UpperBound: end.Key}) + iter, err := store.TODOEngine().NewMVCCIterator(storage.MVCCKeyAndIntentsIterKind, storage.IterOptions{UpperBound: end.Key}) + if err != nil { + t.Fatal(err) + } defer iter.Close() for iter.SeekGE(start); ; iter.Next() { diff --git a/pkg/kv/kvserver/gc/gc_iterator_test.go b/pkg/kv/kvserver/gc/gc_iterator_test.go index 6bb97267b3e5..49a58f6ede32 100644 --- a/pkg/kv/kvserver/gc/gc_iterator_test.go +++ b/pkg/kv/kvserver/gc/gc_iterator_test.go @@ -163,11 +163,14 @@ func TestGCIterator(t *testing.T) { ds.setupTest(t, eng, desc) snap := eng.NewSnapshot() defer snap.Close() - mvccIt := snap.NewMVCCIterator(storage.MVCCKeyAndIntentsIterKind, storage.IterOptions{ + mvccIt, err := snap.NewMVCCIterator(storage.MVCCKeyAndIntentsIterKind, storage.IterOptions{ LowerBound: desc.StartKey.AsRawKey(), UpperBound: desc.EndKey.AsRawKey(), KeyTypes: storage.IterKeyTypePointsAndRanges, }) + if err != nil { + t.Fatal(err) + } mvccIt.SeekLT(storage.MVCCKey{Key: desc.EndKey.AsRawKey()}) defer mvccIt.Close() it := makeGCIterator(mvccIt, tc.gcThreshold) diff --git a/pkg/kv/kvserver/gc/gc_random_test.go b/pkg/kv/kvserver/gc/gc_random_test.go index b7e0a5d710e9..08fda398cca4 100644 --- a/pkg/kv/kvserver/gc/gc_random_test.go +++ b/pkg/kv/kvserver/gc/gc_random_test.go @@ -430,23 +430,25 @@ func assertLiveData( GCTTL: gcTTL, Threshold: gcThreshold, } - pointIt := before.NewMVCCIterator(storage.MVCCKeyAndIntentsIterKind, + pointIt, err := before.NewMVCCIterator(storage.MVCCKeyAndIntentsIterKind, storage.IterOptions{ LowerBound: desc.StartKey.AsRawKey(), UpperBound: desc.EndKey.AsRawKey(), KeyTypes: storage.IterKeyTypePointsAndRanges, }) + require.NoError(t, err) defer pointIt.Close() pointIt.SeekGE(storage.MVCCKey{Key: desc.StartKey.AsRawKey()}) pointExpectationsGenerator := getExpectationsGenerator(t, pointIt, gcThreshold, intentThreshold, &expInfo) - rangeIt := before.NewMVCCIterator(storage.MVCCKeyIterKind, + rangeIt, err := before.NewMVCCIterator(storage.MVCCKeyIterKind, storage.IterOptions{ LowerBound: desc.StartKey.AsRawKey(), UpperBound: desc.EndKey.AsRawKey(), KeyTypes: storage.IterKeyTypeRangesOnly, }) + require.NoError(t, err) defer rangeIt.Close() rangeIt.SeekGE(storage.MVCCKey{Key: desc.StartKey.AsRawKey()}) expectedRanges := mergeRanges(filterRangeFragments(rangeFragmentsFromIt(t, rangeIt), gcThreshold, @@ -454,11 +456,12 @@ func assertLiveData( // Loop over engine data after applying GCer requests and compare with // expected point keys. - itAfter := after.NewMVCCIterator(storage.MVCCKeyAndIntentsIterKind, storage.IterOptions{ + itAfter, err := after.NewMVCCIterator(storage.MVCCKeyAndIntentsIterKind, storage.IterOptions{ LowerBound: desc.StartKey.AsRawKey(), UpperBound: desc.EndKey.AsRawKey(), KeyTypes: storage.IterKeyTypePointsOnly, }) + require.NoError(t, err) defer itAfter.Close() itAfter.SeekGE(storage.MVCCKey{Key: desc.StartKey.AsRawKey()}) @@ -491,12 +494,13 @@ func assertLiveData( } } - rangeItAfter := after.NewMVCCIterator(storage.MVCCKeyIterKind, storage.IterOptions{ + rangeItAfter, err := after.NewMVCCIterator(storage.MVCCKeyIterKind, storage.IterOptions{ LowerBound: desc.StartKey.AsRawKey(), UpperBound: desc.EndKey.AsRawKey(), KeyTypes: storage.IterKeyTypeRangesOnly, RangeKeyMaskingBelow: hlc.Timestamp{}, }) + require.NoError(t, err) defer rangeItAfter.Close() rangeItAfter.SeekGE(storage.MVCCKey{Key: desc.StartKey.AsRawKey()}) actualRanges := mergeRanges(rangeFragmentsFromIt(t, rangeItAfter)) @@ -666,12 +670,13 @@ func getExpectationsGenerator( func getKeyHistory(t *testing.T, r storage.Reader, key roachpb.Key) string { var result []string - it := r.NewMVCCIterator(storage.MVCCKeyAndIntentsIterKind, storage.IterOptions{ + it, err := r.NewMVCCIterator(storage.MVCCKeyAndIntentsIterKind, storage.IterOptions{ LowerBound: key, UpperBound: key.Next(), KeyTypes: storage.IterKeyTypePointsAndRanges, RangeKeyMaskingBelow: hlc.Timestamp{}, }) + require.NoError(t, err) defer it.Close() it.SeekGE(storage.MVCCKey{Key: key}) diff --git a/pkg/kv/kvserver/gc/gc_test.go b/pkg/kv/kvserver/gc/gc_test.go index 97facbd32e7a..9eebe2ae2e6f 100644 --- a/pkg/kv/kvserver/gc/gc_test.go +++ b/pkg/kv/kvserver/gc/gc_test.go @@ -1142,21 +1142,23 @@ func requireEqualReaders( ) { // First compare only points. We assert points and ranges separately for // simplicity. - itExp := exected.NewMVCCIterator(storage.MVCCKeyIterKind, storage.IterOptions{ + itExp, err := exected.NewMVCCIterator(storage.MVCCKeyIterKind, storage.IterOptions{ LowerBound: desc.StartKey.AsRawKey(), UpperBound: desc.EndKey.AsRawKey(), KeyTypes: storage.IterKeyTypePointsOnly, RangeKeyMaskingBelow: hlc.Timestamp{}, }) + require.NoError(t, err) defer itExp.Close() itExp.SeekGE(storage.MVCCKey{Key: desc.StartKey.AsRawKey()}) - itActual := actual.NewMVCCIterator(storage.MVCCKeyIterKind, storage.IterOptions{ + itActual, err := actual.NewMVCCIterator(storage.MVCCKeyIterKind, storage.IterOptions{ LowerBound: desc.StartKey.AsRawKey(), UpperBound: desc.EndKey.AsRawKey(), KeyTypes: storage.IterKeyTypePointsOnly, RangeKeyMaskingBelow: hlc.Timestamp{}, }) + require.NoError(t, err) defer itActual.Close() itActual.SeekGE(storage.MVCCKey{Key: desc.StartKey.AsRawKey()}) @@ -1190,21 +1192,23 @@ func requireEqualReaders( } // Compare only ranges. - itExpRanges := exected.NewMVCCIterator(storage.MVCCKeyIterKind, storage.IterOptions{ + itExpRanges, err := exected.NewMVCCIterator(storage.MVCCKeyIterKind, storage.IterOptions{ LowerBound: desc.StartKey.AsRawKey(), UpperBound: desc.EndKey.AsRawKey(), KeyTypes: storage.IterKeyTypeRangesOnly, RangeKeyMaskingBelow: hlc.Timestamp{}, }) + require.NoError(t, err) defer itExpRanges.Close() itExpRanges.SeekGE(storage.MVCCKey{Key: desc.StartKey.AsRawKey()}) - itActualRanges := actual.NewMVCCIterator(storage.MVCCKeyIterKind, storage.IterOptions{ + itActualRanges, err := actual.NewMVCCIterator(storage.MVCCKeyIterKind, storage.IterOptions{ LowerBound: desc.StartKey.AsRawKey(), UpperBound: desc.EndKey.AsRawKey(), KeyTypes: storage.IterKeyTypeRangesOnly, RangeKeyMaskingBelow: hlc.Timestamp{}, }) + require.NoError(t, err) defer itActualRanges.Close() itActualRanges.SeekGE(storage.MVCCKey{Key: desc.StartKey.AsRawKey()}) @@ -1430,12 +1434,13 @@ func (d tableData) liveDistribution() dataDistribution { func engineData(t *testing.T, r storage.Reader, desc roachpb.RangeDescriptor) []tableCell { var result []tableCell - rangeIt := r.NewMVCCIterator(storage.MVCCKeyIterKind, storage.IterOptions{ + rangeIt, err := r.NewMVCCIterator(storage.MVCCKeyIterKind, storage.IterOptions{ LowerBound: desc.StartKey.AsRawKey(), UpperBound: desc.EndKey.AsRawKey(), KeyTypes: storage.IterKeyTypeRangesOnly, RangeKeyMaskingBelow: hlc.Timestamp{}, }) + require.NoError(t, err) defer rangeIt.Close() rangeIt.SeekGE(storage.MVCCKey{Key: desc.StartKey.AsRawKey()}) makeRangeCells := func(rks []storage.MVCCRangeKey) (tc []tableCell) { @@ -1505,12 +1510,13 @@ func engineData(t *testing.T, r storage.Reader, desc roachpb.RangeDescriptor) [] } result = append(result, makeRangeCells(partialRangeKeys)...) - it := r.NewMVCCIterator(storage.MVCCKeyAndIntentsIterKind, storage.IterOptions{ + it, err := r.NewMVCCIterator(storage.MVCCKeyAndIntentsIterKind, storage.IterOptions{ LowerBound: desc.StartKey.AsRawKey(), UpperBound: desc.EndKey.AsRawKey(), KeyTypes: storage.IterKeyTypePointsOnly, RangeKeyMaskingBelow: hlc.Timestamp{}, }) + require.NoError(t, err) defer it.Close() it.SeekGE(storage.MVCCKey{Key: desc.StartKey.AsRawKey()}) prefix := "" diff --git a/pkg/kv/kvserver/kvstorage/init.go b/pkg/kv/kvserver/kvstorage/init.go index 68ecdb81d16e..e858aa0a8f94 100644 --- a/pkg/kv/kvserver/kvstorage/init.go +++ b/pkg/kv/kvserver/kvstorage/init.go @@ -98,10 +98,13 @@ func checkCanInitializeEngine(ctx context.Context, eng storage.Engine) error { // // We use an EngineIterator to ensure that there are no keys that cannot be // parsed as MVCCKeys (e.g. lock table keys) in the engine. - iter := eng.NewEngineIterator(storage.IterOptions{ + iter, err := eng.NewEngineIterator(storage.IterOptions{ KeyTypes: storage.IterKeyTypePointsAndRanges, UpperBound: roachpb.KeyMax, }) + if err != nil { + return err + } defer iter.Close() valid, err := iter.SeekEngineKeyGE(storage.EngineKey{Key: roachpb.KeyMin}) if err != nil { @@ -160,9 +163,12 @@ func IterateIDPrefixKeys( ) error { rangeID := roachpb.RangeID(1) // NB: Range-ID local keys have no versions and no intents. - iter := reader.NewMVCCIterator(storage.MVCCKeyIterKind, storage.IterOptions{ + iter, err := reader.NewMVCCIterator(storage.MVCCKeyIterKind, storage.IterOptions{ UpperBound: keys.LocalRangeIDPrefix.PrefixEnd().AsRawKey(), }) + if err != nil { + return err + } defer iter.Close() for { diff --git a/pkg/kv/kvserver/logstore/stateloader.go b/pkg/kv/kvserver/logstore/stateloader.go index a3c384b58905..2f042961708c 100644 --- a/pkg/kv/kvserver/logstore/stateloader.go +++ b/pkg/kv/kvserver/logstore/stateloader.go @@ -57,7 +57,10 @@ func (sl StateLoader) LoadLastIndex( ) (kvpb.RaftIndex, error) { prefix := sl.RaftLogPrefix() // NB: raft log has no intents. - iter := reader.NewMVCCIterator(storage.MVCCKeyIterKind, storage.IterOptions{LowerBound: prefix}) + iter, err := reader.NewMVCCIterator(storage.MVCCKeyIterKind, storage.IterOptions{LowerBound: prefix}) + if err != nil { + return 0, err + } defer iter.Close() var lastIndex kvpb.RaftIndex diff --git a/pkg/kv/kvserver/loqrecovery/record.go b/pkg/kv/kvserver/loqrecovery/record.go index 76199429b13a..1e73a84a8af7 100644 --- a/pkg/kv/kvserver/loqrecovery/record.go +++ b/pkg/kv/kvserver/loqrecovery/record.go @@ -75,11 +75,14 @@ func RegisterOfflineRecoveryEvents( successCount := 0 var processingErrors error - iter := readWriter.NewMVCCIterator( + iter, err := readWriter.NewMVCCIterator( storage.MVCCKeyIterKind, storage.IterOptions{ LowerBound: keys.LocalStoreUnsafeReplicaRecoveryKeyMin, UpperBound: keys.LocalStoreUnsafeReplicaRecoveryKeyMax, }) + if err != nil { + return 0, err + } defer iter.Close() iter.SeekGE(storage.MVCCKey{Key: keys.LocalStoreUnsafeReplicaRecoveryKeyMin}) diff --git a/pkg/kv/kvserver/raft_log_truncator_test.go b/pkg/kv/kvserver/raft_log_truncator_test.go index 1c0c83d29bc3..50cdf1388ae9 100644 --- a/pkg/kv/kvserver/raft_log_truncator_test.go +++ b/pkg/kv/kvserver/raft_log_truncator_test.go @@ -202,9 +202,12 @@ func (r *replicaTruncatorTest) printEngine(t *testing.T, eng storage.Engine) { require.NoError(t, err) fmt.Fprintf(r.buf, "truncated index: %d\n", truncState.Index) prefix := r.stateLoader.RaftLogPrefix() - iter := eng.NewMVCCIterator(storage.MVCCKeyIterKind, storage.IterOptions{ + iter, err := eng.NewMVCCIterator(storage.MVCCKeyIterKind, storage.IterOptions{ UpperBound: r.stateLoader.RaftLogKey(math.MaxUint64), }) + if err != nil { + t.Fatal(err) + } defer iter.Close() iter.SeekGE(storage.MVCCKey{Key: r.stateLoader.RaftLogKey(0)}) valid, err := iter.Valid() diff --git a/pkg/kv/kvserver/raftlog/iter_bench_test.go b/pkg/kv/kvserver/raftlog/iter_bench_test.go index c6391397f805..8c5892a8296e 100644 --- a/pkg/kv/kvserver/raftlog/iter_bench_test.go +++ b/pkg/kv/kvserver/raftlog/iter_bench_test.go @@ -56,8 +56,8 @@ type mockReader struct { func (m *mockReader) NewMVCCIterator( storage.MVCCIterKind, storage.IterOptions, -) storage.MVCCIterator { - return m.iter +) (storage.MVCCIterator, error) { + return m.iter, nil } func mkRaftCommand(keySize, valSize, writeBatchSize int) *kvserverpb.RaftCommand { @@ -138,14 +138,20 @@ func BenchmarkIterator(b *testing.B) { b.ReportAllocs() b.ResetTimer() for i := 0; i < b.N; i++ { - it := NewIterator(rangeID, &mockReader{}, IterOptions{Hi: 123456}) + it, err := NewIterator(rangeID, &mockReader{}, IterOptions{Hi: 123456}) + if err != nil { + b.Fatal(err) + } setMockIter(it) it.Close() } }) benchForOp := func(b *testing.B, method func(*Iterator) (bool, error)) { - it := NewIterator(rangeID, &mockReader{}, IterOptions{Hi: 123456}) + it, err := NewIterator(rangeID, &mockReader{}, IterOptions{Hi: 123456}) + if err != nil { + b.Fatal(err) + } setMockIter(it) b.ReportAllocs() diff --git a/pkg/kv/kvserver/raftlog/iter_test.go b/pkg/kv/kvserver/raftlog/iter_test.go index 93bc05d3dc6a..e5329e1d2d07 100644 --- a/pkg/kv/kvserver/raftlog/iter_test.go +++ b/pkg/kv/kvserver/raftlog/iter_test.go @@ -154,7 +154,8 @@ func TestIteratorEmptyLog(t *testing.T) { eng := storage.NewDefaultInMemForTesting() for _, hi := range []kvpb.RaftIndex{0, 1} { - it := NewIterator(rangeID, eng, IterOptions{Hi: hi}) + it, err := NewIterator(rangeID, eng, IterOptions{Hi: hi}) + require.NoError(t, err) ok, err := it.SeekGE(0) it.Close() require.NoError(t, err) @@ -249,7 +250,8 @@ func TestIterator(t *testing.T) { hi = 0 } t.Run(fmt.Sprintf("lo=%s,hi=%s", indToName(lo), indToName(hi)), func(t *testing.T) { - it := NewIterator(rangeID, eng, IterOptions{Hi: hi}) + it, err := NewIterator(rangeID, eng, IterOptions{Hi: hi}) + require.NoError(t, err) sl, err := consumeIter(it, lo) it.Close() require.NoError(t, err) diff --git a/pkg/kv/kvserver/raftlog/iterator.go b/pkg/kv/kvserver/raftlog/iterator.go index b6a75dade76b..35fc682557d9 100644 --- a/pkg/kv/kvserver/raftlog/iterator.go +++ b/pkg/kv/kvserver/raftlog/iterator.go @@ -32,7 +32,7 @@ type storageIter interface { // The raft log is a contiguous sequence of indexes (i.e. no holes) which may be // empty. type Reader interface { - NewMVCCIterator(storage.MVCCIterKind, storage.IterOptions) storage.MVCCIterator + NewMVCCIterator(storage.MVCCIterKind, storage.IterOptions) (storage.MVCCIterator, error) } // An Iterator inspects the raft log. After creation, SeekGE should be invoked, @@ -68,7 +68,7 @@ type IterOptions struct { // RangeID from the provided Reader. // // Callers that can afford allocating a closure may prefer using Visit. -func NewIterator(rangeID roachpb.RangeID, eng Reader, opts IterOptions) *Iterator { +func NewIterator(rangeID roachpb.RangeID, eng Reader, opts IterOptions) (*Iterator, error) { // TODO(tbg): can pool these most of the things below, incl. the *Iterator. prefixBuf := keys.MakeRangeIDPrefixBuf(rangeID) var upperBound roachpb.Key @@ -77,13 +77,17 @@ func NewIterator(rangeID roachpb.RangeID, eng Reader, opts IterOptions) *Iterato } else { upperBound = prefixBuf.RaftLogKey(opts.Hi) } + iter, err := eng.NewMVCCIterator(storage.MVCCKeyIterKind, storage.IterOptions{ + UpperBound: upperBound, + }) + if err != nil { + return nil, err + } return &Iterator{ eng: eng, prefixBuf: prefixBuf, - iter: eng.NewMVCCIterator(storage.MVCCKeyIterKind, storage.IterOptions{ - UpperBound: upperBound, - }), - } + iter: iter, + }, nil } // Close releases the resources associated with this Iterator. @@ -138,7 +142,10 @@ func (it *Iterator) Entry() raftpb.Entry { func Visit( eng Reader, rangeID roachpb.RangeID, lo, hi kvpb.RaftIndex, fn func(raftpb.Entry) error, ) error { - it := NewIterator(rangeID, eng, IterOptions{Hi: hi}) + it, err := NewIterator(rangeID, eng, IterOptions{Hi: hi}) + if err != nil { + return err + } defer it.Close() ok, err := it.SeekGE(lo) if err != nil { diff --git a/pkg/kv/kvserver/rangefeed/catchup_scan.go b/pkg/kv/kvserver/rangefeed/catchup_scan.go index 1432aeaddb5d..990ea25ba359 100644 --- a/pkg/kv/kvserver/rangefeed/catchup_scan.go +++ b/pkg/kv/kvserver/rangefeed/catchup_scan.go @@ -84,7 +84,7 @@ func NewCatchUpIterator( pacer *admission.Pacer, ) *CatchUpIterator { return &CatchUpIterator{ - simpleCatchupIter: storage.NewMVCCIncrementalIterator(reader, + simpleCatchupIter: storage.NewMVCCIncrementalIterator(reader.(storage.ReaderWithMustIterators), storage.MVCCIncrementalIterOptions{ KeyTypes: storage.IterKeyTypePointsAndRanges, StartKey: span.Key, diff --git a/pkg/kv/kvserver/rangefeed/task_test.go b/pkg/kv/kvserver/rangefeed/task_test.go index 91c71009fd57..07c7230e3b75 100644 --- a/pkg/kv/kvserver/rangefeed/task_test.go +++ b/pkg/kv/kvserver/rangefeed/task_test.go @@ -324,9 +324,12 @@ func TestInitResolvedTSScan(t *testing.T) { "legacy intent scanner": { intentScanner: func() (IntentScanner, func()) { engine := makeEngine() - iter := engine.NewMVCCIterator(storage.MVCCKeyAndIntentsIterKind, storage.IterOptions{ + iter, err := engine.NewMVCCIterator(storage.MVCCKeyAndIntentsIterKind, storage.IterOptions{ UpperBound: endKey.AsRawKey(), }) + if err != nil { + t.Fatal(err) + } return NewLegacyIntentScanner(iter), func() { engine.Close() } }, }, @@ -335,10 +338,13 @@ func TestInitResolvedTSScan(t *testing.T) { engine := makeEngine() lowerBound, _ := keys.LockTableSingleKey(startKey.AsRawKey(), nil) upperBound, _ := keys.LockTableSingleKey(endKey.AsRawKey(), nil) - iter := engine.NewEngineIterator(storage.IterOptions{ + iter, err := engine.NewEngineIterator(storage.IterOptions{ LowerBound: lowerBound, UpperBound: upperBound, }) + if err != nil { + t.Fatal(err) + } return NewSeparatedIntentScanner(iter), func() { engine.Close() } }, }, diff --git a/pkg/kv/kvserver/rditer/replica_data_iter.go b/pkg/kv/kvserver/rditer/replica_data_iter.go index 55da433f215a..ff77be7368b7 100644 --- a/pkg/kv/kvserver/rditer/replica_data_iter.go +++ b/pkg/kv/kvserver/rditer/replica_data_iter.go @@ -183,13 +183,18 @@ func (ri *ReplicaMVCCDataIterator) tryCloseAndCreateIter() { if ri.curIndex < 0 || ri.curIndex >= len(ri.spans) { return } - ri.it = ri.reader.NewMVCCIterator( + var err error + ri.it, err = ri.reader.NewMVCCIterator( ri.IterKind, storage.IterOptions{ LowerBound: ri.spans[ri.curIndex].Key, UpperBound: ri.spans[ri.curIndex].EndKey, KeyTypes: ri.KeyTypes, }) + if err != nil { + ri.err = err + return + } if ri.Reverse { ri.it.SeekLT(storage.MakeMVCCMetadataKey(ri.spans[ri.curIndex].EndKey)) } else { @@ -347,11 +352,14 @@ func IterateReplicaKeySpans( for _, span := range spans { for _, keyType := range keyTypes { err := func() error { - iter := reader.NewEngineIterator(storage.IterOptions{ + iter, err := reader.NewEngineIterator(storage.IterOptions{ KeyTypes: keyType, LowerBound: span.Key, UpperBound: span.EndKey, }) + if err != nil { + return err + } defer iter.Close() ok, err := iter.SeekEngineKeyGE(storage.EngineKey{Key: span.Key}) if err == nil && ok { @@ -435,11 +443,14 @@ func IterateMVCCReplicaKeySpans( for _, span := range spans { for _, keyType := range keyTypes { err := func() error { - iter := reader.NewMVCCIterator(storage.MVCCKeyAndIntentsIterKind, storage.IterOptions{ + iter, err := reader.NewMVCCIterator(storage.MVCCKeyAndIntentsIterKind, storage.IterOptions{ LowerBound: span.Key, UpperBound: span.EndKey, KeyTypes: keyType, }) + if err != nil { + return err + } defer iter.Close() if options.Reverse { iter.SeekLT(storage.MakeMVCCMetadataKey(span.EndKey)) diff --git a/pkg/kv/kvserver/replica_evaluate.go b/pkg/kv/kvserver/replica_evaluate.go index 7e6becf70301..f16580598328 100644 --- a/pkg/kv/kvserver/replica_evaluate.go +++ b/pkg/kv/kvserver/replica_evaluate.go @@ -47,7 +47,7 @@ import ( // mutating the original requests). func optimizePuts( reader storage.Reader, origReqs []kvpb.RequestUnion, distinctSpans bool, -) []kvpb.RequestUnion { +) ([]kvpb.RequestUnion, error) { var minKey, maxKey roachpb.Key var unique map[string]struct{} if !distinctSpans { @@ -95,17 +95,20 @@ func optimizePuts( } if firstUnoptimizedIndex < optimizePutThreshold { // don't bother if below this threshold - return origReqs + return origReqs, nil } // iter is being used to find the parts of the key range that is empty. We // don't need to see intents for this purpose since intents also have // provisional values that we will see. - iter := reader.NewMVCCIterator(storage.MVCCKeyIterKind, storage.IterOptions{ + iter, err := reader.NewMVCCIterator(storage.MVCCKeyIterKind, storage.IterOptions{ KeyTypes: storage.IterKeyTypePointsAndRanges, // We want to include maxKey in our scan. Since UpperBound is exclusive, we // need to set it to the key after maxKey. UpperBound: maxKey.Next(), }) + if err != nil { + return nil, err + } defer iter.Close() // If there are enough puts in the run to justify calling seek, @@ -118,7 +121,7 @@ func optimizePuts( // TODO(bdarnell): return an error here instead of silently // running without the optimization? log.Errorf(context.TODO(), "Seek returned error; disabling blind-put optimization: %+v", err) - return origReqs + return origReqs, nil } else if ok && bytes.Compare(iter.UnsafeKey().Key, maxKey) <= 0 { iterKey = iter.UnsafeKey().Key.Clone() } @@ -146,7 +149,7 @@ func optimizePuts( } } } - return reqs + return reqs, nil } // evaluateBatch evaluates a batch request by splitting it up into its @@ -183,10 +186,15 @@ func evaluateBatch( baHeader := ba.Header br := ba.CreateReply() + var err error // Optimize any contiguous sequences of put and conditional put ops. if len(baReqs) >= optimizePutThreshold && evalPath == readWrite { - baReqs = optimizePuts(readWriter, baReqs, baHeader.DistinctSpans) + baReqs, err = optimizePuts(readWriter, baReqs, baHeader.DistinctSpans) + } + if err != nil { + pErr := kvpb.NewErrorWithTxn(err, baHeader.Txn) + return nil, result.Result{}, pErr } // Create a clone of the transaction to store the new txn state produced on diff --git a/pkg/kv/kvserver/replica_raft.go b/pkg/kv/kvserver/replica_raft.go index 78bd0e3f7bda..fe71cf3ac37f 100644 --- a/pkg/kv/kvserver/replica_raft.go +++ b/pkg/kv/kvserver/replica_raft.go @@ -2709,7 +2709,10 @@ func (r *Replica) printRaftTail( end := keys.RaftLogPrefix(r.RangeID).PrefixEnd() // NB: raft log does not have intents. - it := r.store.TODOEngine().NewEngineIterator(storage.IterOptions{LowerBound: start, UpperBound: end}) + it, err := r.store.TODOEngine().NewEngineIterator(storage.IterOptions{LowerBound: start, UpperBound: end}) + if err != nil { + return "", err + } valid, err := it.SeekEngineKeyLT(storage.EngineKey{Key: end}) if err != nil { return "", err diff --git a/pkg/kv/kvserver/replica_rangefeed.go b/pkg/kv/kvserver/replica_rangefeed.go index eb4d8d5e4d80..01b60d887e8c 100644 --- a/pkg/kv/kvserver/replica_rangefeed.go +++ b/pkg/kv/kvserver/replica_rangefeed.go @@ -405,10 +405,14 @@ func (r *Replica) registerWithRangefeedRaftMuLocked( lowerBound, _ := keys.LockTableSingleKey(desc.StartKey.AsRawKey(), nil) upperBound, _ := keys.LockTableSingleKey(desc.EndKey.AsRawKey(), nil) - iter := r.store.TODOEngine().NewEngineIterator(storage.IterOptions{ + iter, err := r.store.TODOEngine().NewEngineIterator(storage.IterOptions{ LowerBound: lowerBound, UpperBound: upperBound, }) + if err != nil { + done.Set(err) + return nil + } return rangefeed.NewSeparatedIntentScanner(iter) } diff --git a/pkg/kv/kvserver/replica_test.go b/pkg/kv/kvserver/replica_test.go index 370cc585fd9e..a5b31127d915 100644 --- a/pkg/kv/kvserver/replica_test.go +++ b/pkg/kv/kvserver/replica_test.go @@ -1828,7 +1828,9 @@ func TestOptimizePuts(t *testing.T) { // Save the original slice, allowing us to assert that it doesn't // change when it is passed to optimizePuts. oldRequests := batch.Requests - batch.Requests = optimizePuts(tc.engine, batch.Requests, false) + var err error + batch.Requests, err = optimizePuts(tc.engine, batch.Requests, false) + require.NoError(t, err) if !reflect.DeepEqual(goldenRequests, oldRequests) { t.Fatalf("%d: optimizePuts mutated the original request slice: %s", i, pretty.Diff(goldenRequests, oldRequests), @@ -3117,7 +3119,10 @@ func TestReplicaTSCacheForwardsIntentTS(t *testing.T) { if _, pErr := tc.SendWrappedWith(kvpb.Header{Txn: txnOld}, &pArgs); pErr != nil { t.Fatal(pErr) } - iter := tc.engine.NewMVCCIterator(storage.MVCCKeyAndIntentsIterKind, storage.IterOptions{Prefix: true}) + iter, err := tc.engine.NewMVCCIterator(storage.MVCCKeyAndIntentsIterKind, storage.IterOptions{Prefix: true}) + if err != nil { + t.Fatal(err) + } defer iter.Close() mvccKey := storage.MakeMVCCMetadataKey(key) iter.SeekGE(mvccKey) diff --git a/pkg/kv/kvserver/spanset/batch.go b/pkg/kv/kvserver/spanset/batch.go index 8138cc2ccb1e..84e95bd5e88b 100644 --- a/pkg/kv/kvserver/spanset/batch.go +++ b/pkg/kv/kvserver/spanset/batch.go @@ -485,20 +485,46 @@ func (s spanSetReader) MVCCIterate( func (s spanSetReader) NewMVCCIterator( iterKind storage.MVCCIterKind, opts storage.IterOptions, -) storage.MVCCIterator { +) (storage.MVCCIterator, error) { + mvccIter, err := s.r.NewMVCCIterator(iterKind, opts) + if err != nil { + return nil, err + } if s.spansOnly { - return NewIterator(s.r.NewMVCCIterator(iterKind, opts), s.spans) + return NewIterator(mvccIter, s.spans), nil + } + return NewIteratorAt(mvccIter, s.spans, s.ts), nil +} + +func (s spanSetReader) MustMVCCIterator( + iterKind storage.MVCCIterKind, opts storage.IterOptions, +) storage.MVCCIterator { + iter, err := s.NewMVCCIterator(iterKind, opts) + if err != nil { + panic(err) } - return NewIteratorAt(s.r.NewMVCCIterator(iterKind, opts), s.spans, s.ts) + return iter } -func (s spanSetReader) NewEngineIterator(opts storage.IterOptions) storage.EngineIterator { +func (s spanSetReader) NewEngineIterator(opts storage.IterOptions) (storage.EngineIterator, error) { + engineIter, err := s.r.NewEngineIterator(opts) + if err != nil { + return nil, err + } return &EngineIterator{ - i: s.r.NewEngineIterator(opts), + i: engineIter, spans: s.spans, spansOnly: s.spansOnly, ts: s.ts, + }, nil +} + +func (s spanSetReader) MustEngineIterator(opts storage.IterOptions) storage.EngineIterator { + iter, err := s.NewEngineIterator(opts) + if err != nil { + panic(err) } + return iter } // ConsistentIterators implements the storage.Reader interface. diff --git a/pkg/server/init.go b/pkg/server/init.go index 9e406fffd2e1..f371b85563c0 100644 --- a/pkg/server/init.go +++ b/pkg/server/init.go @@ -587,10 +587,13 @@ func assertEnginesEmpty(engines []storage.Engine) error { for _, engine := range engines { err := func() error { - iter := engine.NewEngineIterator(storage.IterOptions{ + iter, err := engine.NewEngineIterator(storage.IterOptions{ KeyTypes: storage.IterKeyTypePointsAndRanges, UpperBound: roachpb.KeyMax, }) + if err != nil { + return err + } defer iter.Close() valid, err := iter.SeekEngineKeyGE(storage.EngineKey{Key: roachpb.KeyMin}) diff --git a/pkg/sql/row/fetcher_mvcc_test.go b/pkg/sql/row/fetcher_mvcc_test.go index 32e7d0e41e11..b65620109649 100644 --- a/pkg/sql/row/fetcher_mvcc_test.go +++ b/pkg/sql/row/fetcher_mvcc_test.go @@ -43,7 +43,10 @@ func slurpUserDataKVs(t testing.TB, e storage.Engine) []roachpb.KeyValue { var kvs []roachpb.KeyValue testutils.SucceedsSoon(t, func() error { kvs = nil - it := e.NewMVCCIterator(storage.MVCCKeyAndIntentsIterKind, storage.IterOptions{UpperBound: roachpb.KeyMax}) + it, err := e.NewMVCCIterator(storage.MVCCKeyAndIntentsIterKind, storage.IterOptions{UpperBound: roachpb.KeyMax}) + if err != nil { + t.Fatal(err) + } defer it.Close() for it.SeekGE(storage.MVCCKey{Key: bootstrap.TestingUserTableDataMin()}); ; it.NextKey() { ok, err := it.Valid() diff --git a/pkg/sql/tests/data.go b/pkg/sql/tests/data.go index dfb1dd32e53f..985902889e62 100644 --- a/pkg/sql/tests/data.go +++ b/pkg/sql/tests/data.go @@ -67,13 +67,16 @@ func CheckKeyCountIncludingTombstonedE( } keyCount := 0 - it := engines[0].NewMVCCIterator( + it, err := engines[0].NewMVCCIterator( storage.MVCCKeyIterKind, storage.IterOptions{ LowerBound: tableSpan.Key, UpperBound: tableSpan.EndKey, }, ) + if err != nil { + return err + } for it.SeekGE(storage.MVCCKey{Key: tableSpan.Key}); ; it.NextKey() { ok, err := it.Valid() diff --git a/pkg/storage/batch_test.go b/pkg/storage/batch_test.go index 7d9a07894da5..e86729c4f6d1 100644 --- a/pkg/storage/batch_test.go +++ b/pkg/storage/batch_test.go @@ -186,13 +186,17 @@ func TestReadOnlyBasics(t *testing.T) { _ = ro.MVCCIterate(a.Key, a.Key, MVCCKeyIterKind, IterKeyTypePointsOnly, func(MVCCKeyValue, MVCCRangeKeyStack) error { return iterutil.StopIteration() }) }, - func() { ro.NewMVCCIterator(MVCCKeyIterKind, IterOptions{UpperBound: roachpb.KeyMax}).Close() }, func() { - ro.NewMVCCIterator(MVCCKeyIterKind, IterOptions{ + iter, _ := ro.NewMVCCIterator(MVCCKeyIterKind, IterOptions{UpperBound: roachpb.KeyMax}) + iter.Close() + }, + func() { + iter, _ := ro.NewMVCCIterator(MVCCKeyIterKind, IterOptions{ MinTimestampHint: hlc.MinTimestamp, MaxTimestampHint: hlc.MaxTimestamp, UpperBound: roachpb.KeyMax, - }).Close() + }) + iter.Close() }, } defer func() { @@ -698,7 +702,10 @@ func TestUnindexedBatchThatSupportsReader(t *testing.T) { // Verify that reads on the distinct batch go to the underlying engine, not // to the unindexed batch. - iter := b.NewMVCCIterator(MVCCKeyIterKind, IterOptions{UpperBound: roachpb.KeyMax}) + iter, err := b.NewMVCCIterator(MVCCKeyIterKind, IterOptions{UpperBound: roachpb.KeyMax}) + if err != nil { + t.Fatal(err) + } iter.SeekGE(mvccKey("a")) if ok, err := iter.Valid(); !ok { t.Fatalf("expected iterator to be valid, err=%v", err) @@ -732,7 +739,7 @@ func TestWriteBatchPanicsAsReader(t *testing.T) { b := mvccKey("b") testCases := []func(){ func() { _ = r.MVCCIterate(a.Key, b.Key, MVCCKeyIterKind, IterKeyTypePointsOnly, nil) }, - func() { _ = r.NewMVCCIterator(MVCCKeyIterKind, IterOptions{UpperBound: roachpb.KeyMax}) }, + func() { _, _ = r.NewMVCCIterator(MVCCKeyIterKind, IterOptions{UpperBound: roachpb.KeyMax}) }, } for i, f := range testCases { func() { @@ -775,7 +782,10 @@ func TestBatchIteration(t *testing.T) { } iterOpts := IterOptions{UpperBound: k3.Key} - iter := b.NewMVCCIterator(MVCCKeyIterKind, iterOpts) + iter, err := b.NewMVCCIterator(MVCCKeyIterKind, iterOpts) + if err != nil { + t.Fatal(err) + } defer iter.Close() // Forward iteration, diff --git a/pkg/storage/bench_test.go b/pkg/storage/bench_test.go index 3fdcd1dcf39b..70fbbe4fde15 100644 --- a/pkg/storage/bench_test.go +++ b/pkg/storage/bench_test.go @@ -368,10 +368,13 @@ func BenchmarkIntentScan(b *testing.B) { setupKeysWithIntent(b, eng, numVersions, numFlushedVersions, false, /* resolveAll */ 1, false /* resolveIntentForLatestVersionWhenNotLockUpdate */) lower := makeKey(nil, 0) - iter := eng.NewMVCCIterator(MVCCKeyAndIntentsIterKind, IterOptions{ + iter, err := eng.NewMVCCIterator(MVCCKeyAndIntentsIterKind, IterOptions{ LowerBound: lower, UpperBound: makeKey(nil, numIntentKeys), }) + if err != nil { + b.Fatal(err) + } b.ResetTimer() for i := 0; i < b.N; i++ { valid, err := iter.Valid() @@ -454,10 +457,13 @@ func BenchmarkScanAllIntentsResolved(b *testing.B) { // practice, so we don't want it to happen in this Benchmark // either. b.StopTimer() - iter = eng.NewMVCCIterator(MVCCKeyAndIntentsIterKind, IterOptions{ + iter, err = eng.NewMVCCIterator(MVCCKeyAndIntentsIterKind, IterOptions{ LowerBound: lower, UpperBound: makeKey(nil, numIntentKeys), }) + if err != nil { + b.Fatal(err) + } b.StartTimer() iter.SeekGE(MVCCKey{Key: lower}) } else { @@ -499,10 +505,13 @@ func BenchmarkScanOneAllIntentsResolved(b *testing.B) { buf := append([]byte(nil), lower...) b.ResetTimer() for i := 0; i < b.N; i++ { - iter := eng.NewMVCCIterator(MVCCKeyAndIntentsIterKind, IterOptions{ + iter, err := eng.NewMVCCIterator(MVCCKeyAndIntentsIterKind, IterOptions{ LowerBound: buf, UpperBound: upper, }) + if err != nil { + b.Fatal(err) + } iter.SeekGE(MVCCKey{Key: buf}) valid, err := iter.Valid() if err != nil { @@ -1951,13 +1960,16 @@ func BenchmarkMVCCScannerWithIntentsAndVersions(b *testing.B) { ts := hlc.Timestamp{WallTime: int64(numVersions) + 5} startKey := makeKey(nil, 0) endKey := makeKey(nil, totalNumKeys+1) - iter := newMVCCIterator( + iter, err := newMVCCIterator( rw, ts, false, false, IterOptions{ KeyTypes: IterKeyTypePointsAndRanges, LowerBound: startKey, UpperBound: endKey, }, ) + if err != nil { + b.Fatal(err) + } res, err := mvccScanToKvs(ctx, iter, startKey, endKey, hlc.Timestamp{WallTime: int64(numVersions) + 5}, MVCCScanOptions{}) if err != nil { diff --git a/pkg/storage/col_mvcc.go b/pkg/storage/col_mvcc.go index bd2b65542e50..917593237f58 100644 --- a/pkg/storage/col_mvcc.go +++ b/pkg/storage/col_mvcc.go @@ -401,13 +401,16 @@ func MVCCScanToCols( opts MVCCScanOptions, st *cluster.Settings, ) (MVCCScanResult, error) { - iter := newMVCCIterator( + iter, err := newMVCCIterator( reader, timestamp, !opts.Tombstones, opts.DontInterleaveIntents, IterOptions{ KeyTypes: IterKeyTypePointsAndRanges, LowerBound: key, UpperBound: endKey, }, ) + if err != nil { + return MVCCScanResult{}, err + } defer iter.Close() return mvccScanToCols(ctx, iter, indexFetchSpec, key, endKey, timestamp, opts, st) } diff --git a/pkg/storage/disk_map.go b/pkg/storage/disk_map.go index bbb87b1a4db7..bae5f473f755 100644 --- a/pkg/storage/disk_map.go +++ b/pkg/storage/disk_map.go @@ -114,13 +114,18 @@ func (r *pebbleMap) makeKeyWithSequence(k []byte) []byte { // NewIterator implements the SortedDiskMap interface. func (r *pebbleMap) NewIterator() diskmap.SortedDiskMapIterator { + iter, err := r.store.NewIter(&pebble.IterOptions{ + UpperBound: roachpb.Key(r.prefix).PrefixEnd(), + }) + if err != nil { + // TODO(bilal): Update all diskMap interfaces to allow returning errors here. + panic(err) + } return &pebbleMapIterator{ allowDuplicates: r.allowDuplicates, - iter: r.store.NewIter(&pebble.IterOptions{ - UpperBound: roachpb.Key(r.prefix).PrefixEnd(), - }), - prefixLen: len(r.prefix), - makeKeyScratch: append([]byte{}, r.prefix...), + iter: iter, + prefixLen: len(r.prefix), + makeKeyScratch: append([]byte{}, r.prefix...), } } diff --git a/pkg/storage/disk_map_test.go b/pkg/storage/disk_map_test.go index d8c4da4dba15..02f8acc15267 100644 --- a/pkg/storage/disk_map_test.go +++ b/pkg/storage/disk_map_test.go @@ -44,7 +44,10 @@ func runTestForEngine(ctx context.Context, t *testing.T, filename string, engine // a type switch with implementation-specific code instead. switch e := engine.(type) { case *pebbleTempEngine: - iter := e.db.NewIter(&pebble.IterOptions{UpperBound: roachpb.KeyMax}) + iter, err := e.db.NewIter(&pebble.IterOptions{UpperBound: roachpb.KeyMax}) + if err != nil { + t.Fatal(err) + } defer func() { if err := iter.Close(); err != nil { diff --git a/pkg/storage/engine.go b/pkg/storage/engine.go index e3745244d76a..3d088a20b863 100644 --- a/pkg/storage/engine.go +++ b/pkg/storage/engine.go @@ -593,12 +593,12 @@ type Reader interface { // // 4. Iterators on indexed batches see all batch writes as of their creation // time, but they satisfy ConsistentIterators for engine writes. - NewMVCCIterator(iterKind MVCCIterKind, opts IterOptions) MVCCIterator + NewMVCCIterator(iterKind MVCCIterKind, opts IterOptions) (MVCCIterator, error) // NewEngineIterator returns a new instance of an EngineIterator over this // engine. The caller must invoke EngineIterator.Close() when finished // with the iterator to free resources. The caller can change IterOptions // after this function returns. - NewEngineIterator(opts IterOptions) EngineIterator + NewEngineIterator(opts IterOptions) (EngineIterator, error) // ScanInternal allows a caller to inspect the underlying engine's InternalKeys // using a visitor pattern, while also allowing for keys in shared files to be // skipped if a visitor is provided for visitSharedFiles. Useful for @@ -639,6 +639,25 @@ type Reader interface { PinEngineStateForIterators() error } +// ReaderWithMustIterators is a Reader that guarantees no errors during +// iterator creation. +// +// TODO(bilal): The only user of this interface is NewMVCCIncrementalIterator. +// Update that method to handle errors and remove this interface. +type ReaderWithMustIterators interface { + Reader + + // MustMVCCIterator is identical to NewMVCCIterator, except it is implemented + // only for those Reader implementations that do not return an error on + // iterator creation. + MustMVCCIterator(iterKind MVCCIterKind, opts IterOptions) MVCCIterator + + // MustEngineIterator is identical to NewEngineIterator, except it is + // implemented only for those Reader implementations that do not return an + // error on iterator creation. + MustEngineIterator(opts IterOptions) EngineIterator +} + // Writer is the write interface to an engine's data. type Writer interface { // ApplyBatchRepr atomically applies a set of batched updates. Created by @@ -950,7 +969,8 @@ const ( // Engine is the interface that wraps the core operations of a key/value store. type Engine interface { - ReadWriter + ReaderWithMustIterators + Writer // Attrs returns the engine/store attributes. Attrs() roachpb.Attributes // Capacity returns capacity details for the engine's available storage. @@ -1100,7 +1120,7 @@ type Batch interface { // iterator creation. To guarantee that they see all the mutations, the // iterator has to be repositioned using a seek operation, after the // mutations were done. - Reader + ReaderWithMustIterators WriteBatch } @@ -1380,7 +1400,10 @@ func GetIntent(reader Reader, key roachpb.Key) (*roachpb.Intent, error) { // used for queries. lbKey, _ := keys.LockTableSingleKey(key, nil) - iter := reader.NewEngineIterator(IterOptions{Prefix: true, LowerBound: lbKey}) + iter, err := reader.NewEngineIterator(IterOptions{Prefix: true, LowerBound: lbKey}) + if err != nil { + return nil, err + } defer iter.Close() valid, err := iter.SeekEngineKeyGE(EngineKey{Key: lbKey}) @@ -1467,13 +1490,15 @@ func ScanIntents( ltStart, _ := keys.LockTableSingleKey(start, nil) ltEnd, _ := keys.LockTableSingleKey(end, nil) - iter := reader.NewEngineIterator(IterOptions{LowerBound: ltStart, UpperBound: ltEnd}) + iter, err := reader.NewEngineIterator(IterOptions{LowerBound: ltStart, UpperBound: ltEnd}) + if err != nil { + return nil, err + } defer iter.Close() var meta enginepb.MVCCMetadata var intentBytes int64 var ok bool - var err error for ok, err = iter.SeekEngineKeyGE(EngineKey{Key: ltStart}); ok; ok, err = iter.NextEngineKey() { if err := ctx.Err(); err != nil { return nil, err @@ -1544,17 +1569,19 @@ func ClearRangeWithHeuristic( r Reader, w Writer, start, end roachpb.Key, pointKeyThreshold, rangeKeyThreshold int, ) error { clearPointKeys := func(r Reader, w Writer, start, end roachpb.Key, threshold int) error { - iter := r.NewEngineIterator(IterOptions{ + iter, err := r.NewEngineIterator(IterOptions{ KeyTypes: IterKeyTypePointsOnly, LowerBound: start, UpperBound: end, }) + if err != nil { + return err + } defer iter.Close() // Scan, and drop a RANGEDEL if we reach the threshold. We tighten the span // to the first encountered key, since we can cheaply do so. var ok bool - var err error var count int var firstKey roachpb.Key for ok, err = iter.SeekEngineKeyGE(EngineKey{Key: start}); ok; ok, err = iter.NextEngineKey() { @@ -1590,16 +1617,18 @@ func ClearRangeWithHeuristic( } clearRangeKeys := func(r Reader, w Writer, start, end roachpb.Key, threshold int) error { - iter := r.NewEngineIterator(IterOptions{ + iter, err := r.NewEngineIterator(IterOptions{ KeyTypes: IterKeyTypeRangesOnly, LowerBound: start, UpperBound: end, }) + if err != nil { + return err + } defer iter.Close() // Scan, and drop a RANGEKEYDEL if we reach the threshold. var ok bool - var err error var count int var firstKey roachpb.Key for ok, err = iter.SeekEngineKeyGE(EngineKey{Key: start}); ok; ok, err = iter.NextEngineKey() { @@ -1736,11 +1765,14 @@ func iterateOnReader( return nil } - it := reader.NewMVCCIterator(iterKind, IterOptions{ + it, err := reader.NewMVCCIterator(iterKind, IterOptions{ KeyTypes: keyTypes, LowerBound: start, UpperBound: end, }) + if err != nil { + return err + } defer it.Close() var rangeKeys MVCCRangeKeyStack // cached during iteration @@ -1991,7 +2023,10 @@ func ScanConflictingIntentsForDroppingLatchesEarly( ltEnd, _ := keys.LockTableSingleKey(end, nil) opts.UpperBound = ltEnd } - iter := reader.NewEngineIterator(opts) + iter, err := reader.NewEngineIterator(opts) + if err != nil { + return false, err + } defer iter.Close() var meta enginepb.MVCCMetadata diff --git a/pkg/storage/engine_test.go b/pkg/storage/engine_test.go index 3dd429e1f73b..e4e4ce7066e8 100644 --- a/pkg/storage/engine_test.go +++ b/pkg/storage/engine_test.go @@ -142,7 +142,10 @@ func TestEngineBatchStaleCachedIterator(t *testing.T) { { batch := eng.NewBatch() defer batch.Close() - iter := batch.NewMVCCIterator(MVCCKeyAndIntentsIterKind, IterOptions{UpperBound: roachpb.KeyMax}) + iter, err := batch.NewMVCCIterator(MVCCKeyAndIntentsIterKind, IterOptions{UpperBound: roachpb.KeyMax}) + if err != nil { + t.Fatal(err) + } key := MVCCKey{Key: roachpb.Key("b")} if err := batch.PutUnversioned(key.Key, []byte("foo")); err != nil { @@ -305,7 +308,10 @@ func TestEngineBatch(t *testing.T) { t.Errorf("%d: expected %s, but got %s", i, expectedValue, actualValue) } // Try using an iterator to get the value from the batch. - iter := b.NewMVCCIterator(MVCCKeyAndIntentsIterKind, IterOptions{UpperBound: roachpb.KeyMax}) + iter, err := b.NewMVCCIterator(MVCCKeyAndIntentsIterKind, IterOptions{UpperBound: roachpb.KeyMax}) + if err != nil { + t.Fatal(err) + } iter.SeekGE(key) if ok, err := iter.Valid(); !ok { if currentBatch[len(currentBatch)-1].value != nil { @@ -567,70 +573,85 @@ func TestEngineTimeBound(t *testing.T) { defer batch.Close() testCases := map[string]struct { - iter MVCCIterator + iter func() (MVCCIterator, error) keys int }{ "right not touching": { - iter: batch.NewMVCCIterator(MVCCKeyIterKind, IterOptions{ - MinTimestampHint: maxTimestamp.WallNext(), - MaxTimestampHint: maxTimestamp.WallNext().WallNext(), - UpperBound: roachpb.KeyMax, - }), + iter: func() (MVCCIterator, error) { + return batch.NewMVCCIterator(MVCCKeyIterKind, IterOptions{ + MinTimestampHint: maxTimestamp.WallNext(), + MaxTimestampHint: maxTimestamp.WallNext().WallNext(), + UpperBound: roachpb.KeyMax, + }) + }, keys: 0, }, "left not touching": { - iter: batch.NewMVCCIterator(MVCCKeyIterKind, IterOptions{ - MinTimestampHint: minTimestamp.WallPrev().WallPrev(), - MaxTimestampHint: minTimestamp.WallPrev(), - UpperBound: roachpb.KeyMax, - }), + iter: func() (MVCCIterator, error) { + return batch.NewMVCCIterator(MVCCKeyIterKind, IterOptions{ + MinTimestampHint: minTimestamp.WallPrev().WallPrev(), + MaxTimestampHint: minTimestamp.WallPrev(), + UpperBound: roachpb.KeyMax, + }) + }, keys: 0, }, "right touching": { - iter: batch.NewMVCCIterator(MVCCKeyIterKind, IterOptions{ - MinTimestampHint: maxTimestamp, - MaxTimestampHint: maxTimestamp, - UpperBound: roachpb.KeyMax, - }), + iter: func() (MVCCIterator, error) { + return batch.NewMVCCIterator(MVCCKeyIterKind, IterOptions{ + MinTimestampHint: maxTimestamp, + MaxTimestampHint: maxTimestamp, + UpperBound: roachpb.KeyMax, + }) + }, keys: len(times), }, "right touching ignores logical": { - iter: batch.NewMVCCIterator(MVCCKeyIterKind, IterOptions{ - MinTimestampHint: maxTimestamp.Next(), - MaxTimestampHint: maxTimestamp.Next().Next(), - UpperBound: roachpb.KeyMax, - }), + iter: func() (MVCCIterator, error) { + return batch.NewMVCCIterator(MVCCKeyIterKind, IterOptions{ + MinTimestampHint: maxTimestamp.Next(), + MaxTimestampHint: maxTimestamp.Next().Next(), + UpperBound: roachpb.KeyMax, + }) + }, keys: len(times), }, "left touching": { - iter: batch.NewMVCCIterator(MVCCKeyIterKind, IterOptions{ - MinTimestampHint: minTimestamp, - MaxTimestampHint: minTimestamp, - UpperBound: roachpb.KeyMax, - }), + iter: func() (MVCCIterator, error) { + return batch.NewMVCCIterator(MVCCKeyIterKind, IterOptions{ + MinTimestampHint: minTimestamp, + MaxTimestampHint: minTimestamp, + UpperBound: roachpb.KeyMax, + }) + }, keys: len(times), }, "left touching upperbound": { - iter: batch.NewMVCCIterator(MVCCKeyIterKind, IterOptions{ - MinTimestampHint: minTimestamp, - MaxTimestampHint: minTimestamp, - UpperBound: []byte("02"), - }), + iter: func() (MVCCIterator, error) { + return batch.NewMVCCIterator(MVCCKeyIterKind, IterOptions{ + MinTimestampHint: minTimestamp, + MaxTimestampHint: minTimestamp, + UpperBound: []byte("02"), + }) + }, keys: 2, }, "between": { - iter: batch.NewMVCCIterator(MVCCKeyIterKind, IterOptions{ - MinTimestampHint: minTimestamp.Next(), - MaxTimestampHint: minTimestamp.Next(), - UpperBound: roachpb.KeyMax, - }), + iter: func() (MVCCIterator, error) { + return batch.NewMVCCIterator(MVCCKeyIterKind, IterOptions{ + MinTimestampHint: minTimestamp.Next(), + MaxTimestampHint: minTimestamp.Next(), + UpperBound: roachpb.KeyMax, + }) + }, keys: len(times), }, } for name, tc := range testCases { t.Run(name, func(t *testing.T) { - tbi := tc.iter + tbi, err := tc.iter() + require.NoError(t, err) defer tbi.Close() var count int @@ -649,7 +670,10 @@ func TestEngineTimeBound(t *testing.T) { // Make a regular iterator. Before #21721, this would accidentally pick up the // time bounded iterator instead. - iter := batch.NewMVCCIterator(MVCCKeyAndIntentsIterKind, IterOptions{UpperBound: roachpb.KeyMax}) + iter, err := batch.NewMVCCIterator(MVCCKeyAndIntentsIterKind, IterOptions{UpperBound: roachpb.KeyMax}) + if err != nil { + t.Fatal(err) + } defer iter.Close() var count int @@ -763,8 +787,9 @@ func TestEngineScan1(t *testing.T) { // Test iterator stats. ro := engine.NewReadOnly(StandardDurability) - iter := ro.NewMVCCIterator(MVCCKeyIterKind, + iter, err := ro.NewMVCCIterator(MVCCKeyIterKind, IterOptions{LowerBound: roachpb.Key("cat"), UpperBound: roachpb.Key("server")}) + require.NoError(t, err) iter.SeekGE(MVCCKey{Key: roachpb.Key("cat")}) for { valid, err := iter.Valid() @@ -780,8 +805,9 @@ func TestEngineScan1(t *testing.T) { require.Equal(t, "(interface (dir, seek, step): (fwd, 1, 5), (rev, 0, 0)), "+ "(internal (dir, seek, step): (fwd, 1, 5), (rev, 0, 0))", stats.String()) iter.Close() - iter = ro.NewMVCCIterator(MVCCKeyIterKind, + iter, err = ro.NewMVCCIterator(MVCCKeyIterKind, IterOptions{LowerBound: roachpb.Key("cat"), UpperBound: roachpb.Key("server")}) + require.NoError(t, err) // pebble.Iterator is reused, but stats are reset. stats = iter.Stats().Stats // Setting non-deterministic InternalStats to empty. @@ -967,7 +993,10 @@ func TestSnapshotMethods(t *testing.T) { } // Verify NewMVCCIterator still iterates over original snapshot. - iter := snap.NewMVCCIterator(MVCCKeyAndIntentsIterKind, IterOptions{UpperBound: roachpb.KeyMax}) + iter, err := snap.NewMVCCIterator(MVCCKeyAndIntentsIterKind, IterOptions{UpperBound: roachpb.KeyMax}) + if err != nil { + t.Fatal(err) + } iter.SeekGE(newKey) if ok, err := iter.Valid(); err != nil { t.Fatal(err) @@ -1104,10 +1133,11 @@ func TestCreateCheckpoint_SpanConstrained(t *testing.T) { require.NoError(t, err) defer cDB.Close() - iter := cDB.NewMVCCIterator(MVCCKeyIterKind, IterOptions{ + iter, err := cDB.NewMVCCIterator(MVCCKeyIterKind, IterOptions{ LowerBound: key(low), UpperBound: key(high), }) + require.NoError(t, err) defer iter.Close() iter.SeekGE(MVCCKey{Key: key(low)}) count := 0 @@ -1895,7 +1925,10 @@ func TestEngineIteratorVisibility(t *testing.T) { LowerBound: keys.LocalMax, UpperBound: keys.MaxKey, } - iterOld := r.NewMVCCIterator(iterKind, opts) + iterOld, err := r.NewMVCCIterator(iterKind, opts) + if err != nil { + t.Fatal(err) + } defer iterOld.Close() // Pin the pinned reader, if it supports it. This should ensure later @@ -1928,7 +1961,10 @@ func TestEngineIteratorVisibility(t *testing.T) { // Create another iterator from the regular reader. Consistent iterators // should see the old state (because iterOld was already created for // it), others should see the new state. - iterNew := r.NewMVCCIterator(iterKind, opts) + iterNew, err := r.NewMVCCIterator(iterKind, opts) + if err != nil { + t.Fatal(err) + } defer iterNew.Close() if r.ConsistentIterators() { require.Equal(t, expectOld, scanIter(t, iterNew)) @@ -1939,7 +1975,10 @@ func TestEngineIteratorVisibility(t *testing.T) { // Create a new iterator from the pinned reader. Readers with consistent // iterators should see the old (pinned) state, others should see the // new state. - iterPinned := rPinned.NewMVCCIterator(iterKind, opts) + iterPinned, err := rPinned.NewMVCCIterator(iterKind, opts) + if err != nil { + t.Fatal(err) + } defer iterPinned.Close() if rPinned.ConsistentIterators() { require.Equal(t, expectOld, scanIter(t, iterPinned)) @@ -1988,7 +2027,10 @@ func TestEngineIteratorVisibility(t *testing.T) { // A new iterator should read our own writes if the reader supports it, // but consistent iterators should not see the changes to the underlying // engine either way. - iterOwn := r.NewMVCCIterator(iterKind, opts) + iterOwn, err := r.NewMVCCIterator(iterKind, opts) + if err != nil { + t.Fatal(err) + } defer iterOwn.Close() if tc.readOwnWrites { if r.ConsistentIterators() { @@ -2514,11 +2556,12 @@ func TestEngineRangeKeyMutations(t *testing.T) { func scanRangeKeys(t *testing.T, r Reader) []MVCCRangeKeyValue { t.Helper() - iter := r.NewMVCCIterator(MVCCKeyIterKind, IterOptions{ + iter, err := r.NewMVCCIterator(MVCCKeyIterKind, IterOptions{ KeyTypes: IterKeyTypeRangesOnly, LowerBound: keys.LocalMax, UpperBound: keys.MaxKey, }) + require.NoError(t, err) defer iter.Close() var rangeKeys []MVCCRangeKeyValue @@ -2539,10 +2582,11 @@ func scanRangeKeys(t *testing.T, r Reader) []MVCCRangeKeyValue { func scanPointKeys(t *testing.T, r Reader) []MVCCKey { t.Helper() - iter := r.NewMVCCIterator(MVCCKeyIterKind, IterOptions{ + iter, err := r.NewMVCCIterator(MVCCKeyIterKind, IterOptions{ LowerBound: keys.LocalMax, UpperBound: keys.MaxKey, }) + require.NoError(t, err) defer iter.Close() var pointKeys []MVCCKey diff --git a/pkg/storage/intent_interleaving_iter.go b/pkg/storage/intent_interleaving_iter.go index fe0d45cb51a7..7139fc35ab7e 100644 --- a/pkg/storage/intent_interleaving_iter.go +++ b/pkg/storage/intent_interleaving_iter.go @@ -168,7 +168,7 @@ func isLocal(k roachpb.Key) bool { return k.Compare(keys.LocalMax) < 0 } -func newIntentInterleavingIterator(reader Reader, opts IterOptions) MVCCIterator { +func newIntentInterleavingIterator(reader Reader, opts IterOptions) (MVCCIterator, error) { if !opts.MinTimestampHint.IsEmpty() || !opts.MaxTimestampHint.IsEmpty() { panic("intentInterleavingIter must not be used with timestamp hints") } @@ -257,7 +257,11 @@ func newIntentInterleavingIterator(reader Reader, opts IterOptions) MVCCIterator // // Note that we can reuse intentKeyBuf, intentLimitKeyBuf after // NewEngineIterator returns. - intentIter := reader.NewEngineIterator(intentOpts).(*pebbleIterator) + intentEngineIter, err := reader.NewEngineIterator(intentOpts) + if err != nil { + return nil, err + } + intentIter := intentEngineIter.(*pebbleIterator) // The creation of these iterators can race with concurrent mutations, which // may make them inconsistent with each other. So we clone here, to ensure @@ -265,7 +269,11 @@ func newIntentInterleavingIterator(reader Reader, opts IterOptions) MVCCIterator // and we use that when possible to save allocations). var iter *pebbleIterator if reader.ConsistentIterators() { - iter = maybeUnwrapUnsafeIter(reader.NewMVCCIterator(MVCCKeyIterKind, opts)).(*pebbleIterator) + mvccIter, err := reader.NewMVCCIterator(MVCCKeyIterKind, opts) + if err != nil { + return nil, err + } + iter = maybeUnwrapUnsafeIter(mvccIter).(*pebbleIterator) } else { iter = newPebbleIteratorByCloning(intentIter.CloneContext(), opts, StandardDurability) } @@ -279,7 +287,7 @@ func newIntentInterleavingIterator(reader Reader, opts IterOptions) MVCCIterator intentKeyBuf: intentKeyBuf, intentLimitKeyBuf: intentLimitKeyBuf, } - return iiIter + return iiIter, nil } // TODO(sumeer): the limits generated below are tight for the current value of diff --git a/pkg/storage/intent_interleaving_iter_test.go b/pkg/storage/intent_interleaving_iter_test.go index 6bbcbde24be2..5feea36beb47 100644 --- a/pkg/storage/intent_interleaving_iter_test.go +++ b/pkg/storage/intent_interleaving_iter_test.go @@ -337,7 +337,11 @@ func TestIntentInterleavingIter(t *testing.T) { if d.HasArg("prefix") { d.ScanArgs(t, "prefix", &opts.Prefix) } - iter := maybeWrapInUnsafeIter(newIntentInterleavingIterator(eng, opts)) + iiter, err := newIntentInterleavingIterator(eng, opts) + if err != nil { + t.Fatal(err) + } + iter := maybeWrapInUnsafeIter(iiter) var b strings.Builder defer iter.Close() // pos is the original : prefix computed by @@ -404,33 +408,53 @@ func TestIntentInterleavingIterBoundaries(t *testing.T) { // Boundary cases for constrainedToLocal. func() { opts := IterOptions{LowerBound: keys.MinKey} - iter := newIntentInterleavingIterator(eng, opts).(*intentInterleavingIter) + iiiter, err := newIntentInterleavingIterator(eng, opts) + if err != nil { + t.Fatal(err) + } + iter := iiiter.(*intentInterleavingIter) defer iter.Close() require.Equal(t, constrainedToLocal, iter.constraint) iter.SeekLT(MVCCKey{Key: keys.LocalMax}) }() func() { opts := IterOptions{UpperBound: keys.LocalMax} - iter := newIntentInterleavingIterator(eng, opts).(*intentInterleavingIter) + iiiter, err := newIntentInterleavingIterator(eng, opts) + if err != nil { + t.Fatal(err) + } + iter := iiiter.(*intentInterleavingIter) defer iter.Close() require.Equal(t, constrainedToLocal, iter.constraint) }() require.Panics(t, func() { opts := IterOptions{UpperBound: keys.LocalMax} - iter := newIntentInterleavingIterator(eng, opts).(*intentInterleavingIter) + iiiter, err := newIntentInterleavingIterator(eng, opts) + if err != nil { + t.Fatal(err) + } + iter := iiiter.(*intentInterleavingIter) defer iter.Close() iter.SeekLT(MVCCKey{Key: keys.MaxKey}) }) // Boundary cases for constrainedToGlobal. func() { opts := IterOptions{LowerBound: keys.LocalMax} - iter := newIntentInterleavingIterator(eng, opts).(*intentInterleavingIter) + iiiter, err := newIntentInterleavingIterator(eng, opts) + if err != nil { + t.Fatal(err) + } + iter := iiiter.(*intentInterleavingIter) defer iter.Close() require.Equal(t, constrainedToGlobal, iter.constraint) }() func() { opts := IterOptions{LowerBound: keys.LocalMax} - iter := newIntentInterleavingIterator(eng, opts).(*intentInterleavingIter) + iiiter, err := newIntentInterleavingIterator(eng, opts) + if err != nil { + t.Fatal(err) + } + iter := iiiter.(*intentInterleavingIter) defer iter.Close() require.Equal(t, constrainedToGlobal, iter.constraint) iter.SeekLT(MVCCKey{Key: keys.LocalMax}) @@ -438,14 +462,22 @@ func TestIntentInterleavingIterBoundaries(t *testing.T) { // Panics for using a local key that is above the lock table. require.Panics(t, func() { opts := IterOptions{UpperBound: keys.LocalMax} - iter := newIntentInterleavingIterator(eng, opts).(*intentInterleavingIter) + iiiter, err := newIntentInterleavingIterator(eng, opts) + if err != nil { + t.Fatal(err) + } + iter := iiiter.(*intentInterleavingIter) defer iter.Close() require.Equal(t, constrainedToLocal, iter.constraint) iter.SeekLT(MVCCKey{Key: keys.LocalRangeLockTablePrefix.PrefixEnd()}) }) require.Panics(t, func() { opts := IterOptions{UpperBound: keys.LocalMax} - iter := newIntentInterleavingIterator(eng, opts).(*intentInterleavingIter) + iiiter, err := newIntentInterleavingIterator(eng, opts) + if err != nil { + t.Fatal(err) + } + iter := iiiter.(*intentInterleavingIter) defer iter.Close() require.Equal(t, constrainedToLocal, iter.constraint) iter.SeekGE(MVCCKey{Key: keys.LocalRangeLockTablePrefix.PrefixEnd()}) @@ -454,13 +486,21 @@ func TestIntentInterleavingIterBoundaries(t *testing.T) { // specified. func() { opts := IterOptions{Prefix: true, LowerBound: keys.LocalMax} - iter := newIntentInterleavingIterator(eng, opts).(*intentInterleavingIter) + iiiter, err := newIntentInterleavingIterator(eng, opts) + if err != nil { + t.Fatal(err) + } + iter := iiiter.(*intentInterleavingIter) defer iter.Close() require.Equal(t, constrainedToGlobal, iter.constraint) }() // Prefix iteration with no bounds. func() { - iter := newIntentInterleavingIterator(eng, IterOptions{Prefix: true}).(*intentInterleavingIter) + iiiter, err := newIntentInterleavingIterator(eng, IterOptions{Prefix: true}) + if err != nil { + t.Fatal(err) + } + iter := iiiter.(*intentInterleavingIter) defer iter.Close() require.Equal(t, notConstrained, iter.constraint) }() @@ -668,11 +708,13 @@ func doOps(t *testing.T, ops []string, eng Engine, interleave bool, out *strings if d.HasArg("upper") { opts.UpperBound = scanRoachKey(t, &d, "upper") } + var err error if interleave { - iter = newIntentInterleavingIterator(eng, opts) + iter, err = newIntentInterleavingIterator(eng, opts) } else { - iter = eng.NewMVCCIterator(MVCCKeyIterKind, opts) + iter, err = eng.NewMVCCIterator(MVCCKeyIterKind, opts) } + require.NoError(t, err) lowerStr := "nil" if opts.LowerBound != nil { lowerStr = string(makePrintableRoachpbKey(opts.LowerBound)) @@ -843,10 +885,14 @@ func BenchmarkIntentInterleavingIterNext(b *testing.B) { func(b *testing.B) { var iter MVCCIterator opts := IterOptions{LowerBound: state.keyPrefix, UpperBound: state.keyPrefix.PrefixEnd()} + var err error if state.separated { - iter = newIntentInterleavingIterator(state.eng, opts) + iter, err = newIntentInterleavingIterator(state.eng, opts) } else { - iter = state.eng.NewMVCCIterator(MVCCKeyIterKind, opts) + iter, err = state.eng.NewMVCCIterator(MVCCKeyIterKind, opts) + } + if err != nil { + b.Fatal(err) } defer iter.Close() startKey := MVCCKey{Key: state.keyPrefix} @@ -882,10 +928,14 @@ func BenchmarkIntentInterleavingIterPrev(b *testing.B) { var iter MVCCIterator endKey := MVCCKey{Key: state.keyPrefix.PrefixEnd()} opts := IterOptions{LowerBound: state.keyPrefix, UpperBound: endKey.Key} + var err error if state.separated { - iter = newIntentInterleavingIterator(state.eng, opts) + iter, err = newIntentInterleavingIterator(state.eng, opts) } else { - iter = state.eng.NewMVCCIterator(MVCCKeyIterKind, opts) + iter, err = state.eng.NewMVCCIterator(MVCCKeyIterKind, opts) + } + if err != nil { + b.Fatal(err) } defer iter.Close() iter.SeekLT(endKey) @@ -925,10 +975,14 @@ func BenchmarkIntentInterleavingSeekGEAndIter(b *testing.B) { var iter MVCCIterator endKey := state.keyPrefix.PrefixEnd() opts := IterOptions{LowerBound: state.keyPrefix, UpperBound: endKey} + var err error if state.separated { - iter = newIntentInterleavingIterator(state.eng, opts) + iter, err = newIntentInterleavingIterator(state.eng, opts) } else { - iter = state.eng.NewMVCCIterator(MVCCKeyIterKind, opts) + iter, err = state.eng.NewMVCCIterator(MVCCKeyIterKind, opts) + } + if err != nil { + b.Fatal(err) } defer iter.Close() b.ResetTimer() diff --git a/pkg/storage/intent_reader_writer.go b/pkg/storage/intent_reader_writer.go index 8d3b74c6e555..92c8d0670920 100644 --- a/pkg/storage/intent_reader_writer.go +++ b/pkg/storage/intent_reader_writer.go @@ -128,7 +128,7 @@ var intentInterleavingReaderPool = sync.Pool{ // intentInterleavingReader can be freed once this method returns. func (imr *intentInterleavingReader) NewMVCCIterator( iterKind MVCCIterKind, opts IterOptions, -) MVCCIterator { +) (MVCCIterator, error) { if (!opts.MinTimestampHint.IsEmpty() || !opts.MaxTimestampHint.IsEmpty()) && iterKind == MVCCKeyAndIntentsIterKind { panic("cannot ask for interleaved intents when specifying timestamp hints") diff --git a/pkg/storage/intent_reader_writer_test.go b/pkg/storage/intent_reader_writer_test.go index 4c0e74ee10b9..616ac3bdc690 100644 --- a/pkg/storage/intent_reader_writer_test.go +++ b/pkg/storage/intent_reader_writer_test.go @@ -57,7 +57,11 @@ func printLTKey(k LockTableKey) string { } func printEngContents(b *strings.Builder, eng Engine) { - iter := eng.NewEngineIterator(IterOptions{UpperBound: roachpb.KeyMax}) + iter, err := eng.NewEngineIterator(IterOptions{UpperBound: roachpb.KeyMax}) + if err != nil { + fmt.Fprintf(b, "error: %s\n", err.Error()) + return + } defer iter.Close() fmt.Fprintf(b, "=== Storage contents ===\n") valid, err := iter.SeekEngineKeyGE(EngineKey{Key: []byte("")}) diff --git a/pkg/storage/metamorphic/operations.go b/pkg/storage/metamorphic/operations.go index 9e10b24d514a..23775952d99c 100644 --- a/pkg/storage/metamorphic/operations.go +++ b/pkg/storage/metamorphic/operations.go @@ -608,11 +608,14 @@ type iterOpenOp struct { func (i iterOpenOp) run(ctx context.Context) string { rw := i.m.getReadWriter(i.rw) - iter := rw.NewMVCCIterator(storage.MVCCKeyIterKind, storage.IterOptions{ + iter, err := rw.NewMVCCIterator(storage.MVCCKeyIterKind, storage.IterOptions{ Prefix: false, LowerBound: i.key, UpperBound: i.endKey.Next(), }) + if err != nil { + return err.Error() + } i.m.setIterInfo(i.id, iteratorInfo{ id: i.id, diff --git a/pkg/storage/mvcc.go b/pkg/storage/mvcc.go index bb9da19d7370..24269e4407e9 100644 --- a/pkg/storage/mvcc.go +++ b/pkg/storage/mvcc.go @@ -887,13 +887,16 @@ func MVCCBlindPutInlineWithPrev( // rejected below Raft in that case, but it would trip this assertion. We have // plenty of other tests and assertions for this. if false && ms != nil { - iter := newMVCCIterator( + iter, err := newMVCCIterator( rw, hlc.Timestamp{}, false /* rangeKeyMasking */, false, /* noInterleavedIntents */ IterOptions{ KeyTypes: IterKeyTypePointsAndRanges, Prefix: true, }, ) + if err != nil { + return err + } defer iter.Close() var meta enginepb.MVCCMetadata ok, metaKeySize, metaValSize, _, err := mvccGetMetadata(iter, MVCCKey{Key: key}, &meta) @@ -1042,7 +1045,7 @@ func newMVCCIterator( rangeKeyMasking bool, noInterleavedIntents bool, opts IterOptions, -) MVCCIterator { +) (MVCCIterator, error) { // If reading inline then just return a plain MVCCIterator without intents. // However, we allow the caller to enable range keys, since they may be needed // for conflict checks when writing inline values. @@ -1126,12 +1129,15 @@ func MVCCGetWithValueHeader( } return result, enginepb.MVCCValueHeader{}, nil } - iter := newMVCCIterator( + iter, err := newMVCCIterator( reader, timestamp, false /* rangeKeyMasking */, opts.DontInterleaveIntents, IterOptions{ KeyTypes: IterKeyTypePointsAndRanges, Prefix: true, }, ) + if err != nil { + return result, enginepb.MVCCValueHeader{}, err + } defer iter.Close() value, intent, vh, err := mvccGetWithValueHeader(ctx, iter, key, timestamp, opts) val := value.ToPointer() @@ -1501,12 +1507,16 @@ func MVCCPut( var iter MVCCIterator blind := opts.Stats == nil && timestamp.IsEmpty() if !blind { - iter = newMVCCIterator( + var err error + iter, err = newMVCCIterator( rw, timestamp, false /* rangeKeyMasking */, false /* noInterleavedIntents */, IterOptions{ KeyTypes: IterKeyTypePointsAndRanges, Prefix: true, }, ) + if err != nil { + return err + } defer iter.Close() } return mvccPutUsingIter(ctx, rw, iter, key, timestamp, value, nil, opts) @@ -1549,12 +1559,15 @@ func MVCCDelete( timestamp hlc.Timestamp, opts MVCCWriteOptions, ) (foundKey bool, err error) { - iter := newMVCCIterator( + iter, err := newMVCCIterator( rw, timestamp, false /* rangeKeyMasking */, false /* noInterleavedIntents */, IterOptions{ KeyTypes: IterKeyTypePointsAndRanges, Prefix: true, }, ) + if err != nil { + return false, err + } defer iter.Close() buf := newPutBuffer() @@ -2269,12 +2282,15 @@ func MVCCIncrement( opts MVCCWriteOptions, inc int64, ) (int64, error) { - iter := newMVCCIterator( + iter, err := newMVCCIterator( rw, timestamp, false /* rangeKeyMasking */, false /* noInterleavedIntents */, IterOptions{ KeyTypes: IterKeyTypePointsAndRanges, Prefix: true, }, ) + if err != nil { + return 0, err + } defer iter.Close() var int64Val int64 @@ -2305,7 +2321,7 @@ func MVCCIncrement( return newValue, nil } - err := mvccPutUsingIter(ctx, rw, iter, key, timestamp, noValue, valueFn, opts) + err = mvccPutUsingIter(ctx, rw, iter, key, timestamp, noValue, valueFn, opts) return newInt64Val, err } @@ -2347,12 +2363,15 @@ func MVCCConditionalPut( allowIfDoesNotExist CPutMissingBehavior, opts MVCCWriteOptions, ) error { - iter := newMVCCIterator( + iter, err := newMVCCIterator( rw, timestamp, false /* rangeKeyMasking */, false /* noInterleavedIntents */, IterOptions{ KeyTypes: IterKeyTypePointsAndRanges, Prefix: true, }, ) + if err != nil { + return err + } defer iter.Close() return mvccConditionalPutUsingIter( @@ -2428,12 +2447,15 @@ func MVCCInitPut( failOnTombstones bool, opts MVCCWriteOptions, ) error { - iter := newMVCCIterator( + iter, err := newMVCCIterator( rw, timestamp, false /* rangeKeyMasking */, false /* noInterleavedIntents */, IterOptions{ KeyTypes: IterKeyTypePointsAndRanges, Prefix: true, }, ) + if err != nil { + return err + } defer iter.Close() return mvccInitPutUsingIter(ctx, rw, iter, key, timestamp, value, failOnTombstones, opts) } @@ -2733,11 +2755,14 @@ func MVCCClearTimeRange( // Fetch the existing range keys (if any), to adjust MVCC stats. We set up // a new iterator for every batch, which both sees our own writes as well as // any range keys outside of the time bounds. - rkIter := rw.NewMVCCIterator(MVCCKeyIterKind, IterOptions{ + rkIter, err := rw.NewMVCCIterator(MVCCKeyIterKind, IterOptions{ KeyTypes: IterKeyTypeRangesOnly, LowerBound: leftPeekBound, UpperBound: rightPeekBound, }) + if err != nil { + return err + } defer rkIter.Close() cmp, remaining, err := PeekRangeKeysRight(rkIter, clearRangeKeys.Bounds.Key) @@ -2810,7 +2835,7 @@ func MVCCClearTimeRange( // time-range, as we do not want to clear any running transactions. We don't // _expect_ to hit this since the RevertRange is only intended for non-live // key spans, but there could be an intent leftover. - iter := NewMVCCIncrementalIterator(rw, MVCCIncrementalIterOptions{ + iter := NewMVCCIncrementalIterator(rw.(ReaderWithMustIterators), MVCCIncrementalIterOptions{ KeyTypes: IterKeyTypePointsAndRanges, StartKey: key, EndKey: endKey, @@ -3059,12 +3084,15 @@ func MVCCDeleteRange( buf := newPutBuffer() defer buf.release() - iter := newMVCCIterator( + iter, err := newMVCCIterator( rw, timestamp, false /* rangeKeyMasking */, false /* noInterleavedIntents */, IterOptions{ KeyTypes: IterKeyTypePointsAndRanges, Prefix: true, }, ) + if err != nil { + return nil, nil, 0, err + } defer iter.Close() var keys []roachpb.Key @@ -3222,12 +3250,15 @@ func MVCCPredicateDeleteRange( // Create some reusable machinery for flushing a run with point tombstones // that is typically used in a single MVCCPut call. - pointTombstoneIter := newMVCCIterator( + pointTombstoneIter, err := newMVCCIterator( rw, endTime, false /* rangeKeyMasking */, false /* noInterleavedIntents */, IterOptions{ KeyTypes: IterKeyTypePointsAndRanges, Prefix: true, }, ) + if err != nil { + return nil, err + } defer pointTombstoneIter.Close() pointTombstoneBuf := newPutBuffer() defer pointTombstoneBuf.release() @@ -3280,7 +3311,7 @@ func MVCCPredicateDeleteRange( // endTime. We don't _expect_ to hit intents or newer keys in the client // provided span since the MVCCPredicateDeleteRange is only intended for // non-live key spans, but there could be an intent leftover. - iter := NewMVCCIncrementalIterator(rw, MVCCIncrementalIterOptions{ + iter := NewMVCCIncrementalIterator(rw.(ReaderWithMustIterators), MVCCIncrementalIterOptions{ EndKey: endKey, StartTime: predicates.StartTime, EndTime: hlc.MaxTimestamp, @@ -3458,12 +3489,15 @@ func MVCCDeleteRangeUsingTombstone( // with newer MVCC range tombstones. if idempotent { if noPointKeys, err := func() (bool, error) { - iter := rw.NewMVCCIterator(MVCCKeyIterKind, IterOptions{ + iter, err := rw.NewMVCCIterator(MVCCKeyIterKind, IterOptions{ KeyTypes: IterKeyTypePointsAndRanges, LowerBound: startKey, UpperBound: endKey, RangeKeyMaskingBelow: timestamp, }) + if err != nil { + return false, err + } defer iter.Close() for iter.SeekGE(MVCCKey{Key: startKey}); ; iter.Next() { if ok, err := iter.Valid(); err != nil { @@ -3487,7 +3521,7 @@ func MVCCDeleteRangeUsingTombstone( // do a separate time-bound scan for point key conflicts. if msCovered != nil { if err := func() error { - iter := NewMVCCIncrementalIterator(rw, MVCCIncrementalIterOptions{ + iter := NewMVCCIncrementalIterator(rw.(ReaderWithMustIterators), MVCCIncrementalIterOptions{ KeyTypes: IterKeyTypePointsOnly, StartKey: startKey, EndKey: endKey, @@ -3519,7 +3553,10 @@ func MVCCDeleteRangeUsingTombstone( iterOpts.KeyTypes = IterKeyTypeRangesOnly iterOpts.RangeKeyMaskingBelow = hlc.Timestamp{} } - iter := rw.NewMVCCIterator(MVCCKeyIterKind, iterOpts) + iter, err := rw.NewMVCCIterator(MVCCKeyIterKind, iterOpts) + if err != nil { + return err + } defer iter.Close() iter.SeekGE(MVCCKey{Key: startKey}) @@ -3600,11 +3637,14 @@ func MVCCDeleteRangeUsingTombstone( if rightPeekBound == nil { rightPeekBound = keys.MaxKey } - rkIter := rw.NewMVCCIterator(MVCCKeyIterKind, IterOptions{ + rkIter, err := rw.NewMVCCIterator(MVCCKeyIterKind, IterOptions{ KeyTypes: IterKeyTypeRangesOnly, LowerBound: leftPeekBound, UpperBound: rightPeekBound, }) + if err != nil { + return err + } defer rkIter.Close() // Peek to the left. @@ -4051,13 +4091,16 @@ func MVCCScan( timestamp hlc.Timestamp, opts MVCCScanOptions, ) (MVCCScanResult, error) { - iter := newMVCCIterator( + iter, err := newMVCCIterator( reader, timestamp, !opts.Tombstones, opts.DontInterleaveIntents, IterOptions{ KeyTypes: IterKeyTypePointsAndRanges, LowerBound: key, UpperBound: endKey, }, ) + if err != nil { + return MVCCScanResult{}, err + } defer iter.Close() return mvccScanToKvs(ctx, iter, key, endKey, timestamp, opts) } @@ -4070,13 +4113,16 @@ func MVCCScanToBytes( timestamp hlc.Timestamp, opts MVCCScanOptions, ) (MVCCScanResult, error) { - iter := newMVCCIterator( + iter, err := newMVCCIterator( reader, timestamp, !opts.Tombstones, opts.DontInterleaveIntents, IterOptions{ KeyTypes: IterKeyTypePointsAndRanges, LowerBound: key, UpperBound: endKey, }, ) + if err != nil { + return MVCCScanResult{}, err + } defer iter.Close() return mvccScanToBytes(ctx, iter, key, endKey, timestamp, opts) } @@ -4120,13 +4166,16 @@ func MVCCIterate( opts MVCCScanOptions, f func(roachpb.KeyValue) error, ) ([]roachpb.Intent, error) { - iter := newMVCCIterator( + iter, err := newMVCCIterator( reader, timestamp, !opts.Tombstones, opts.DontInterleaveIntents, IterOptions{ KeyTypes: IterKeyTypePointsAndRanges, LowerBound: key, UpperBound: endKey, }, ) + if err != nil { + return nil, err + } defer iter.Close() var intents []roachpb.Intent @@ -4285,10 +4334,14 @@ func MVCCResolveWriteIntent( return false, 0, &roachpb.Span{Key: intent.Key}, nil } - iterAndBuf := GetBufUsingIter(rw.NewMVCCIterator(MVCCKeyAndIntentsIterKind, IterOptions{ + iter, err := rw.NewMVCCIterator(MVCCKeyAndIntentsIterKind, IterOptions{ KeyTypes: IterKeyTypePointsAndRanges, Prefix: true, - })) + }) + if err != nil { + return false, 0, nil, err + } + iterAndBuf := GetBufUsingIter(iter) iterAndBuf.iter.SeekIntentGE(intent.Key, intent.Txn.ID) // Production code will use a buffered writer, which makes the numBytes // calculation accurate. Note that an inaccurate numBytes (e.g. 0 in the @@ -5107,7 +5160,10 @@ func MVCCResolveWriteIntentRange( } ltStart, _ := keys.LockTableSingleKey(intent.Key, nil) ltEnd, _ := keys.LockTableSingleKey(intent.EndKey, nil) - engineIter := rw.NewEngineIterator(IterOptions{LowerBound: ltStart, UpperBound: ltEnd}) + engineIter, err := rw.NewEngineIterator(IterOptions{LowerBound: ltStart, UpperBound: ltEnd}) + if err != nil { + return 0, 0, nil, 0, err + } var mvccIter MVCCIterator iterOpts := IterOptions{ KeyTypes: IterKeyTypePointsAndRanges, @@ -5116,7 +5172,10 @@ func MVCCResolveWriteIntentRange( } if rw.ConsistentIterators() { // Production code should always have consistent iterators. - mvccIter = rw.NewMVCCIterator(MVCCKeyIterKind, iterOpts) + mvccIter, err = rw.NewMVCCIterator(MVCCKeyIterKind, iterOpts) + if err != nil { + return 0, 0, nil, 0, err + } } else { // For correctness, we need mvccIter to be consistent with engineIter. mvccIter = newPebbleIteratorByCloning(engineIter.CloneContext(), iterOpts, StandardDurability) @@ -5233,11 +5292,14 @@ func MVCCGarbageCollect( // Bound the iterator appropriately for the set of keys we'll be garbage // collecting. - iter := rw.NewMVCCIterator(MVCCKeyAndIntentsIterKind, IterOptions{ + iter, err := rw.NewMVCCIterator(MVCCKeyAndIntentsIterKind, IterOptions{ LowerBound: keys[0].Key, UpperBound: keys[len(keys)-1].Key.Next(), KeyTypes: IterKeyTypePointsAndRanges, }) + if err != nil { + return err + } defer iter.Close() // Cached stack of range tombstones covering current point. Used to determine @@ -5558,11 +5620,14 @@ func MVCCGarbageCollectRangeKeys( // Bound the iterator appropriately for the set of keys we'll be garbage // collecting. We are using latch bounds to collect info about adjacent // range fragments for correct MVCCStats updates. - iter := rw.NewMVCCIterator(MVCCKeyIterKind, IterOptions{ + iter, err := rw.NewMVCCIterator(MVCCKeyIterKind, IterOptions{ LowerBound: gcKey.LatchSpan.Key, UpperBound: gcKey.LatchSpan.EndKey, KeyTypes: IterKeyTypeRangesOnly, }) + if err != nil { + return err + } defer iter.Close() for iter.SeekGE(MVCCKey{Key: gcKey.LatchSpan.Key}); ; iter.Next() { @@ -5644,7 +5709,7 @@ func MVCCGarbageCollectRangeKeys( // Verify that there are no remaining data under the deleted range using // time bound iterator. - ptIter := NewMVCCIncrementalIterator(rw, MVCCIncrementalIterOptions{ + ptIter := NewMVCCIncrementalIterator(rw.(ReaderWithMustIterators), MVCCIncrementalIterOptions{ KeyTypes: IterKeyTypePointsOnly, StartKey: rangeKeys.Bounds.Key, EndKey: rangeKeys.Bounds.EndKey, @@ -5735,12 +5800,15 @@ func CanGCEntireRange( if isLocal(start) || isLocal(end) { return coveredByRangeTombstones, errors.Errorf("range emptiness check can only be done on global ranges") } - iter := rw.NewMVCCIterator(MVCCKeyAndIntentsIterKind, IterOptions{ + iter, err := rw.NewMVCCIterator(MVCCKeyAndIntentsIterKind, IterOptions{ KeyTypes: IterKeyTypePointsAndRanges, LowerBound: start, UpperBound: end, RangeKeyMaskingBelow: gcThreshold, }) + if err != nil { + return coveredByRangeTombstones, err + } defer iter.Close() iter.SeekGE(MVCCKey{Key: start}) for ; ; iter.Next() { @@ -5790,11 +5858,14 @@ func MVCCGarbageCollectPointsWithClearRange( countKeys, float64(countKeys)*1e9/float64(timeutil.Since(begin)), removedEntries) }(timeutil.Now()) - iter := rw.NewMVCCIterator(MVCCKeyAndIntentsIterKind, IterOptions{ + iter, err := rw.NewMVCCIterator(MVCCKeyAndIntentsIterKind, IterOptions{ LowerBound: start, UpperBound: end, KeyTypes: IterKeyTypePointsAndRanges, }) + if err != nil { + return err + } defer iter.Close() iter.SeekGE(MVCCKey{Key: start}) @@ -5914,7 +5985,10 @@ func MVCCFindSplitKey( key = roachpb.RKey(keys.LocalMax) } - it := reader.NewMVCCIterator(MVCCKeyAndIntentsIterKind, IterOptions{UpperBound: endKey.AsRawKey()}) + it, err := reader.NewMVCCIterator(MVCCKeyAndIntentsIterKind, IterOptions{UpperBound: endKey.AsRawKey()}) + if err != nil { + return nil, err + } defer it.Close() // We want to avoid splitting at the first key in the range because that @@ -6030,7 +6104,10 @@ func MVCCFirstSplitKey( return nil, nil } - it := reader.NewMVCCIterator(MVCCKeyAndIntentsIterKind, IterOptions{UpperBound: endKey.AsRawKey()}) + it, err := reader.NewMVCCIterator(MVCCKeyAndIntentsIterKind, IterOptions{UpperBound: endKey.AsRawKey()}) + if err != nil { + return nil, err + } defer it.Close() // If the caller has provided a desiredSplitKey less than the minimum split @@ -6092,11 +6169,14 @@ func ComputeStatsWithVisitors( pointKeyVisitor func(MVCCKey, []byte) error, rangeKeyVisitor func(MVCCRangeKeyValue) error, ) (enginepb.MVCCStats, error) { - iter := r.NewMVCCIterator(MVCCKeyAndIntentsIterKind, IterOptions{ + iter, err := r.NewMVCCIterator(MVCCKeyAndIntentsIterKind, IterOptions{ KeyTypes: IterKeyTypePointsAndRanges, LowerBound: start, UpperBound: end, }) + if err != nil { + return enginepb.MVCCStats{}, err + } defer iter.Close() iter.SeekGE(MVCCKey{Key: start}) return computeStatsForIterWithVisitors(iter, nowNanos, pointKeyVisitor, rangeKeyVisitor) @@ -6388,13 +6468,17 @@ func MVCCIsSpanEmpty( // error on any inline values, and the caller may want to respect them instead. var iter SimpleMVCCIterator if opts.StartTS.IsEmpty() && opts.EndTS.IsEmpty() { - iter = reader.NewMVCCIterator(MVCCKeyAndIntentsIterKind, IterOptions{ + var err error + iter, err = reader.NewMVCCIterator(MVCCKeyAndIntentsIterKind, IterOptions{ KeyTypes: IterKeyTypePointsAndRanges, LowerBound: opts.StartKey, UpperBound: opts.EndKey, }) + if err != nil { + return false, err + } } else { - iter = NewMVCCIncrementalIterator(reader, MVCCIncrementalIterOptions{ + iter = NewMVCCIncrementalIterator(reader.(ReaderWithMustIterators), MVCCIncrementalIterOptions{ KeyTypes: IterKeyTypePointsAndRanges, StartKey: opts.StartKey, EndKey: opts.EndKey, @@ -6566,7 +6650,7 @@ func mvccExportToWriter( // actually doing the backup work. elasticCPUHandle.StartTimer() - iter := NewMVCCIncrementalIterator(reader, MVCCIncrementalIterOptions{ + iter := NewMVCCIncrementalIterator(reader.(ReaderWithMustIterators), MVCCIncrementalIterOptions{ KeyTypes: IterKeyTypePointsAndRanges, StartKey: opts.StartKey.Key, EndKey: opts.EndKey, @@ -7030,12 +7114,15 @@ func ReplacePointTombstonesWithRangeTombstones( end = start.Next() } - iter := rw.NewMVCCIterator(MVCCKeyAndIntentsIterKind, IterOptions{ + iter, err := rw.NewMVCCIterator(MVCCKeyAndIntentsIterKind, IterOptions{ KeyTypes: IterKeyTypePointsAndRanges, Prefix: end.Equal(start.Next()), LowerBound: start, UpperBound: end, }) + if err != nil { + return err + } defer iter.Close() var clearedKey MVCCKey @@ -7193,11 +7280,14 @@ func isWatchedSystemTable(key roachpb.Key) bool { func MVCCLookupRangeKeyValue( reader Reader, key, endKey roachpb.Key, ts hlc.Timestamp, ) ([]byte, error) { - it := reader.NewMVCCIterator(MVCCKeyIterKind, IterOptions{ + it, err := reader.NewMVCCIterator(MVCCKeyIterKind, IterOptions{ LowerBound: key, UpperBound: endKey, KeyTypes: IterKeyTypeRangesOnly, }) + if err != nil { + return nil, err + } defer it.Close() it.SeekGE(MVCCKey{Key: key}) diff --git a/pkg/storage/mvcc_history_test.go b/pkg/storage/mvcc_history_test.go index b0fa2906593b..301ff75db9bc 100644 --- a/pkg/storage/mvcc_history_test.go +++ b/pkg/storage/mvcc_history_test.go @@ -967,7 +967,10 @@ func cmdCheckIntent(e *evalCtx) error { return e.withReader(func(r storage.Reader) error { var meta enginepb.MVCCMetadata - iter := r.NewMVCCIterator(storage.MVCCKeyAndIntentsIterKind, storage.IterOptions{Prefix: true}) + iter, err := r.NewMVCCIterator(storage.MVCCKeyAndIntentsIterKind, storage.IterOptions{Prefix: true}) + if err != nil { + return err + } defer iter.Close() iter.SeekGE(storage.MVCCKey{Key: key}) ok, err := iter.Valid() @@ -1713,7 +1716,10 @@ func cmdIterNew(e *evalCtx) error { } r := e.newReader() - iter := r.NewMVCCIterator(kind, opts) + iter, err := r.NewMVCCIterator(kind, opts) + if err != nil { + return err + } iter = newMetamorphicIterator(e.t, e.metamorphicIterSeed(), iter).(storage.MVCCIterator) if opts.Prefix != iter.IsPrefix() { return errors.Errorf("prefix iterator returned IsPrefix=false") @@ -1777,7 +1783,7 @@ func cmdIterNewIncremental(e *evalCtx) error { } r := e.newReader() - it := storage.SimpleMVCCIterator(storage.NewMVCCIncrementalIterator(r, opts)) + it := storage.SimpleMVCCIterator(storage.NewMVCCIncrementalIterator(r.(storage.ReaderWithMustIterators), opts)) // Can't metamorphically move the iterator around since when intents get aggregated // or emitted we can't undo that later at the level of the metamorphic iterator. if opts.IntentPolicy == storage.MVCCIncrementalIterIntentPolicyError { @@ -1806,7 +1812,11 @@ func cmdIterNewReadAsOf(e *evalCtx) error { opts.UpperBound = keys.MaxKey } r := e.newReader() - innerIter := newMetamorphicIterator(e.t, e.metamorphicIterSeed(), r.NewMVCCIterator(storage.MVCCKeyIterKind, opts)) + mvccIter, err := r.NewMVCCIterator(storage.MVCCKeyIterKind, opts) + if err != nil { + return err + } + innerIter := newMetamorphicIterator(e.t, e.metamorphicIterSeed(), mvccIter) iter := &iterWithCloser{innerIter, r.Close} e.iter = storage.NewReadAsOfIterator(iter, asOf) e.iterRangeKeys.Clear() diff --git a/pkg/storage/mvcc_incremental_iterator.go b/pkg/storage/mvcc_incremental_iterator.go index 42e1dc0c62a1..9211557940d5 100644 --- a/pkg/storage/mvcc_incremental_iterator.go +++ b/pkg/storage/mvcc_incremental_iterator.go @@ -174,8 +174,10 @@ type MVCCIncrementalIterOptions struct { // NewMVCCIncrementalIterator creates an MVCCIncrementalIterator with the // specified reader and options. The timestamp hint range should not be more // restrictive than the start and end time range. +// +// TODO(bilal): Update this method to take a storage.Reader and return an error func NewMVCCIncrementalIterator( - reader Reader, opts MVCCIncrementalIterOptions, + reader ReaderWithMustIterators, opts MVCCIncrementalIterOptions, ) *MVCCIncrementalIterator { // Default to MaxTimestamp for EndTime, since the code assumes it is set. if opts.EndTime.IsEmpty() { @@ -195,7 +197,7 @@ func NewMVCCIncrementalIterator( if useTBI { // An iterator without the timestamp hints is created to ensure that the // iterator visits every required version of every key that has changed. - iter = reader.NewMVCCIterator(MVCCKeyAndIntentsIterKind, IterOptions{ + iter = reader.MustMVCCIterator(MVCCKeyAndIntentsIterKind, IterOptions{ KeyTypes: opts.KeyTypes, LowerBound: opts.StartKey, UpperBound: opts.EndKey, @@ -210,7 +212,7 @@ func NewMVCCIncrementalIterator( if tbiRangeKeyMasking.LessEq(opts.StartTime) && opts.KeyTypes == IterKeyTypePointsAndRanges { tbiRangeKeyMasking = opts.StartTime.Next() } - timeBoundIter = reader.NewMVCCIterator(MVCCKeyIterKind, IterOptions{ + timeBoundIter = reader.MustMVCCIterator(MVCCKeyIterKind, IterOptions{ KeyTypes: opts.KeyTypes, LowerBound: opts.StartKey, UpperBound: opts.EndKey, @@ -221,7 +223,7 @@ func NewMVCCIncrementalIterator( RangeKeyMaskingBelow: tbiRangeKeyMasking, }) } else { - iter = reader.NewMVCCIterator(MVCCKeyAndIntentsIterKind, IterOptions{ + iter = reader.MustMVCCIterator(MVCCKeyAndIntentsIterKind, IterOptions{ KeyTypes: opts.KeyTypes, LowerBound: opts.StartKey, UpperBound: opts.EndKey, diff --git a/pkg/storage/mvcc_incremental_iterator_test.go b/pkg/storage/mvcc_incremental_iterator_test.go index 64381b0685d3..644cc7929e7c 100644 --- a/pkg/storage/mvcc_incremental_iterator_test.go +++ b/pkg/storage/mvcc_incremental_iterator_test.go @@ -1088,7 +1088,7 @@ func slurpKVsInTimeRange( reader Reader, prefix roachpb.Key, startTime, endTime hlc.Timestamp, ) ([]MVCCKeyValue, error) { endKey := prefix.PrefixEnd() - iter := NewMVCCIncrementalIterator(reader, MVCCIncrementalIterOptions{ + iter := NewMVCCIncrementalIterator(reader.(ReaderWithMustIterators), MVCCIncrementalIterOptions{ EndKey: endKey, StartTime: startTime, EndTime: endTime, @@ -1379,7 +1379,8 @@ func TestMVCCIncrementalIteratorIntentStraddlesSStables(t *testing.T) { { // Iterate over the entries in the first DB, ingesting them into SSTables // in the second DB. - it := db1.NewEngineIterator(IterOptions{UpperBound: keys.MaxKey}) + it, err := db1.NewEngineIterator(IterOptions{UpperBound: keys.MaxKey}) + require.NoError(t, err) defer it.Close() valid, err := it.SeekEngineKeyGE(EngineKey{Key: keys.LocalRangeLockTablePrefix}) ingest(it, valid, err, 1) @@ -1472,7 +1473,8 @@ func collectMatchingWithMVCCIterator( t *testing.T, eng Engine, start, end hlc.Timestamp, ) []MVCCKeyValue { var expectedKVs []MVCCKeyValue - iter := eng.NewMVCCIterator(MVCCKeyAndIntentsIterKind, IterOptions{UpperBound: roachpb.KeyMax}) + iter, err := eng.NewMVCCIterator(MVCCKeyAndIntentsIterKind, IterOptions{UpperBound: roachpb.KeyMax}) + require.NoError(t, err) defer iter.Close() iter.SeekGE(MVCCKey{Key: localMax}) for { diff --git a/pkg/storage/mvcc_stats_test.go b/pkg/storage/mvcc_stats_test.go index 498f7c215571..b80d8f95f705 100644 --- a/pkg/storage/mvcc_stats_test.go +++ b/pkg/storage/mvcc_stats_test.go @@ -1475,11 +1475,14 @@ var mvccStatsTests = []struct { { name: "ComputeStatsForIter", fn: func(r Reader, start, end roachpb.Key, nowNanos int64) (enginepb.MVCCStats, error) { - iter := r.NewMVCCIterator(MVCCKeyAndIntentsIterKind, IterOptions{ + iter, err := r.NewMVCCIterator(MVCCKeyAndIntentsIterKind, IterOptions{ KeyTypes: IterKeyTypePointsAndRanges, LowerBound: start, UpperBound: end, }) + if err != nil { + return enginepb.MVCCStats{}, err + } defer iter.Close() iter.SeekGE(MVCCKey{Key: start}) return ComputeStatsForIter(iter, nowNanos) diff --git a/pkg/storage/mvcc_test.go b/pkg/storage/mvcc_test.go index 5bc6a09bbbef..00a3d8708c10 100644 --- a/pkg/storage/mvcc_test.go +++ b/pkg/storage/mvcc_test.go @@ -834,7 +834,10 @@ func TestMVCCInvalidateIterator(t *testing.T) { { // Seek the iter to a valid position. - iter := batch.NewMVCCIterator(MVCCKeyAndIntentsIterKind, iterOptions) + iter, err := batch.NewMVCCIterator(MVCCKeyAndIntentsIterKind, iterOptions) + if err != nil { + t.Fatal(err) + } iter.SeekGE(MakeMVCCMetadataKey(key)) iter.Close() } @@ -848,9 +851,15 @@ func TestMVCCInvalidateIterator(t *testing.T) { case "findSplitKey": _, err = MVCCFindSplitKey(ctx, batch, roachpb.RKeyMin, roachpb.RKeyMax, 64<<20) case "computeStatsForIter": - iter := batch.NewMVCCIterator(MVCCKeyAndIntentsIterKind, iterOptions) + iter, err := batch.NewMVCCIterator(MVCCKeyAndIntentsIterKind, iterOptions) + if err != nil { + t.Fatal(err) + } iter.SeekGE(MVCCKey{Key: iterOptions.LowerBound}) _, err = ComputeStatsForIter(iter, 0) + if err != nil { + t.Fatal(err) + } iter.Close() } if err != nil { @@ -858,7 +867,10 @@ func TestMVCCInvalidateIterator(t *testing.T) { } // Verify that the iter is invalid. - iter := batch.NewMVCCIterator(MVCCKeyAndIntentsIterKind, iterOptions) + iter, err := batch.NewMVCCIterator(MVCCKeyAndIntentsIterKind, iterOptions) + if err != nil { + t.Fatal(err) + } defer iter.Close() if ok, _ := iter.Valid(); ok { t.Fatalf("iterator should not be valid") @@ -3726,8 +3738,11 @@ func checkEngineEquality( log.Infof(ctx, "checkEngineEquality") } makeIter := func(eng Engine) MVCCIterator { - iter := eng.NewMVCCIterator(MVCCKeyAndIntentsIterKind, + iter, err := eng.NewMVCCIterator(MVCCKeyAndIntentsIterKind, IterOptions{LowerBound: span.Key, UpperBound: span.EndKey}) + if err != nil { + t.Fatal(err) + } iter.SeekGE(MVCCKey{Key: span.Key}) return iter } @@ -3990,8 +4005,11 @@ func TestRandomizedSavepointRollbackAndIntentResolution(t *testing.T) { _, _, _, _, err = MVCCResolveWriteIntentRange(ctx, eng, nil, lu, MVCCResolveWriteIntentRangeOptions{}) require.NoError(t, err) { - iter := eng.NewMVCCIterator(MVCCKeyAndIntentsIterKind, + iter, err := eng.NewMVCCIterator(MVCCKeyAndIntentsIterKind, IterOptions{LowerBound: lu.Span.Key, UpperBound: lu.Span.EndKey}) + if err != nil { + t.Fatal(err) + } defer iter.Close() iter.SeekGE(MVCCKey{Key: lu.Span.Key}) valid, err := iter.Valid() @@ -4024,8 +4042,11 @@ func TestRandomizedSavepointRollbackAndIntentResolution(t *testing.T) { // Compact the engine so that SINGLEDEL consumes the SETWITHDEL, becoming a // DEL. require.NoError(t, eng.Compact()) - iter := eng.NewMVCCIterator(MVCCKeyAndIntentsIterKind, + iter, err := eng.NewMVCCIterator(MVCCKeyAndIntentsIterKind, IterOptions{LowerBound: lu.Span.Key, UpperBound: lu.Span.EndKey}) + if err != nil { + t.Fatal(err) + } defer iter.Close() iter.SeekGE(MVCCKey{Key: lu.Span.Key}) if lu.Status == roachpb.COMMITTED { @@ -4996,9 +5017,10 @@ type readWriterReturningSeekLTTrackingIterator struct { // NewMVCCIterator injects a seekLTTrackingIterator over the engine's real iterator. func (rw *readWriterReturningSeekLTTrackingIterator) NewMVCCIterator( iterKind MVCCIterKind, opts IterOptions, -) MVCCIterator { - rw.it.MVCCIterator = rw.ReadWriter.NewMVCCIterator(iterKind, opts) - return &rw.it +) (MVCCIterator, error) { + var err error + rw.it.MVCCIterator, err = rw.ReadWriter.NewMVCCIterator(iterKind, opts) + return &rw.it, err } // seekLTTrackingIterator is used to determine the number of times seekLT is @@ -5688,11 +5710,14 @@ func TestMVCCGarbageCollectRanges(t *testing.T) { require.NoError(t, MVCCGarbageCollectRangeKeys(ctx, engine, &ms, rangeKeys), "failed to run mvcc range tombstone garbage collect") - it := engine.NewMVCCIterator(MVCCKeyIterKind, IterOptions{ + it, err := engine.NewMVCCIterator(MVCCKeyIterKind, IterOptions{ KeyTypes: IterKeyTypeRangesOnly, LowerBound: d.rangeStart, UpperBound: d.rangeEnd, }) + if err != nil { + t.Fatal(err) + } defer it.Close() it.SeekGE(MVCCKey{Key: d.rangeStart}) expectIndex := 0 @@ -5914,11 +5939,14 @@ func TestMVCCGarbageCollectClearRange(t *testing.T) { require.Empty(t, ks) ms.AgeTo(tsMax.WallTime) - it := engine.NewMVCCIterator(MVCCKeyAndIntentsIterKind, IterOptions{ + it, err := engine.NewMVCCIterator(MVCCKeyAndIntentsIterKind, IterOptions{ KeyTypes: IterKeyTypePointsAndRanges, LowerBound: rangeStart, UpperBound: rangeEnd, }) + if err != nil { + t.Fatal(err) + } defer it.Close() expMs, err := ComputeStatsForIter(it, tsMax.WallTime) require.NoError(t, err, "failed to compute stats for range") @@ -7104,7 +7132,10 @@ func mvccGetRaw(t *testing.T, r Reader, key MVCCKey) []byte { } func mvccGetRawWithError(t *testing.T, r Reader, key MVCCKey) ([]byte, error) { - iter := r.NewMVCCIterator(MVCCKeyAndIntentsIterKind, IterOptions{Prefix: true}) + iter, err := r.NewMVCCIterator(MVCCKeyAndIntentsIterKind, IterOptions{Prefix: true}) + if err != nil { + t.Fatal(err) + } defer iter.Close() iter.SeekGE(key) if ok, err := iter.Valid(); err != nil || !ok { diff --git a/pkg/storage/pebble.go b/pkg/storage/pebble.go index bdc0b8e5f9f2..c890e5fac5e7 100644 --- a/pkg/storage/pebble.go +++ b/pkg/storage/pebble.go @@ -1485,24 +1485,54 @@ func (p *Pebble) MVCCIterate( } // NewMVCCIterator implements the Engine interface. -func (p *Pebble) NewMVCCIterator(iterKind MVCCIterKind, opts IterOptions) MVCCIterator { +func (p *Pebble) NewMVCCIterator(iterKind MVCCIterKind, opts IterOptions) (MVCCIterator, error) { if iterKind == MVCCKeyAndIntentsIterKind { r := wrapReader(p) // Doing defer r.Free() does not inline. - iter := r.NewMVCCIterator(iterKind, opts) + iter, err := r.NewMVCCIterator(iterKind, opts) r.Free() - return maybeWrapInUnsafeIter(iter) + if err != nil { + return nil, err + } + return maybeWrapInUnsafeIter(iter), nil } - iter := newPebbleIterator(p.db, opts, StandardDurability, p) - return maybeWrapInUnsafeIter(iter) + iter, err := newPebbleIterator(p.db, opts, StandardDurability, p) + if err != nil { + return nil, err + } + return maybeWrapInUnsafeIter(iter), nil +} + +// MustMVCCIterator implements the ReaderWithMustIterators interface. +// +// If the underlying DB struct in Pebble ever starts returning errors in +// NewIter(), this method must be removed. +func (p *Pebble) MustMVCCIterator(iterKind MVCCIterKind, opts IterOptions) MVCCIterator { + iter, err := p.NewMVCCIterator(iterKind, opts) + if err != nil { + panic(err) + } + return iter } // NewEngineIterator implements the Engine interface. -func (p *Pebble) NewEngineIterator(opts IterOptions) EngineIterator { +func (p *Pebble) NewEngineIterator(opts IterOptions) (EngineIterator, error) { return newPebbleIterator(p.db, opts, StandardDurability, p) } +// MustEngineIterator implements the ReaderWithMustIterators interface. +// +// If the underlying DB struct in Pebble ever starts returning errors in +// NewIter(), this method must be removed. +func (p *Pebble) MustEngineIterator(opts IterOptions) EngineIterator { + iter, err := p.NewEngineIterator(opts) + if err != nil { + panic(err) + } + return iter +} + // ScanInternal implements the Engine interface. func (p *Pebble) ScanInternal( ctx context.Context, @@ -2450,7 +2480,9 @@ func (p *pebbleReadOnly) MVCCIterate( } // NewMVCCIterator implements the Engine interface. -func (p *pebbleReadOnly) NewMVCCIterator(iterKind MVCCIterKind, opts IterOptions) MVCCIterator { +func (p *pebbleReadOnly) NewMVCCIterator( + iterKind MVCCIterKind, opts IterOptions, +) (MVCCIterator, error) { if p.closed { panic("using a closed pebbleReadOnly") } @@ -2458,9 +2490,12 @@ func (p *pebbleReadOnly) NewMVCCIterator(iterKind MVCCIterKind, opts IterOptions if iterKind == MVCCKeyAndIntentsIterKind { r := wrapReader(p) // Doing defer r.Free() does not inline. - iter := r.NewMVCCIterator(iterKind, opts) + iter, err := r.NewMVCCIterator(iterKind, opts) r.Free() - return maybeWrapInUnsafeIter(iter) + if err != nil { + return nil, err + } + return maybeWrapInUnsafeIter(iter), nil } iter := &p.normalIter @@ -2471,13 +2506,15 @@ func (p *pebbleReadOnly) NewMVCCIterator(iterKind MVCCIterKind, opts IterOptions return newPebbleIteratorByCloning(CloneContext{ rawIter: p.iter, engine: p.parent, - }, opts, p.durability) + }, opts, p.durability), nil } if iter.iter != nil { iter.setOptions(opts, p.durability) } else { - iter.initReuseOrCreate(p.parent.db, p.iter, p.iterUsed, opts, p.durability, p.parent) + if err := iter.initReuseOrCreate(p.parent.db, p.iter, p.iterUsed, opts, p.durability, p.parent); err != nil { + return nil, err + } if p.iter == nil { // For future cloning. p.iter = iter.iter @@ -2487,11 +2524,23 @@ func (p *pebbleReadOnly) NewMVCCIterator(iterKind MVCCIterKind, opts IterOptions } iter.inuse = true - return maybeWrapInUnsafeIter(iter) + return maybeWrapInUnsafeIter(iter), nil +} + +// MustMVCCIterator implements the ReaderWithMustIterators interface. +// +// If the underlying DB struct in Pebble ever starts returning errors in +// NewIter(), this method must be removed. +func (p *pebbleReadOnly) MustMVCCIterator(iterKind MVCCIterKind, opts IterOptions) MVCCIterator { + iter, err := p.NewMVCCIterator(iterKind, opts) + if err != nil { + panic(err) + } + return iter } // NewEngineIterator implements the Engine interface. -func (p *pebbleReadOnly) NewEngineIterator(opts IterOptions) EngineIterator { +func (p *pebbleReadOnly) NewEngineIterator(opts IterOptions) (EngineIterator, error) { if p.closed { panic("using a closed pebbleReadOnly") } @@ -2504,13 +2553,16 @@ func (p *pebbleReadOnly) NewEngineIterator(opts IterOptions) EngineIterator { return newPebbleIteratorByCloning(CloneContext{ rawIter: p.iter, engine: p.parent, - }, opts, p.durability) + }, opts, p.durability), nil } if iter.iter != nil { iter.setOptions(opts, p.durability) } else { - iter.initReuseOrCreate(p.parent.db, p.iter, p.iterUsed, opts, p.durability, p.parent) + err := iter.initReuseOrCreate(p.parent.db, p.iter, p.iterUsed, opts, p.durability, p.parent) + if err != nil { + return nil, err + } if p.iter == nil { // For future cloning. p.iter = iter.iter @@ -2520,6 +2572,18 @@ func (p *pebbleReadOnly) NewEngineIterator(opts IterOptions) EngineIterator { } iter.inuse = true + return iter, nil +} + +// MustEngineIterator implements the ReaderWithMustIterators interface. +// +// If the underlying DB struct in Pebble ever starts returning errors in +// NewIter(), this method must be removed. +func (p *pebbleReadOnly) MustEngineIterator(opts IterOptions) EngineIterator { + iter, err := p.NewEngineIterator(opts) + if err != nil { + panic(err) + } return iter } @@ -2535,7 +2599,11 @@ func (p *pebbleReadOnly) PinEngineStateForIterators() error { if p.durability == GuaranteedDurability { o = &pebble.IterOptions{OnlyReadGuaranteedDurable: true} } - p.iter = pebbleiter.MaybeWrap(p.parent.db.NewIter(o)) + iter, err := p.parent.db.NewIter(o) + if err != nil { + return err + } + p.iter = pebbleiter.MaybeWrap(iter) // NB: p.iterUsed == false avoids cloning this in NewMVCCIterator(), since // we've just created it. } @@ -2703,24 +2771,56 @@ func (p *pebbleSnapshot) MVCCIterate( } // NewMVCCIterator implements the Reader interface. -func (p *pebbleSnapshot) NewMVCCIterator(iterKind MVCCIterKind, opts IterOptions) MVCCIterator { +func (p *pebbleSnapshot) NewMVCCIterator( + iterKind MVCCIterKind, opts IterOptions, +) (MVCCIterator, error) { if iterKind == MVCCKeyAndIntentsIterKind { r := wrapReader(p) // Doing defer r.Free() does not inline. - iter := r.NewMVCCIterator(iterKind, opts) + iter, err := r.NewMVCCIterator(iterKind, opts) r.Free() - return maybeWrapInUnsafeIter(iter) + if err != nil { + return nil, err + } + return maybeWrapInUnsafeIter(iter), nil } - iter := MVCCIterator(newPebbleIterator(p.snapshot, opts, StandardDurability, p.parent)) - return maybeWrapInUnsafeIter(iter) + iter, err := newPebbleIterator(p.snapshot, opts, StandardDurability, p.parent) + if err != nil { + return nil, err + } + return maybeWrapInUnsafeIter(MVCCIterator(iter)), nil +} + +// MustMVCCIterator implements the ReaderWithMustIterators interface. +// +// If the underlying Snapshot struct in Pebble ever starts returning errors in +// NewIter(), this method must be removed. +func (p *pebbleSnapshot) MustMVCCIterator(iterKind MVCCIterKind, opts IterOptions) MVCCIterator { + iter, err := p.NewMVCCIterator(iterKind, opts) + if err != nil { + panic(err) + } + return iter } // NewEngineIterator implements the Reader interface. -func (p pebbleSnapshot) NewEngineIterator(opts IterOptions) EngineIterator { +func (p pebbleSnapshot) NewEngineIterator(opts IterOptions) (EngineIterator, error) { return newPebbleIterator(p.snapshot, opts, StandardDurability, p.parent) } +// MustEngineIterator implements the ReaderWithMustIterators interface. +// +// If the underlying Snapshot struct in Pebble ever starts returning errors in +// NewIter(), this method must be removed. +func (p *pebbleSnapshot) MustEngineIterator(opts IterOptions) EngineIterator { + iter, err := p.NewEngineIterator(opts) + if err != nil { + panic(err) + } + return iter +} + // ConsistentIterators implements the Reader interface. func (p pebbleSnapshot) ConsistentIterators() bool { return true diff --git a/pkg/storage/pebble_batch.go b/pkg/storage/pebble_batch.go index 2bcbd0519ead..ee690a5946d3 100644 --- a/pkg/storage/pebble_batch.go +++ b/pkg/storage/pebble_batch.go @@ -179,7 +179,9 @@ func (p *pebbleBatch) MVCCIterate( } // NewMVCCIterator implements the Batch interface. -func (p *pebbleBatch) NewMVCCIterator(iterKind MVCCIterKind, opts IterOptions) MVCCIterator { +func (p *pebbleBatch) NewMVCCIterator( + iterKind MVCCIterKind, opts IterOptions, +) (MVCCIterator, error) { if p.writeOnly { panic("write-only batch") } @@ -187,9 +189,12 @@ func (p *pebbleBatch) NewMVCCIterator(iterKind MVCCIterKind, opts IterOptions) M if iterKind == MVCCKeyAndIntentsIterKind { r := wrapReader(p) // Doing defer r.Free() does not inline. - iter := r.NewMVCCIterator(iterKind, opts) + iter, err := r.NewMVCCIterator(iterKind, opts) r.Free() - return maybeWrapInUnsafeIter(iter) + if err != nil { + return nil, err + } + return maybeWrapInUnsafeIter(iter), nil } iter := &p.normalIter @@ -204,13 +209,15 @@ func (p *pebbleBatch) NewMVCCIterator(iterKind MVCCIterKind, opts IterOptions) M return newPebbleIteratorByCloning(CloneContext{ rawIter: p.iter, engine: p.parent, - }, opts, StandardDurability) + }, opts, StandardDurability), nil } if iter.iter != nil { iter.setOptions(opts, StandardDurability) } else { - iter.initReuseOrCreate(handle, p.iter, p.iterUsed, opts, StandardDurability, p.parent) + if err := iter.initReuseOrCreate(handle, p.iter, p.iterUsed, opts, StandardDurability, p.parent); err != nil { + return nil, err + } if p.iter == nil { // For future cloning. p.iter = iter.iter @@ -219,11 +226,23 @@ func (p *pebbleBatch) NewMVCCIterator(iterKind MVCCIterKind, opts IterOptions) M } iter.inuse = true - return maybeWrapInUnsafeIter(iter) + return maybeWrapInUnsafeIter(iter), nil +} + +// MustMVCCIterator implements the ReaderWithMustIterators interface. +// +// If the underlying Batch struct in Pebble ever starts returning errors in +// NewIter(), this method must be removed. +func (p *pebbleBatch) MustMVCCIterator(iterKind MVCCIterKind, opts IterOptions) MVCCIterator { + iter, err := p.NewMVCCIterator(iterKind, opts) + if err != nil { + panic(err) + } + return iter } // NewEngineIterator implements the Batch interface. -func (p *pebbleBatch) NewEngineIterator(opts IterOptions) EngineIterator { +func (p *pebbleBatch) NewEngineIterator(opts IterOptions) (EngineIterator, error) { if p.writeOnly { panic("write-only batch") } @@ -240,13 +259,15 @@ func (p *pebbleBatch) NewEngineIterator(opts IterOptions) EngineIterator { return newPebbleIteratorByCloning(CloneContext{ rawIter: p.iter, engine: p.parent, - }, opts, StandardDurability) + }, opts, StandardDurability), nil } if iter.iter != nil { iter.setOptions(opts, StandardDurability) } else { - iter.initReuseOrCreate(handle, p.iter, p.iterUsed, opts, StandardDurability, p.parent) + if err := iter.initReuseOrCreate(handle, p.iter, p.iterUsed, opts, StandardDurability, p.parent); err != nil { + return nil, err + } if p.iter == nil { // For future cloning. p.iter = iter.iter @@ -255,6 +276,18 @@ func (p *pebbleBatch) NewEngineIterator(opts IterOptions) EngineIterator { } iter.inuse = true + return iter, nil +} + +// MustEngineIterator implements the ReaderWithMustIterators interface. +// +// If the underlying Batch struct in Pebble ever starts returning errors in +// NewIter(), this method must be removed. +func (p *pebbleBatch) MustEngineIterator(opts IterOptions) EngineIterator { + iter, err := p.NewEngineIterator(opts) + if err != nil { + panic(err) + } return iter } @@ -282,12 +315,18 @@ func (p *pebbleBatch) ConsistentIterators() bool { // PinEngineStateForIterators implements the Batch interface. func (p *pebbleBatch) PinEngineStateForIterators() error { + var err error if p.iter == nil { + var iter *pebble.Iterator if p.batch.Indexed() { - p.iter = pebbleiter.MaybeWrap(p.batch.NewIter(nil)) + iter, err = p.batch.NewIter(nil) } else { - p.iter = pebbleiter.MaybeWrap(p.db.NewIter(nil)) + iter, err = p.db.NewIter(nil) } + if err != nil { + return err + } + p.iter = pebbleiter.MaybeWrap(iter) // NB: p.iterUsed == false avoids cloning this in NewMVCCIterator(). We've // just created it, so cloning it would just be overhead. } @@ -395,11 +434,14 @@ func (p *pebbleBatch) ClearMVCCIteratorRange( start, end roachpb.Key, pointKeys, rangeKeys bool, ) error { clearPointKeys := func(start, end roachpb.Key) error { - iter := p.NewMVCCIterator(MVCCKeyAndIntentsIterKind, IterOptions{ + iter, err := p.NewMVCCIterator(MVCCKeyAndIntentsIterKind, IterOptions{ KeyTypes: IterKeyTypePointsOnly, LowerBound: start, UpperBound: end, }) + if err != nil { + return err + } defer iter.Close() for iter.SeekGE(MVCCKey{Key: start}); ; iter.Next() { if valid, err := iter.Valid(); err != nil { @@ -423,11 +465,14 @@ func (p *pebbleBatch) ClearMVCCIteratorRange( } clearRangeKeys := func(start, end roachpb.Key) error { - iter := p.NewMVCCIterator(MVCCKeyIterKind, IterOptions{ + iter, err := p.NewMVCCIterator(MVCCKeyIterKind, IterOptions{ KeyTypes: IterKeyTypeRangesOnly, LowerBound: start, UpperBound: end, }) + if err != nil { + return err + } defer iter.Close() for iter.SeekGE(MVCCKey{Key: start}); ; iter.Next() { if valid, err := iter.Valid(); err != nil { diff --git a/pkg/storage/pebble_iterator.go b/pkg/storage/pebble_iterator.go index a0a526bfa391..b6c3c2d39923 100644 --- a/pkg/storage/pebble_iterator.go +++ b/pkg/storage/pebble_iterator.go @@ -87,12 +87,16 @@ var pebbleIterPool = sync.Pool{ // newPebbleIterator creates a new Pebble iterator for the given Pebble reader. func newPebbleIterator( handle pebble.Reader, opts IterOptions, durability DurabilityRequirement, parent *Pebble, -) *pebbleIterator { +) (*pebbleIterator, error) { p := pebbleIterPool.Get().(*pebbleIterator) p.reusable = false // defensive p.init(nil, opts, durability, parent) - p.iter = pebbleiter.MaybeWrap(handle.NewIter(&p.options)) - return p + iter, err := handle.NewIter(&p.options) + if err != nil { + return nil, err + } + p.iter = pebbleiter.MaybeWrap(iter) + return p, nil } // newPebbleIteratorByCloning creates a new Pebble iterator by cloning the given @@ -173,15 +177,19 @@ func (p *pebbleIterator) initReuseOrCreate( opts IterOptions, durability DurabilityRequirement, statsReporter *Pebble, -) { +) error { if iter != nil && !clone { p.init(iter, opts, durability, statsReporter) - return + return nil } p.init(nil, opts, durability, statsReporter) if iter == nil { - p.iter = pebbleiter.MaybeWrap(handle.NewIter(&p.options)) + innerIter, err := handle.NewIter(&p.options) + if err != nil { + return err + } + p.iter = pebbleiter.MaybeWrap(innerIter) } else if clone { var err error p.iter, err = iter.Clone(pebble.CloneOptions{ @@ -190,9 +198,10 @@ func (p *pebbleIterator) initReuseOrCreate( }) if err != nil { p.Close() - panic(err) + return err } } + return nil } // setOptions updates the options for a pebbleIterator. If p.iter is non-nil, it diff --git a/pkg/storage/pebble_iterator_test.go b/pkg/storage/pebble_iterator_test.go index c2bff73d42ed..3c6cf8349360 100644 --- a/pkg/storage/pebble_iterator_test.go +++ b/pkg/storage/pebble_iterator_test.go @@ -73,7 +73,8 @@ func TestPebbleIterator_Corruption(t *testing.T) { LowerBound: []byte("a"), UpperBound: []byte("z"), } - iter := newPebbleIterator(p.db, iterOpts, StandardDurability, p) + iter, err := newPebbleIterator(p.db, iterOpts, StandardDurability, p) + require.NoError(t, err) // Seeking into the table catches the corruption. ok, err := iter.SeekEngineKeyGE(ek) diff --git a/pkg/storage/pebble_mvcc_scanner_test.go b/pkg/storage/pebble_mvcc_scanner_test.go index 7de6ddc30b56..4f388c760c72 100644 --- a/pkg/storage/pebble_mvcc_scanner_test.go +++ b/pkg/storage/pebble_mvcc_scanner_test.go @@ -76,8 +76,9 @@ func TestMVCCScanWithManyVersionsAndSeparatedIntents(t *testing.T) { reader := eng.NewReadOnly(StandardDurability) defer reader.Close() - iter := reader.NewMVCCIterator( + iter, err := reader.NewMVCCIterator( MVCCKeyAndIntentsIterKind, IterOptions{LowerBound: keys[0], UpperBound: roachpb.Key("d")}) + require.NoError(t, err) defer iter.Close() // Look for older versions that come after the scanner has exhausted its @@ -142,8 +143,9 @@ func TestMVCCScanWithLargeKeyValue(t *testing.T) { reader := eng.NewReadOnly(StandardDurability) defer reader.Close() - iter := reader.NewMVCCIterator( + iter, err := reader.NewMVCCIterator( MVCCKeyAndIntentsIterKind, IterOptions{LowerBound: keys[0], UpperBound: roachpb.Key("e")}) + require.NoError(t, err) defer iter.Close() ts := hlc.Timestamp{WallTime: 2} @@ -156,7 +158,7 @@ func TestMVCCScanWithLargeKeyValue(t *testing.T) { } var results pebbleResults mvccScanner.init(nil /* txn */, uncertainty.Interval{}, &results) - _, _, _, err := mvccScanner.scan(context.Background()) + _, _, _, err = mvccScanner.scan(context.Background()) require.NoError(t, err) kvData := results.finish() @@ -221,8 +223,9 @@ func TestMVCCScanWithMemoryAccounting(t *testing.T) { }() // iterator that can span over all the written keys. - iter := eng.NewMVCCIterator(MVCCKeyAndIntentsIterKind, + iter, err := eng.NewMVCCIterator(MVCCKeyAndIntentsIterKind, IterOptions{LowerBound: makeKey(nil, 0), UpperBound: makeKey(nil, 11)}) + require.NoError(t, err) defer iter.Close() // Narrow scan succeeds with a budget of 6000. diff --git a/pkg/storage/pebble_test.go b/pkg/storage/pebble_test.go index 9329425e8c11..201ba2f24bd4 100644 --- a/pkg/storage/pebble_test.go +++ b/pkg/storage/pebble_test.go @@ -99,7 +99,10 @@ func TestPebbleIterReuse(t *testing.T) { } } - iter1 := batch.NewMVCCIterator(MVCCKeyAndIntentsIterKind, IterOptions{LowerBound: []byte{40}, UpperBound: []byte{50}}) + iter1, err := batch.NewMVCCIterator(MVCCKeyAndIntentsIterKind, IterOptions{LowerBound: []byte{40}, UpperBound: []byte{50}}) + if err != nil { + t.Fatal(err) + } valuesCount := 0 // Seek to a value before the lower bound. Identical to seeking to the lower bound. iter1.SeekGE(MVCCKey{Key: []byte{30}}) @@ -128,7 +131,10 @@ func TestPebbleIterReuse(t *testing.T) { // is lower than the previous iterator's lower bound. This should still result // in the right amount of keys being returned; the lower bound from the // previous iterator should get zeroed. - iter2 := batch.NewMVCCIterator(MVCCKeyAndIntentsIterKind, IterOptions{UpperBound: []byte{10}}) + iter2, err := batch.NewMVCCIterator(MVCCKeyAndIntentsIterKind, IterOptions{UpperBound: []byte{10}}) + if err != nil { + t.Fatal(err) + } valuesCount = 0 // This is a peculiar test that is disregarding how local and global keys // affect the behavior of MVCCIterators. This test is writing []byte{0} @@ -313,9 +319,15 @@ func TestPebbleIterConsistency(t *testing.T) { // Since an iterator is created on pebbleReadOnly, pebbleBatch before // writing a newer version of "a", the newer version will not be visible to // iterators that are created later. - roEngine.NewMVCCIterator(MVCCKeyIterKind, IterOptions{UpperBound: []byte("a")}).Close() - batch.NewMVCCIterator(MVCCKeyIterKind, IterOptions{UpperBound: []byte("a")}).Close() - eng.NewMVCCIterator(MVCCKeyIterKind, IterOptions{UpperBound: []byte("a")}).Close() + iter, err := roEngine.NewMVCCIterator(MVCCKeyIterKind, IterOptions{UpperBound: []byte("a")}) + require.NoError(t, err) + iter.Close() + batchIter, err := batch.NewMVCCIterator(MVCCKeyIterKind, IterOptions{UpperBound: []byte("a")}) + require.NoError(t, err) + batchIter.Close() + engIter, err := eng.NewMVCCIterator(MVCCKeyIterKind, IterOptions{UpperBound: []byte("a")}) + require.NoError(t, err) + engIter.Close() // Pin the state for iterators. require.Nil(t, roEngine2.PinEngineStateForIterators()) require.Nil(t, batch2.PinEngineStateForIterators()) @@ -325,7 +337,8 @@ func TestPebbleIterConsistency(t *testing.T) { v2 := MVCCValue{Value: roachpb.MakeValueFromString("a2")} require.NoError(t, eng.PutMVCC(k2, v2)) - checkMVCCIter := func(iter MVCCIterator) { + checkMVCCIter := func(iter MVCCIterator, err error) { + require.NoError(t, err) defer iter.Close() iter.SeekGE(MVCCKey{Key: []byte("a")}) valid, err := iter.Valid() @@ -338,7 +351,8 @@ func TestPebbleIterConsistency(t *testing.T) { require.False(t, valid) require.NoError(t, err) } - checkEngineIter := func(iter EngineIterator) { + checkEngineIter := func(iter EngineIterator, err error) { + require.NoError(t, err) defer iter.Close() valid, err := iter.SeekEngineKeyGE(EngineKey{Key: []byte("a")}) require.Equal(t, true, valid) @@ -373,7 +387,8 @@ func TestPebbleIterConsistency(t *testing.T) { checkEngineIter(batch2.NewEngineIterator(IterOptions{UpperBound: []byte("b")})) checkEngineIter(batch2.NewEngineIterator(IterOptions{Prefix: true})) - checkIterSeesBothValues := func(iter MVCCIterator) { + checkIterSeesBothValues := func(iter MVCCIterator, err error) { + require.NoError(t, err) defer iter.Close() iter.SeekGE(MVCCKey{Key: []byte("a")}) count := 0 @@ -745,13 +760,13 @@ func TestPebbleMVCCTimeIntervalCollectorAndFilter(t *testing.T) { } expect = append(expect, kv) } - - iter := eng.NewMVCCIterator(MVCCKeyIterKind, IterOptions{ + iter, err := eng.NewMVCCIterator(MVCCKeyIterKind, IterOptions{ KeyTypes: keyType, UpperBound: keys.MaxKey, MinTimestampHint: tc.minTimestamp, MaxTimestampHint: tc.maxTimestamp, }) + require.NoError(t, err) defer iter.Close() require.Equal(t, expect, scanIter(t, iter)) }) @@ -866,13 +881,15 @@ func TestPebbleMVCCTimeIntervalWithClears(t *testing.T) { } expect = append(expect, kv) } - - iter := eng.NewMVCCIterator(MVCCKeyIterKind, IterOptions{ + iter, err := eng.NewMVCCIterator(MVCCKeyIterKind, IterOptions{ KeyTypes: keyType, UpperBound: keys.MaxKey, MinTimestampHint: tc.minTimestamp, MaxTimestampHint: tc.maxTimestamp, }) + if err != nil { + t.Fatal(err) + } defer iter.Close() require.Equal(t, expect, scanIter(t, iter)) }) @@ -946,13 +963,15 @@ func TestPebbleMVCCTimeIntervalWithRangeClears(t *testing.T) { } expect = append(expect, kv) } - - iter := eng.NewMVCCIterator(MVCCKeyIterKind, IterOptions{ + iter, err := eng.NewMVCCIterator(MVCCKeyIterKind, IterOptions{ KeyTypes: keyType, UpperBound: keys.MaxKey, MinTimestampHint: tc.minTimestamp, MaxTimestampHint: tc.maxTimestamp, }) + if err != nil { + t.Fatal(err) + } defer iter.Close() require.Equal(t, expect, scanIter(t, iter)) }) @@ -1014,11 +1033,14 @@ func TestPebbleTablePropertyFilter(t *testing.T) { } for name, tc := range testcases { t.Run(name, func(t *testing.T) { - iter := eng.NewMVCCIterator(MVCCKeyIterKind, IterOptions{ + iter, err := eng.NewMVCCIterator(MVCCKeyIterKind, IterOptions{ UpperBound: keys.MaxKey, MinTimestampHint: hlc.Timestamp{WallTime: tc.minTimestamp}, MaxTimestampHint: hlc.Timestamp{WallTime: tc.maxTimestamp}, }) + if err != nil { + t.Fatal(err) + } defer iter.Close() kvs := scanIter(t, iter) @@ -1079,7 +1101,10 @@ func TestPebbleFlushCallbackAndDurabilityRequirement(t *testing.T) { // Returns the value found or nil. checkGetAndIter := func(reader Reader) []byte { v := mvccGetRaw(t, reader, k) - iter := reader.NewMVCCIterator(MVCCKeyIterKind, IterOptions{UpperBound: k.Key.Next()}) + iter, err := reader.NewMVCCIterator(MVCCKeyIterKind, IterOptions{UpperBound: k.Key.Next()}) + if err != nil { + t.Fatal(err) + } defer iter.Close() iter.SeekGE(k) valid, err := iter.Valid() @@ -1166,8 +1191,14 @@ func TestPebbleReaderMultipleIterators(t *testing.T) { for name, r := range testcases { t.Run(name, func(t *testing.T) { // Make sure we can create two iterators of the same type. - i1 := r.NewMVCCIterator(MVCCKeyIterKind, IterOptions{LowerBound: a1.Key, UpperBound: keys.MaxKey}) - i2 := r.NewMVCCIterator(MVCCKeyIterKind, IterOptions{LowerBound: b1.Key, UpperBound: keys.MaxKey}) + i1, err := r.NewMVCCIterator(MVCCKeyIterKind, IterOptions{LowerBound: a1.Key, UpperBound: keys.MaxKey}) + if err != nil { + t.Fatal(err) + } + i2, err := r.NewMVCCIterator(MVCCKeyIterKind, IterOptions{LowerBound: b1.Key, UpperBound: keys.MaxKey}) + if err != nil { + t.Fatal(err) + } // Make sure the iterators are independent. i1.SeekGE(a1) @@ -1191,9 +1222,15 @@ func TestPebbleReaderMultipleIterators(t *testing.T) { i2.Close() // Quick check for engine iterators too. - e1 := r.NewEngineIterator(IterOptions{UpperBound: keys.MaxKey}) + e1, err := r.NewEngineIterator(IterOptions{UpperBound: keys.MaxKey}) + if err != nil { + t.Fatal(err) + } defer e1.Close() - e2 := r.NewEngineIterator(IterOptions{UpperBound: keys.MaxKey}) + e2, err := r.NewEngineIterator(IterOptions{UpperBound: keys.MaxKey}) + if err != nil { + t.Fatal(err) + } defer e2.Close() }) } diff --git a/pkg/storage/read_as_of_iterator_test.go b/pkg/storage/read_as_of_iterator_test.go index 64ddfdb6a8df..6f640a49a054 100644 --- a/pkg/storage/read_as_of_iterator_test.go +++ b/pkg/storage/read_as_of_iterator_test.go @@ -86,7 +86,8 @@ func TestReadAsOfIterator(t *testing.T) { batch := pebble.NewBatch() defer batch.Close() populateBatch(t, batch, test.input) - iter := batch.NewMVCCIterator(MVCCKeyAndIntentsIterKind, IterOptions{UpperBound: roachpb.KeyMax}) + iter, err := batch.NewMVCCIterator(MVCCKeyAndIntentsIterKind, IterOptions{UpperBound: roachpb.KeyMax}) + require.NoError(t, err) defer iter.Close() subtests := []iterSubtest{ @@ -159,7 +160,8 @@ func TestReadAsOfIteratorSeek(t *testing.T) { batch := pebble.NewBatch() defer batch.Close() populateBatch(t, batch, test.input) - iter := batch.NewMVCCIterator(MVCCKeyAndIntentsIterKind, IterOptions{UpperBound: roachpb.KeyMax}) + iter, err := batch.NewMVCCIterator(MVCCKeyAndIntentsIterKind, IterOptions{UpperBound: roachpb.KeyMax}) + require.NoError(t, err) defer iter.Close() asOf := hlc.Timestamp{} diff --git a/pkg/storage/sst.go b/pkg/storage/sst.go index 2834adc76d88..bec2c3ccaae4 100644 --- a/pkg/storage/sst.go +++ b/pkg/storage/sst.go @@ -146,10 +146,13 @@ func CheckSSTConflicts( // first, where there are no keys in the reader between the sstable's start // and end keys. We use a non-prefix iterator for this search, and reopen a // prefix one if there are engine keys in the span. - nonPrefixIter := reader.NewMVCCIterator(MVCCKeyAndIntentsIterKind, IterOptions{ + nonPrefixIter, err := reader.NewMVCCIterator(MVCCKeyAndIntentsIterKind, IterOptions{ KeyTypes: IterKeyTypePointsAndRanges, UpperBound: end.Key, }) + if err != nil { + return statsDiff, err + } nonPrefixIter.SeekGE(start) valid, err := nonPrefixIter.Valid() nonPrefixIter.Close() @@ -183,10 +186,13 @@ func CheckSSTConflicts( } rkIter.Close() - rkIter = reader.NewMVCCIterator(MVCCKeyIterKind, IterOptions{ + rkIter, err = reader.NewMVCCIterator(MVCCKeyIterKind, IterOptions{ UpperBound: rightPeekBound, KeyTypes: IterKeyTypeRangesOnly, }) + if err != nil { + return enginepb.MVCCStats{}, err + } rkIter.SeekGE(start) var engineHasRangeKeys bool @@ -220,7 +226,7 @@ func CheckSSTConflicts( // https://github.com/cockroachdb/cockroach/issues/92254 statsDiff.ContainsEstimates += 2 } - extIter := reader.NewMVCCIterator(MVCCKeyAndIntentsIterKind, IterOptions{ + extIter, err := reader.NewMVCCIterator(MVCCKeyAndIntentsIterKind, IterOptions{ KeyTypes: IterKeyTypePointsAndRanges, LowerBound: leftPeekBound, UpperBound: rightPeekBound, @@ -228,6 +234,9 @@ func CheckSSTConflicts( Prefix: usePrefixSeek, useL6Filters: true, }) + if err != nil { + return enginepb.MVCCStats{}, err + } defer extIter.Close() sstIter, err := NewMemSSTIterator(sst, false, IterOptions{ diff --git a/pkg/testutils/storageutils/mvcc.go b/pkg/testutils/storageutils/mvcc.go index ea883834a2d0..b76baf791154 100644 --- a/pkg/testutils/storageutils/mvcc.go +++ b/pkg/testutils/storageutils/mvcc.go @@ -27,7 +27,10 @@ func MVCCGetRaw(t *testing.T, r storage.Reader, key storage.MVCCKey) []byte { // MVCCGetRawWithError is like MVCCGetRaw, but returns an error rather than // failing the test. func MVCCGetRawWithError(t *testing.T, r storage.Reader, key storage.MVCCKey) ([]byte, error) { - iter := r.NewMVCCIterator(storage.MVCCKeyAndIntentsIterKind, storage.IterOptions{Prefix: true}) + iter, err := r.NewMVCCIterator(storage.MVCCKeyAndIntentsIterKind, storage.IterOptions{Prefix: true}) + if err != nil { + return nil, err + } defer iter.Close() iter.SeekGE(key) if ok, err := iter.Valid(); err != nil || !ok { diff --git a/pkg/testutils/storageutils/scan.go b/pkg/testutils/storageutils/scan.go index b60bbea8eb1a..d51b0ad9c73b 100644 --- a/pkg/testutils/storageutils/scan.go +++ b/pkg/testutils/storageutils/scan.go @@ -87,11 +87,14 @@ func ScanIter(t *testing.T, iter storage.SimpleMVCCIterator) KVs { func ScanKeySpan(t *testing.T, r storage.Reader, start, end roachpb.Key) KVs { t.Helper() - iter := r.NewMVCCIterator(storage.MVCCKeyAndIntentsIterKind, storage.IterOptions{ + iter, err := r.NewMVCCIterator(storage.MVCCKeyAndIntentsIterKind, storage.IterOptions{ KeyTypes: storage.IterKeyTypePointsAndRanges, LowerBound: start, UpperBound: end, }) + if err != nil { + panic(err) + } defer iter.Close() return ScanIter(t, iter) } diff --git a/pkg/ts/pruning.go b/pkg/ts/pruning.go index da111461cc15..677cb291b4d5 100644 --- a/pkg/ts/pruning.go +++ b/pkg/ts/pruning.go @@ -64,7 +64,10 @@ func (tsdb *DB) findTimeSeries( thresholds := tsdb.computeThresholds(now.WallTime) // NB: timeseries don't have intents. - iter := reader.NewMVCCIterator(storage.MVCCKeyIterKind, storage.IterOptions{UpperBound: endKey.AsRawKey()}) + iter, err := reader.NewMVCCIterator(storage.MVCCKeyIterKind, storage.IterOptions{UpperBound: endKey.AsRawKey()}) + if err != nil { + return nil, err + } defer iter.Close() for iter.SeekGE(next); ; iter.SeekGE(next) {