Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions core/deprecatedstate/state.go
Original file line number Diff line number Diff line change
Expand Up @@ -855,7 +855,7 @@ func (s *State) valueAt(key []byte, height uint64) ([]byte, error) {
}

seekKey := binary.BigEndian.AppendUint64(key, height)
for it.Seek(seekKey); it.Valid(); it.Next() {
for it.SeekGE(seekKey); it.Valid(); it.Next() {
seekedKey := it.Key()
// seekedKey size should be `len(key) + sizeof(uint64)` and seekedKey should match key prefix
if len(seekedKey) != len(key)+8 || !bytes.HasPrefix(seekedKey, key) {
Expand Down Expand Up @@ -995,7 +995,7 @@ func (s *State) lastUpdatedBlockNumber(

seekKey := binary.BigEndian.AppendUint64(historyKeyPrefix, upToBlock)

if it.Seek(seekKey) {
if it.SeekGE(seekKey) {
seekedKey := it.Key()
seekedBlock := binary.BigEndian.Uint64(seekedKey[len(historyKeyPrefix):])
if seekedBlock == upToBlock {
Expand Down
4 changes: 2 additions & 2 deletions core/state/state_reader.go
Original file line number Diff line number Diff line change
Expand Up @@ -305,7 +305,7 @@ func (s *StateReader) valueAt(prefix []byte, blockNum uint64, cb func(val []byte

seekKey := binary.BigEndian.AppendUint64(prefix, blockNum)

if !it.Seek(seekKey) || binary.BigEndian.Uint64(it.Key()[len(prefix):]) != blockNum {
if !it.SeekGE(seekKey) || binary.BigEndian.Uint64(it.Key()[len(prefix):]) != blockNum {
if !it.Prev() {
return ErrNoHistoryValue
}
Expand Down Expand Up @@ -334,7 +334,7 @@ func (s *StateReader) lastUpdatedBlockNumber(

seekKey := binary.BigEndian.AppendUint64(historyKeyPrefix, upToBlock)

if it.Seek(seekKey) {
if it.SeekGE(seekKey) {
seekedKey := it.Key()
seekedBlock := binary.BigEndian.Uint64(seekedKey[len(historyKeyPrefix):])
if seekedBlock == upToBlock {
Expand Down
15 changes: 12 additions & 3 deletions db/iterator.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,10 @@ type Iterator interface {
// if the iterator is pointing at a valid entry and false otherwise.
First() bool

// Last moves the iterator to the last key/value pair. Returns true
// if the iterator is pointing at a valid entry and false otherwise.
Last() bool

// Prev moves the iterator to the previous key/value pair. Returns true
// if the iterator is pointing at a valid entry and false otherwise.
Prev() bool
Expand All @@ -33,14 +37,19 @@ type Iterator interface {
// DO NOT USE this unless the value is consumed immediately.
//
// UncopiedValue returns the value at the current position without copying it.
// The returned slice is invalidated by the next call to [Next], [Prev], or [Seek].
// The returned slice is invalidated by the next call to [First], [Last], [Next], [Prev],
// [SeekGE], or [SeekLT].
// Callers must copy the value if it needs to outlive the current iteration.
//
// This is intended for immediate unmarshalling to avoid an extra allocation.
UncopiedValue() ([]byte, error)

// Seek would seek to the provided key if present. If absent, it would seek to the next
// SeekGE would seek to the provided key if present. If absent, it would seek to the next
// key in lexicographical order. Returns true if the iterator is pointing at a valid entry
// and false otherwise.
Seek(key []byte) bool
SeekGE(key []byte) bool

// SeekLT moves the iterator to the last key strictly less than key. Returns true
// if the iterator is pointing at a valid entry and false otherwise.
SeekLT(key []byte) bool
}
2 changes: 1 addition & 1 deletion db/memory/batch.go
Original file line number Diff line number Diff line change
Expand Up @@ -143,7 +143,7 @@ func (b *batch) DeleteRange(start, end []byte) error {
}
defer it.Close()

for ok := it.Seek(start); ok; ok = it.Next() {
for ok := it.SeekGE(start); ok; ok = it.Next() {
if bytes.Compare(it.Key(), end) >= 0 {
break
}
Expand Down
71 changes: 45 additions & 26 deletions db/memory/iterator.go
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
package memory

import (
"bytes"
"errors"
"slices"

Expand All @@ -11,10 +10,12 @@ import (
var _ db.Iterator = (*iterator)(nil)

type iterator struct {
curInd int
keys []string
values [][]byte
closed bool
// curInd is clamped to [-1, len(keys)] so stepping past either end stays exhausted.
curInd int
positioned bool
keys []string
values [][]byte
closed bool
}

func (i *iterator) Valid() bool {
Expand All @@ -30,36 +31,39 @@ func (i *iterator) First() bool {
panic(errIteratorClosed)
}

i.positioned = true
i.curInd = 0
return i.Valid()
}

// Prev on an unpositioned iterator moves to the first key, consistent with pebble.
func (i *iterator) Prev() bool {
if i.closed {
panic(errIteratorClosed)
}

if i.curInd == 0 {
// consistent with pebble's behaviour when Prev() is called on the first key:
// iterator becomes invalid
i.curInd = -1
return false
}

if i.curInd == -1 {
if !i.positioned {
return i.First()
}

i.curInd--
return true
if i.curInd >= 0 {
i.curInd--
}
return i.Valid()
}

func (i *iterator) Next() bool {
if i.closed {
panic(errIteratorClosed)
}

i.curInd++
if !i.positioned {
return i.First()
}

if i.curInd < len(i.keys) {
i.curInd++
}
return i.Valid()
}

Expand Down Expand Up @@ -101,22 +105,37 @@ func (i *iterator) UncopiedValue() ([]byte, error) {
return i.values[i.curInd], nil
}

func (i *iterator) Seek(key []byte) bool {
func (i *iterator) Last() bool {
if i.closed {
panic(errIteratorClosed)
}

for j := range i.keys {
if bytes.Compare(key, []byte(i.keys[j])) <= 0 {
i.curInd = j
return true
}
i.positioned = true
i.curInd = len(i.keys) - 1
return i.Valid()
}

// SeekGE positions one past the end when key is past all entries, so that Prev() gives the
// last valid key, consistent with pebble's behaviour.
func (i *iterator) SeekGE(key []byte) bool {
if i.closed {
panic(errIteratorClosed)
}

i.positioned = true
i.curInd, _ = slices.BinarySearch(i.keys, string(key))
return i.Valid()
}

func (i *iterator) SeekLT(key []byte) bool {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Failed SeekLT + Prev behaves differently from pebble here. Pebble returns false, while memory turns it into First().

Probably, there are other differences in memory and pebble behaviour on such "out-of-range" cases, like SeekGE + Next etc.

There are no places in production where we use similar combinations of moves, but at least worth checking

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, thank you! I will go over it and see the places behavior deviates between DBs and try to make all behave same

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe, we can add something similar to pebble's positioned flag to track this or strictly limit curInd

And different behaviour existed before this PR on other scenarios, SeekLT + Prev is the new one. So maybe we can fix it in other PR or leave it as it is. What do you think?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed. I added a positioned flag as you suggested, and curInd can now only go one step past either end, so moving further keeps the iterator exhausted. That covers the SeekLT + Prev case and an older one where SeekGE past the end + Next + Prev returned true with an empty key. A new iterator still moves to the first key on Next/Prev, same as pebble.

To check there weren't more, I ran every move sequence up to 3 steps long against memory, pebble, pebblev2 and remote (3 keys, 1 key, empty DB). They all match now. I also added a table of these cases to the shared suite in db/testutil.go, so all backends are tested against them.

While checking the rest of the API I found three other ways memory differs from pebble, none related to iterator moves:

  1. Batch DeleteRange deletes the keys that exist when it's called, while pebble applies the range when the batch is written. Keys written to the DB in between survive in memory.
  2. NewIterator(prefix, false) still stops at the prefix in memory, while pebble uses the prefix only as the starting point. No production code passes a prefix with false today.
  3. A prefix made only of 0xFF bytes returns nothing in memory, because its upper bound is nil and memory treats that as an empty bound instead of no bound.

These are older than this PR and unrelated to SeekLT/Last, so I'll fix them in a separate PR.

if i.closed {
panic(errIteratorClosed)
}

// key is past all entries: position one past the end so that Prev() gives the last valid key,
// consistent with pebble's behaviour when SeekGE finds no key within the iterator bounds.
i.curInd = len(i.keys)
return false
i.positioned = true
index, _ := slices.BinarySearch(i.keys, string(key))
i.curInd = index - 1
return i.Valid()
}

func (i *iterator) Close() error {
Expand Down
14 changes: 13 additions & 1 deletion db/pebble/iterator.go
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,12 @@ func (i *iterator) First() bool {
return i.iter.First()
}

func (i *iterator) Last() bool {
defer i.listener.OnIO(false, time.Now())
i.positioned = true
return i.iter.Last()
}

func (i *iterator) Prev() bool {
if !i.positioned {
return i.First()
Expand All @@ -74,12 +80,18 @@ func (i *iterator) Next() bool {
return i.iter.Next()
}

func (i *iterator) Seek(key []byte) bool {
func (i *iterator) SeekGE(key []byte) bool {
defer i.listener.OnIO(false, time.Now())
i.positioned = true
return i.iter.SeekGE(key)
}

func (i *iterator) SeekLT(key []byte) bool {
defer i.listener.OnIO(false, time.Now())
i.positioned = true
return i.iter.SeekLT(key)
}

func (i *iterator) Close() error {
if i.iter == nil {
return pebble.ErrClosed
Expand Down
14 changes: 13 additions & 1 deletion db/pebblev2/iterator.go
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,12 @@ func (i *iterator) First() bool {
return i.iter.First()
}

func (i *iterator) Last() bool {
defer i.listener.OnIO(false, time.Now())
i.positioned = true
return i.iter.Last()
}

func (i *iterator) Prev() bool {
if !i.positioned {
return i.First()
Expand All @@ -74,12 +80,18 @@ func (i *iterator) Next() bool {
return i.iter.Next()
}

func (i *iterator) Seek(key []byte) bool {
func (i *iterator) SeekGE(key []byte) bool {
defer i.listener.OnIO(false, time.Now())
i.positioned = true
return i.iter.SeekGE(key)
}

func (i *iterator) SeekLT(key []byte) bool {
defer i.listener.OnIO(false, time.Now())
i.positioned = true
return i.iter.SeekLT(key)
}

func (i *iterator) Close() error {
if i.iter == nil {
return pebble.ErrClosed
Expand Down
15 changes: 13 additions & 2 deletions db/remote/db_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,7 @@ func TestRemote(t *testing.T) {
require.NoError(t, err)
defer it.Close()

assert.True(t, it.Seek([]byte{1}))
assert.True(t, it.SeekGE([]byte{1}))
assert.Equal(t, it.Key(), []byte{1})
v, err := it.Value()
require.NoError(t, err)
Expand Down Expand Up @@ -171,11 +171,22 @@ func TestRemoteIteratorBounds(t *testing.T) {

// Prev after an out-of-range Seek lands on the last key within the bounds,
// then walks back and stops at the lower bound.
require.False(t, it.Seek([]byte{0x01, 0xFF}))
require.False(t, it.SeekGE([]byte{0x01, 0xFF}))
found = nil
for valid := it.Prev(); valid; valid = it.Prev() {
found = append(found, slices.Clone(it.Key()))
}
assert.Equal(t, [][]byte{{0x01, 0x02}, {0x01, 0x01}, {0x01, 0x00}}, found)
assert.False(t, it.Valid())

require.True(t, it.Last())
assert.Equal(t, []byte{0x01, 0x02}, it.Key())

require.True(t, it.SeekLT([]byte{0x01, 0x01}))
assert.Equal(t, []byte{0x01, 0x00}, it.Key())

require.True(t, it.SeekLT([]byte{0x01, 0xFF}), "SeekLT past the bounds lands on the last key")
assert.Equal(t, []byte{0x01, 0x02}, it.Key())

require.False(t, it.SeekLT([]byte{0x01, 0x00}))
}
34 changes: 19 additions & 15 deletions db/remote/iterator.go
Original file line number Diff line number Diff line change
Expand Up @@ -63,29 +63,33 @@
}

func (i *iterator) First() bool {
if err := i.doOpAndUpdate(gen.Op_FIRST, nil); err != nil {
i.logger.Debug("Error", zap.Stringer("op", gen.Op_FIRST), zap.Error(err))
}
return len(i.currentK) > 0 || len(i.currentV) > 0
return i.move(gen.Op_FIRST, nil)
}

func (i *iterator) Last() bool {
return i.move(gen.Op_LAST, nil)
}

func (i *iterator) Prev() bool {
if err := i.doOpAndUpdate(gen.Op_PREV, nil); err != nil {
i.logger.Debug("Error", zap.Stringer("op", gen.Op_PREV), zap.Error(err))
}
return len(i.currentK) > 0 || len(i.currentV) > 0
return i.move(gen.Op_PREV, nil)
}

func (i *iterator) Next() bool {
if err := i.doOpAndUpdate(gen.Op_NEXT, nil); err != nil {
i.logger.Debug("Error", zap.Stringer("op", gen.Op_NEXT), zap.Error(err))
}
return len(i.currentK) > 0 || len(i.currentV) > 0
return i.move(gen.Op_NEXT, nil)
}

func (i *iterator) SeekGE(key []byte) bool {
return i.move(gen.Op_SEEK_GE, key)
}

func (i *iterator) SeekLT(key []byte) bool {
return i.move(gen.Op_SEEK_LT, key)
}

func (i *iterator) Seek(key []byte) bool {
if err := i.doOpAndUpdate(gen.Op_SEEK, key); err != nil {
i.logger.Debug("Error", zap.Stringer("op", gen.Op_SEEK), zap.Error(err))
// move runs a positioning op and reports whether the iterator landed on an entry.
func (i *iterator) move(op gen.Op, key []byte) bool {
if err := i.doOpAndUpdate(op, key); err != nil {
i.logger.Debug("Error", zap.Stringer("op", op), zap.Error(err))

Check warning on line 92 in db/remote/iterator.go

View check run for this annotation

Codecov / codecov/patch

db/remote/iterator.go#L92

Added line #L92 was not covered by tests
}
return len(i.currentK) > 0 || len(i.currentV) > 0
}
Expand Down
Loading
Loading