keys,storage: add lock table key space, EngineKey, and LockTableKey

[sumeerbhola]

This includes an EngineIterator interface that currently has no
implementation.

The next PR will rename Iterator to MVCCIterator.

Release note: None

[cockroach-teamcity]

This change is 

[petermattis]

The storage parts look good. Love the thorough commenting. I didn't pay much attention to the new keys as I'm not super familiar with the design constraints for the separated lock table. I'll leave reviewing that to @nvanbenschoten.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @nvanbenschoten, @petermattis, and @sumeerbhola)

---

pkg/storage/engine.go, line 156 at r1 (raw file):

	// SeekGE advances the iterator to the first key in the engine which
	// is >= the provided key.
	SeekGE(key EngineKey)

There is value in symmetry with the existing Iterator API, but there is also an opportunity to adjust the API for higher performance. Having this method return (EngineKey, []byte) would avoid subsequent calls to {Key,UnsafeKey} or {Value,UnsafeValue}. Ditto for the other positioning methods (SeekLT, Next, and Prev). I don't particularly mind this API being different, especially if we have a TODO to eventually adjust the Iterator API as well.

---

pkg/storage/engine_key.go, line 38 at r1 (raw file):

type EngineKey struct {
	Key     roachpb.Key
	Version []byte

Do Key and Version always share the same underlying memory? If yes, should we consider storing Version as an unexported length and provide a Version() []byte accessor that dynamically constructs it? The advantage of this approach is shrinking the EngineKey struct from 48 bytes to 32 bytes. That might be a perf win, or this might be a pessimization if we're frequently accessing Version.

---

pkg/storage/engine_key.go, line 159 at r1 (raw file):

}

// DecodeEngineKey decodes the given bytes as an EngineKey.

Should we be noting when these new functions correspond to existing functions? This one is essentially enginepb.SplitMVCCKey. Is the intention to deprecate enginepb.SplitMVCCKey?

---

pkg/storage/engine_key.go, line 206 at r1 (raw file):

// ToEngineKey converts a lock table key to an EngineKey.
func (lk LockTableKey) ToEngineKey() EngineKey {

This performs an allocation. We'll need to be cautious about using this function in performance sensitive code. I wonder if it needs to exist at all.

[nvanbenschoten]

Reviewed 7 of 7 files at r1.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @sumeerbhola)

---

pkg/keys/constants.go, line 172 at r1 (raw file):

	// Different lock strengths may use different value types. The exclusive
	// lock strength uses MVCCMetadata as the value type, since it does
	// double duty as a reference to a provisional MVCC value.

Could you add a TODO to reference the other value type here once it's introduced?

---

pkg/keys/doc.go, line 171 at r1 (raw file):

	// 		`localRangeIDUnreplicatedInfix`.
	// 	  - Range local keys all share `LocalRangePrefix`.
	//    - Range lock (which are also local keys) all share

I think this looks off in reviewable because the previous lines use a tab.

---

pkg/keys/keys_test.go, line 730 at r1 (raw file):

func TestLockTableKeyEncodeDecode(t *testing.T) {
	expectedPrefix := append([]byte(nil), LocalRangeLockTablePrefix...)
	expectedPrefix = append(expectedPrefix, LockTableSingleKeyInfix...)

Is this used?

[nvanbenschoten]

Oh, one thing I meant to mention is that we're doing a lot of work to support bloom filter access for lockTable lookups of point reads. Unfortunately, SQL doesn't actually use point reads in almost any case today. This is being worked on in #46758, so I still think the focus here is appropriate, but it's something to keep in mind.

[sumeerbhola]

TFTRs!

Unfortunately, SQL doesn't actually use point reads in almost any case today. This is being worked on in #46758, so I still think the focus here is appropriate, but it's something to keep in mind.

Ack. I'd looked at https://reviewable.io/reviews/cockroachdb/cockroach/52511 when I was trying to optimize geospatial queries, so hopefully it happens in the near future.

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @nvanbenschoten and @petermattis)

---

pkg/keys/constants.go, line 172 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Could you add a TODO to reference the other value type here once it's introduced?

Done

---

pkg/keys/doc.go, line 171 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

I think this looks off in reviewable because the previous lines use a tab.

Fixed.

---

pkg/keys/keys_test.go, line 730 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Is this used?

Oversight (which lint caught). It is now used.

---

pkg/storage/engine.go, line 156 at r1 (raw file):

Previously, petermattis (Peter Mattis) wrote…

There is value in symmetry with the existing Iterator API, but there is also an opportunity to adjust the API for higher performance. Having this method return (EngineKey, []byte) would avoid subsequent calls to {Key,UnsafeKey} or {Value,UnsafeValue}. Ditto for the other positioning methods (SeekLT, Next, and Prev). I don't particularly mind this API being different, especially if we have a TODO to eventually adjust the Iterator API as well.

I'm assuming this would mean returning an unsafe key/value. Decoding is not zero-cost, and even getting the unsafe value means calling pebble.Iterator.Value (unlike InternalIterator, pebble.Iterator returns a bool from these positioning methods). Maybe the caller doesn't care about the key and value and just whether the iterator is still valid, or just wants the key. Making the right choice here seems tricky, so I've left this as-is. Let me know if you have a strong opinion.

---

pkg/storage/engine_key.go, line 38 at r1 (raw file):

Do Key and Version always share the same underlying memory?

Not sure, yet. I suspect not, because the Key could from a different source like a roachpb.Key of an MVCC value (though not for the LockTableKey case, since there one performs an additional encoding using LockTableSingleKey). How about I leave this for later?

If yes, should we consider storing Version as an unexported length and provide a Version() []byte accessor that dynamically constructs it? The advantage of this approach is shrinking the EngineKey struct from 48 bytes to 32 bytes. That might be a perf win, or this might be a pessimization if we're frequently accessing Version.

My one previous experience with shrinking from 48 to 40 bytes in golang did not seem to help #53446 (comment) but that could be a less frequently copied object. Do you have a good way of deciding when it would help, based on looking at profiles?

---

pkg/storage/engine_key.go, line 159 at r1 (raw file):

Should we be noting when these new functions correspond to existing functions? This one is essentially enginepb.SplitMVCCKey.
Is the intention to deprecate enginepb.SplitMVCCKey?

The intention here was to have a "clean" decoding interface for EngineKey. I am not sure about replacing SplitMVCCKey -- the callers usually discards the second slice, and perhaps that is faster. I don't have a good sense of golang performance -- do you think avoiding that slice assignment is significant? Meanwhile, I've added a TODO.

---

pkg/storage/engine_key.go, line 206 at r1 (raw file):

Previously, petermattis (Peter Mattis) wrote…

This performs an allocation. We'll need to be cautious about using this function in performance sensitive code. I wonder if it needs to exist at all.

Good point. Most of the functions here are place-holders that will get adjusted when adding real code that uses them. This may not need to exist if Puts/Clears to the lock table are an internal implementation detail of adding an intent or clearing an intent, since we currently only have exclusive locks/intents. But eventually we will have other lock strengths too. I've added a TODO for the future to avoid the allocation.

[petermattis]

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @nvanbenschoten and @sumeerbhola)

---

pkg/storage/engine.go, line 151 at r1 (raw file):

	UnsafeValue() []byte
	// Key returns the current key.
	Key() MVCCKey

s/MVCCKey/EngineKey/g

---

pkg/storage/engine.go, line 156 at r1 (raw file):

Previously, sumeerbhola wrote…

I'm assuming this would mean returning an unsafe key/value. Decoding is not zero-cost, and even getting the unsafe value means calling pebble.Iterator.Value (unlike InternalIterator, pebble.Iterator returns a bool from these positioning methods). Maybe the caller doesn't care about the key and value and just whether the iterator is still valid, or just wants the key. Making the right choice here seems tricky, so I've left this as-is. Let me know if you have a strong opinion.

Won't the caller always be calling Key() or UnsafeKey() after a positioning call? When would we make a positioning call and not perform such a check? At the very least, I think we could return (bool, error) and get rid of Valid().

---

pkg/storage/engine_key.go, line 38 at r1 (raw file):

Previously, sumeerbhola wrote…

Do Key and Version always share the same underlying memory?

Not sure, yet. I suspect not, because the Key could from a different source like a roachpb.Key of an MVCC value (though not for the LockTableKey case, since there one performs an additional encoding using LockTableSingleKey). How about I leave this for later?

If yes, should we consider storing Version as an unexported length and provide a Version() []byte accessor that dynamically constructs it? The advantage of this approach is shrinking the EngineKey struct from 48 bytes to 32 bytes. That might be a perf win, or this might be a pessimization if we're frequently accessing Version.

My one previous experience with shrinking from 48 to 40 bytes in golang did not seem to help #53446 (comment) but that could be a less frequently copied object. Do you have a good way of deciding when it would help, based on looking at profiles?

Yes, it is fine to leave this to later.

---

pkg/storage/engine_key.go, line 159 at r1 (raw file):

Previously, sumeerbhola wrote…

Should we be noting when these new functions correspond to existing functions? This one is essentially enginepb.SplitMVCCKey.
Is the intention to deprecate enginepb.SplitMVCCKey?

The intention here was to have a "clean" decoding interface for EngineKey. I am not sure about replacing SplitMVCCKey -- the callers usually discards the second slice, and perhaps that is faster. I don't have a good sense of golang performance -- do you think avoiding that slice assignment is significant? Meanwhile, I've added a TODO.

Ack on not replacing SplitMVCCKey right now. But I do think this comment should mention the other function: This function is similar to "enginepb.SplitMVCCKey" or something like that.

[nvanbenschoten]

The pkg/keys part

Reviewed 5 of 5 files at r2.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @sumeerbhola)

[sumeerbhola]

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @nvanbenschoten, @petermattis, and @sumeerbhola)

---

pkg/storage/engine.go, line 151 at r1 (raw file):

Previously, petermattis (Peter Mattis) wrote…

s/MVCCKey/EngineKey/g

oops. Done.

---

pkg/storage/engine.go, line 156 at r1 (raw file):

Won't the caller always be calling Key() or UnsafeKey() after a positioning call? When would we make a positioning call and not perform such a check?

I don't know of any. But there are cases that don't look at value, like https://github.com/cockroachdb/cockroach/blob/master/pkg/storage/mvcc.go#L2800

At the very least, I think we could return (bool, error) and get rid of Valid().

Good point. Done.
If we had to choose between returning error in-band or returning the (key, value), I would lean towards the former (unlike Pebble's InternalIterator which requires calling the Error() function), since I've been bitten too many times by code that forgot to handle an error and returned an incorrect/incomplete result as truth.

---

pkg/storage/engine_key.go, line 159 at r1 (raw file):

Previously, petermattis (Peter Mattis) wrote…

Ack on not replacing SplitMVCCKey right now. But I do think this comment should mention the other function: This function is similar to "enginepb.SplitMVCCKey" or something like that.

Done.

[petermattis]

Reviewable status: complete! 1 of 0 LGTMs obtained (and 1 stale) (waiting on @nvanbenschoten and @sumeerbhola)

[sumeerbhola]

bors r+

[craig]

Build succeeded:

• GitHub CI (Cockroach)