From 550d48368f2875bf9faf6e14f8f4d51021d96233 Mon Sep 17 00:00:00 2001 From: Peter Mattis Date: Thu, 24 Oct 2019 07:29:59 -0400 Subject: [PATCH] storage/engine: fix pebbleBatch.iter reuse `pebbleBatch.NewIterator` was setting `pebbleBatch.iter.inuse = true`, and then calling `pebbleIterator.init` which was clearing that field. This was broken by #41859 which refactored how `pebbleBatch` iterator reuse works. Added separate cached prefix and normal iterators to `pebble{Batch,ReadOnly}`. Various bits of higher-level code expect to be able to have a prefix and normal iterator open at the same time. In particularly, this comes up during intent resolution. This also mimics our usage of RocksDB which seems desirable in the short term even though the semantics of having two cached iterators is slightly odd. Fixes #41899 --- pkg/storage/engine/mvcc_logical_ops_test.go | 7 +-- pkg/storage/engine/pebble.go | 29 +++++++----- pkg/storage/engine/pebble_batch.go | 52 +++++++++++++-------- pkg/storage/engine/pebble_iterator.go | 15 ++++-- 4 files changed, 66 insertions(+), 37 deletions(-) diff --git a/pkg/storage/engine/mvcc_logical_ops_test.go b/pkg/storage/engine/mvcc_logical_ops_test.go index c857efddeefe..d0d44b181247 100644 --- a/pkg/storage/engine/mvcc_logical_ops_test.go +++ b/pkg/storage/engine/mvcc_logical_ops_test.go @@ -13,7 +13,7 @@ package engine import ( "context" "math" - "reflect" + "strings" "testing" "github.com/cockroachdb/cockroach/pkg/keys" @@ -21,6 +21,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/storage/engine/enginepb" "github.com/cockroachdb/cockroach/pkg/util/hlc" "github.com/cockroachdb/cockroach/pkg/util/leaktest" + "github.com/kr/pretty" ) func TestMVCCOpLogWriter(t *testing.T) { @@ -164,8 +165,8 @@ func TestMVCCOpLogWriter(t *testing.T) { TxnID: txn2.ID, }), } - if ops := ol.LogicalOps(); !reflect.DeepEqual(exp, ops) { - t.Errorf("expected logical ops %+v, found %+v", exp, ops) + if diff := pretty.Diff(exp, ol.LogicalOps()); diff != nil { + t.Errorf("unexpected logical op differences:\n%s", strings.Join(diff, "\n")) } }) } diff --git a/pkg/storage/engine/pebble.go b/pkg/storage/engine/pebble.go index 371627419529..f76eec1c1055 100644 --- a/pkg/storage/engine/pebble.go +++ b/pkg/storage/engine/pebble.go @@ -528,9 +528,10 @@ func (p *Pebble) GetSSTables() (sstables SSTableInfos) { } type pebbleReadOnly struct { - parent *Pebble - iter pebbleIterator - closed bool + parent *Pebble + prefixIter pebbleIterator + normalIter pebbleIterator + closed bool } var _ ReadWriter = &pebbleReadOnly{} @@ -540,7 +541,8 @@ func (p *pebbleReadOnly) Close() { panic("closing an already-closed pebbleReadOnly") } p.closed = true - p.iter.destroy() + p.prefixIter.destroy() + p.normalIter.destroy() } func (p *pebbleReadOnly) Closed() bool { @@ -580,18 +582,23 @@ func (p *pebbleReadOnly) NewIterator(opts IterOptions) Iterator { return newPebbleIterator(p.parent.db, opts) } - if p.iter.inuse { + iter := &p.normalIter + if opts.Prefix { + iter = &p.prefixIter + } + if iter.inuse { panic("iterator already in use") } - p.iter.inuse = true - p.iter.reusable = true - if p.iter.iter != nil { - p.iter.setOptions(opts) + if iter.iter != nil { + iter.setOptions(opts) } else { - p.iter.init(p.parent.db, opts) + iter.init(p.parent.db, opts) + iter.reusable = true } - return &p.iter + + iter.inuse = true + return iter } // Writer methods are not implemented for pebbleReadOnly. Ideally, the code diff --git a/pkg/storage/engine/pebble_batch.go b/pkg/storage/engine/pebble_batch.go index d3222fb257a7..53387cde22f8 100644 --- a/pkg/storage/engine/pebble_batch.go +++ b/pkg/storage/engine/pebble_batch.go @@ -23,7 +23,8 @@ type pebbleBatch struct { db *pebble.DB batch *pebble.Batch buf []byte - iter pebbleIterator + prefixIter pebbleIterator + normalIter pebbleIterator closed bool isDistinct bool distinctOpen bool @@ -45,9 +46,15 @@ func newPebbleBatch(db *pebble.DB, batch *pebble.Batch) *pebbleBatch { db: db, batch: batch, buf: pb.buf, - iter: pebbleIterator{ - lowerBoundBuf: pb.iter.lowerBoundBuf, - upperBoundBuf: pb.iter.upperBoundBuf, + prefixIter: pebbleIterator{ + lowerBoundBuf: pb.prefixIter.lowerBoundBuf, + upperBoundBuf: pb.prefixIter.upperBoundBuf, + reusable: true, + }, + normalIter: pebbleIterator{ + lowerBoundBuf: pb.normalIter.lowerBoundBuf, + upperBoundBuf: pb.normalIter.upperBoundBuf, + reusable: true, }, } return pb @@ -59,6 +66,11 @@ func (p *pebbleBatch) Close() { panic("closing an already-closed pebbleBatch") } p.closed = true + + // Destroy the iterators before closing the batch. + p.prefixIter.destroy() + p.normalIter.destroy() + if !p.isDistinct { _ = p.batch.Close() p.batch = nil @@ -66,7 +78,7 @@ func (p *pebbleBatch) Close() { p.parentBatch.distinctOpen = false p.isDistinct = false } - p.iter.destroy() + pebbleBatchPool.Put(p) } @@ -157,20 +169,24 @@ func (p *pebbleBatch) NewIterator(opts IterOptions) Iterator { return newPebbleIterator(p.batch, opts) } - if p.iter.inuse { + iter := &p.normalIter + if opts.Prefix { + iter = &p.prefixIter + } + if iter.inuse { panic("iterator already in use") } - p.iter.inuse = true - p.iter.reusable = true - if p.iter.iter != nil { - p.iter.setOptions(opts) + if iter.iter != nil { + iter.setOptions(opts) } else if p.batch.Indexed() { - p.iter.init(p.batch, opts) + iter.init(p.batch, opts) } else { - p.iter.init(p.db, opts) + iter.init(p.db, opts) } - return &p.iter + + iter.inuse = true + return iter } // NewIterator implements the Batch interface. @@ -322,12 +338,10 @@ func (p *pebbleBatch) Distinct() ReadWriter { // optimization. In Pebble we're still using the same underlying batch and if // it is indexed we'll still be indexing it as we Go. p.distinctOpen = true - return &pebbleBatch{ - db: p.db, - batch: p.batch, - parentBatch: p, - isDistinct: true, - } + d := newPebbleBatch(p.db, p.batch) + d.parentBatch = p + d.isDistinct = true + return d } // Empty implements the Batch interface. diff --git a/pkg/storage/engine/pebble_iterator.go b/pkg/storage/engine/pebble_iterator.go index eaa1ffdc7930..398c8dca4a68 100644 --- a/pkg/storage/engine/pebble_iterator.go +++ b/pkg/storage/engine/pebble_iterator.go @@ -71,6 +71,7 @@ func (p *pebbleIterator) init(handle pebble.Reader, opts IterOptions) { lowerBoundBuf: p.lowerBoundBuf, upperBoundBuf: p.upperBoundBuf, prefix: opts.Prefix, + reusable: p.reusable, } if !opts.Prefix && len(opts.UpperBound) == 0 && len(opts.LowerBound) == 0 { @@ -125,6 +126,8 @@ func (p *pebbleIterator) init(handle pebble.Reader, opts IterOptions) { if p.iter == nil { panic("unable to create iterator") } + + p.inuse = true } func (p *pebbleIterator) setOptions(opts IterOptions) { @@ -156,11 +159,12 @@ func (p *pebbleIterator) setOptions(opts IterOptions) { // Close implements the Iterator interface. func (p *pebbleIterator) Close() { + if !p.inuse { + panic("closing idle iterator") + } + p.inuse = false + if p.reusable { - if !p.inuse { - panic("closing idle iterator") - } - p.inuse = false return } @@ -533,6 +537,9 @@ func (p *pebbleIterator) Stats() IteratorStats { } func (p *pebbleIterator) destroy() { + if p.inuse { + panic("iterator still in use") + } if p.iter != nil { err := p.iter.Close() if err != nil {