From 291077eee999d4a95e17d4f02b13c1484dd07b92 Mon Sep 17 00:00:00 2001 From: Jackson Owens Date: Fri, 5 Jan 2024 15:30:48 -0500 Subject: [PATCH] db: test iterator error while switching to forward direction This commit adds datadriven test cases for an iterator error encountered while switching from backward to forward iteration. It also fixes a case within NextPrefix in which an `invariants` build would panic, improperly assuming iterKey is non-nil (the only case in which it may be nil is if an error occurred). --- data_test.go | 10 +++++ iterator.go | 10 +++++ iterator_histories_test.go | 15 +++++++ testdata/iter_histories/errors | 78 ++++++++++++++++++++++++++++++++++ 4 files changed, 113 insertions(+) diff --git a/data_test.go b/data_test.go index bb338cb6ee..ca772e3197 100644 --- a/data_test.go +++ b/data_test.go @@ -1368,6 +1368,16 @@ func parseDBOptionsArgs(opts *Options, args []datadriven.CmdArg) error { for i := range opts.Levels { opts.Levels[i].BlockSize = v } + case "cache-size": + if opts.Cache != nil { + opts.Cache.Unref() + opts.Cache = nil + } + size, err := strconv.ParseInt(cmdArg.Vals[0], 10, 64) + if err != nil { + return err + } + opts.Cache = NewCache(size) case "index-block-size": v, err := strconv.Atoi(cmdArg.Vals[0]) if err != nil { diff --git a/iterator.go b/iterator.go index 6af262e511..ca2189402d 100644 --- a/iterator.go +++ b/iterator.go @@ -1772,6 +1772,16 @@ func (i *Iterator) nextPrefix() IterValidityState { // key with the user key less than i.key, so we're guaranteed to // land on the correct key with a single Next. i.iterKey, i.iterValue = i.iter.Next() + if i.iterKey == nil { + // This should only be possible if i.iter.Next() encountered an + // error. + if i.iter.Error() == nil { + i.opts.logger.Fatalf("pebble: invariant violation: Nexting internal iterator from iterPosPrev found nothing") + } + // NB: Iterator.Error() will return i.iter.Error(). + i.iterValidityState = IterExhausted + return i.iterValidityState + } if invariants.Enabled && !i.equal(i.iterKey.UserKey, i.key) { i.opts.logger.Fatalf("pebble: invariant violation: Nexting internal iterator from iterPosPrev landed on %q, not %q", i.iterKey.UserKey, i.key) diff --git a/iterator_histories_test.go b/iterator_histories_test.go index 3c6e5dc963..f0061add24 100644 --- a/iterator_histories_test.go +++ b/iterator_histories_test.go @@ -32,6 +32,7 @@ func TestIterHistories(t *testing.T) { } var d *DB + var refedCache bool var buf bytes.Buffer iters := map[string]*Iterator{} batches := map[string]*Batch{} @@ -54,9 +55,13 @@ func TestIterHistories(t *testing.T) { opts.DisableAutomaticCompactions = true opts.EnsureDefaults() opts.WithFSDefaults() + originalCache := opts.Cache if err := parseDBOptionsArgs(opts, td.CmdArgs); err != nil { return nil, err } + // If the test replaced the cache, we'll need to unref the + // new cache later. + refedCache = opts.Cache != originalCache return opts, nil } cleanup := func() (err error) { @@ -85,6 +90,12 @@ func TestIterHistories(t *testing.T) { err = firstError(err, d.Close()) d = nil } + + if refedCache { + opts.Cache.Unref() + opts.Cache = nil + refedCache = false + } return err } defer cleanup() @@ -111,9 +122,13 @@ func TestIterHistories(t *testing.T) { if err := cleanup(); err != nil { return err.Error() } + originalCache := opts.Cache if err := parseDBOptionsArgs(opts, td.CmdArgs); err != nil { return err.Error() } + // If the test replaced the cache, we'll need to unref the + // new cache later. + refedCache = originalCache != opts.Cache d, err = Open("", opts) require.NoError(t, err) return "" diff --git a/testdata/iter_histories/errors b/testdata/iter_histories/errors index 7641856db0..3f57af3094 100644 --- a/testdata/iter_histories/errors +++ b/testdata/iter_histories/errors @@ -144,3 +144,81 @@ prev e: (e9, .) d: (d9, .) err=injected error + +reset +---- + +# Test the case of switching from reverse to forward iteration. During reverse +# iteration, the underlying iterator may be positioned at an internal key with a +# user key less than the current Key() [i.pos = iterPosPrev]. When switching +# back to the forward direction, the iterator must be stepped forward to a user +# key representing the current Key(). +# +# Previously, if this step errored with the invariants tag enabled, a nil +# pointer panic was possible as an invariant assertion assumed that i.iterKey +# became non-nil (based on the knowledge that there must exist internal keys +# that represent the current Key()). This test exercises this case for both +# Next and NextPrefix. +# +# We set the cache-size to 1 so that no unused blocks are held in the cache, and +# we know the Next of the internal iterator will need to read from disk. + +define auto-compactions=off cache-size=1 block-size=1 +L1 + a.SET.1:a1 + b.SET.1:b1 + c.SET.1:c1 + d.SET.1:d1 + e.SET.1:e1 +---- +1: + 000004:[a#1,SET-e#1,SET] + +layout filename=000004.sst +---- + 0 data (22) + 27 data (22) + 54 data (22) + 81 data (22) + 108 data (22) + 135 index (84) + 224 properties (567) + 796 meta-index (33) + 834 footer (53) + 887 EOF + +# NB: Block offset to key contained: +# 0 -> a, 27 -> b, 54 -> c, 81 -> d, 108 -> e +# +# After the last prev in the below cases, the top-level pebble.Iterator is +# positioned at c, but the underlying internal iterator is positioned at b. +# We set up error injection to error when loading the block corresponding to 'c' +# but only on the second tim the block is loaded. + +reopen cache-size=1 auto-compactions=off enable-table-stats=false inject-errors=((ErrInjected (And (PathMatch "000004.sst") (OpFileReadAt 54) (OnIndex 1)))) +---- + +combined-iter +last +prev +prev +next-prefix +---- +e: (e1, .) +d: (d1, .) +c: (c1, .) +err=injected error + +reopen cache-size=1 auto-compactions=off enable-table-stats=false inject-errors=((ErrInjected (And (PathMatch "000004.sst") (OpFileReadAt 54) (OnIndex 1)))) +---- + +combined-iter +last +prev +prev +next +---- +e: (e1, .) +d: (d1, .) +c: (c1, .) +err=injected error