Skip to content

storage: new error interface in MVCCIterator #82589

Open
Open
storage: new error interface in MVCCIterator#82589
Labels
A-storageRelating to our storage engine (Pebble) on-disk storage.C-cleanupTech debt, refactors, loose ends, etc. Solution not expected to significantly change behavior.C-performancePerf of queries or internals. Solution not expected to change functional behavior.T-storageStorage Team
@erikgrinaker

Description

@erikgrinaker
erikgrinaker
opened on Jun 8, 2022

Currently, key decoding errors are silently discarded in e.g. pebbleIterator.UnsafeKey(), because the interface is infallible. Also, we need to repeatedly call Valid() both during iteration and also on every call to HasPointAndRange() (which must return false when Valid() return false), which has additional cost. See e.g.

cockroach/pkg/storage/pebble_iterator.go

Lines 322 to 339 in 28e600d

// The MVCCIterator interface is broken in that it silently discards
// the error when UnsafeKey(), Key() are unable to parse the key as
// an MVCCKey. This is especially problematic if the caller is
// accidentally iterating into the lock table key space, since that
// parsing will fail. We do a cheap check here to make sure we are
// not in the lock table key space.
//
// TODO(sumeer): fix this properly by changing those method signatures.
k := p.iter.Key()
if len(k) == 0 {
return false, errors.Errorf("iterator encountered 0 length key")
}
// Last byte is the version length + 1 or 0.
versionLen := int(k[len(k)-1])
if versionLen == engineKeyVersionLockTableLen+1 {
p.mvccDone = true
return false, nil
}

We should improve this by making the interface fallible. For example, this could mean getting rid of Valid(), making all positioning operations return (bool, error), and returning error from decoding functions like Key() and RangeKeys(). We should also consider how to minimize the amount of checks during typical iterations, and measure the effect this has on performance.

The EngineIterator interface already does this, see it for examples. The interface should be discussed with storage before implementation.

Jira issue: CRDB-16523

Metadata

Metadata

Assignees

No one assigned

    Labels

    A-storageRelating to our storage engine (Pebble) on-disk storage.C-cleanupTech debt, refactors, loose ends, etc. Solution not expected to significantly change behavior.C-performancePerf of queries or internals. Solution not expected to change functional behavior.T-storageStorage Team

    Type

    No type

    Projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions