From 859ba50481daa5114afedbff4ab99b9cea43fab5 Mon Sep 17 00:00:00 2001 From: Erik Grinaker Date: Sun, 10 Apr 2022 18:07:03 +0000 Subject: [PATCH 1/2] storage: fix batch reads with multiple concurrent iterators Release note: None --- pkg/storage/pebble_batch.go | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/pkg/storage/pebble_batch.go b/pkg/storage/pebble_batch.go index c64dea4b77a1..6bc4043fd66f 100644 --- a/pkg/storage/pebble_batch.go +++ b/pkg/storage/pebble_batch.go @@ -215,18 +215,18 @@ func (p *pebbleBatch) NewMVCCIterator(iterKind MVCCIterKind, opts IterOptions) M if opts.Prefix { iter = &p.prefixIter } + handle := pebble.Reader(p.batch) + if !p.batch.Indexed() { + handle = p.db + } if iter.inuse { - return newPebbleIterator(p.db, p.iter, opts, StandardDurability) + return newPebbleIterator(handle, p.iter, opts, StandardDurability) } if iter.iter != nil { iter.setOptions(opts, StandardDurability) } else { - if p.batch.Indexed() { - iter.init(p.batch, p.iter, p.iterUnused, opts, StandardDurability) - } else { - iter.init(p.db, p.iter, p.iterUnused, opts, StandardDurability) - } + iter.init(handle, p.iter, p.iterUnused, opts, StandardDurability) if p.iter == nil { // For future cloning. p.iter = iter.iter @@ -252,18 +252,18 @@ func (p *pebbleBatch) NewEngineIterator(opts IterOptions) EngineIterator { if opts.Prefix { iter = &p.prefixEngineIter } + handle := pebble.Reader(p.batch) + if !p.batch.Indexed() { + handle = p.db + } if iter.inuse { - return newPebbleIterator(p.db, p.iter, opts, StandardDurability) + return newPebbleIterator(handle, p.iter, opts, StandardDurability) } if iter.iter != nil { iter.setOptions(opts, StandardDurability) } else { - if p.batch.Indexed() { - iter.init(p.batch, p.iter, p.iterUnused, opts, StandardDurability) - } else { - iter.init(p.db, p.iter, p.iterUnused, opts, StandardDurability) - } + iter.init(handle, p.iter, p.iterUnused, opts, StandardDurability) if p.iter == nil { // For future cloning. p.iter = iter.iter From 19748e39f30189dcc213b90f4e7fa2eb38be1d2b Mon Sep 17 00:00:00 2001 From: Erik Grinaker Date: Sun, 10 Apr 2022 18:09:28 +0000 Subject: [PATCH 2/2] storage: minor code cleanups Release note: None --- pkg/storage/intent_interleaving_iter.go | 9 +-------- pkg/storage/pebble_iterator.go | 21 +++++++++------------ 2 files changed, 10 insertions(+), 20 deletions(-) diff --git a/pkg/storage/intent_interleaving_iter.go b/pkg/storage/intent_interleaving_iter.go index bd9b92365a45..ba038a23be93 100644 --- a/pkg/storage/intent_interleaving_iter.go +++ b/pkg/storage/intent_interleaving_iter.go @@ -233,7 +233,7 @@ func newIntentInterleavingIterator(reader Reader, opts IterOptions) MVCCIterator if reader.ConsistentIterators() { iter = reader.NewMVCCIterator(MVCCKeyIterKind, opts) } else { - iter = newMVCCIteratorByCloningEngineIter(intentIter, opts) + iter = newPebbleIterator(nil, intentIter.GetRawIter(), opts, StandardDurability) } *iiIter = intentInterleavingIter{ @@ -973,13 +973,6 @@ func (i *intentInterleavingIter) SupportsPrev() bool { return true } -// newMVCCIteratorByCloningEngineIter assumes MVCCKeyIterKind and no timestamp -// hints. It uses pebble.Iterator.Clone to ensure that the two iterators see -// the identical engine state. -func newMVCCIteratorByCloningEngineIter(iter EngineIterator, opts IterOptions) MVCCIterator { - return newPebbleIterator(nil, iter.GetRawIter(), opts, StandardDurability) -} - // unsageMVCCIterator is used in RaceEnabled test builds to randomly inject // changes to unsafe keys retrieved from MVCCIterators. type unsafeMVCCIterator struct { diff --git a/pkg/storage/pebble_iterator.go b/pkg/storage/pebble_iterator.go index 5fd598c5ad1f..f8f57000657c 100644 --- a/pkg/storage/pebble_iterator.go +++ b/pkg/storage/pebble_iterator.go @@ -35,12 +35,9 @@ type pebbleIterator struct { // Reusable buffer for MVCCKey or EngineKey encoding. keyBuf []byte // Buffers for copying iterator bounds to. Note that the underlying memory - // is not GCed upon Close(), to reduce the number of overall allocations. We - // use two slices for each of the bounds since this caller should not change - // the slice holding the current bounds, that the callee (pebble.MVCCIterator) - // is currently using, until after the caller has made the SetBounds call. - lowerBoundBuf [2][]byte - upperBoundBuf [2][]byte + // is not GCed upon Close(), to reduce the number of overall allocations. + lowerBoundBuf []byte + upperBoundBuf []byte // Set to true to govern whether to call SeekPrefixGE or SeekGE. Skips // SSTables based on MVCC/Engine key when true. @@ -184,15 +181,15 @@ func (p *pebbleIterator) setOptions(opts IterOptions, durability DurabilityRequi // Since we are encoding keys with an empty version anyway, we can just // append the NUL byte instead of calling the above encode functions which // will do the same thing. - p.lowerBoundBuf[0] = append(p.lowerBoundBuf[0][:0], opts.LowerBound...) - p.lowerBoundBuf[0] = append(p.lowerBoundBuf[0], 0x00) - p.options.LowerBound = p.lowerBoundBuf[0] + p.lowerBoundBuf = append(p.lowerBoundBuf[:0], opts.LowerBound...) + p.lowerBoundBuf = append(p.lowerBoundBuf, 0x00) + p.options.LowerBound = p.lowerBoundBuf } if opts.UpperBound != nil { // Same as above. - p.upperBoundBuf[0] = append(p.upperBoundBuf[0][:0], opts.UpperBound...) - p.upperBoundBuf[0] = append(p.upperBoundBuf[0], 0x00) - p.options.UpperBound = p.upperBoundBuf[0] + p.upperBoundBuf = append(p.upperBoundBuf[:0], opts.UpperBound...) + p.upperBoundBuf = append(p.upperBoundBuf, 0x00) + p.options.UpperBound = p.upperBoundBuf } if opts.MaxTimestampHint.IsSet() {