storage: new error interface in MVCCIterator

[erikgrinaker]

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

[blathers-crl]

cc @cockroachdb/replication

[erikgrinaker]

As discussed in #82691 (review), while we're overhauling the interface here anyway, let's consider getting rid of Key() and Value(). It's trivial for callers to clone as needed, e.g. iter.UnsafeKey().Clone() or append(buf[:0], iter.UnsafeKey()...) depending.

[jbowens]

@itsbilal — this might be worth tackling as a part of simplifying CheckSSTConflicts during the 23.2 development cycle. There's been at least one bug from accidentally omitting the Valid() call, and all the boilerplate of those Valid() calls really hamper readability.