storage: introduce indirection between lock table keys and lock strength

[arulajmani]

Lock table keys include a single byte that denotes the lock's strength. Prior to this patch, that byte was directly derived from the lock strength. Versions <= 23.1 considered intents to have Exclusive lock strength; versions > 23.1 treat intents as having a new (stronger) lock strength -- lock.Intent.

This requires us to change how lock table keys are serialized and deserialized -- otherwise, we'd have a incompatibility issue between intents written across these versions. To that end, this patch introduces a layer of indirection between a lock's strength and the strength byte that's persisted in lock table keys. We then ensure the byte itself doesn't change between versions -- i.e, all versions use 3 (the enum value of lock.Exclusive) as the byte written by an intent.

Closes #107947

Release note: None

[cockroach-teamcity]

This change is 

[nvanbenschoten]

Reviewed 3 of 10 files at r1, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @arulajmani and @RaduBerinde)

---

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

// (lock.Strength, StrengthByte), where the StrengthByte corresponds to the byte
// persisted in LockTableKeys.
var replicatedLockStrengthToByteMap = func() [lock.NumLockStrength]byte {

This is close, but the way this usually looks in Go is a little bit more like:

var strToByte = [...]byte{
	lock.Shared:    1,
	lock.Exclusive: 2,
	lock.Intent:    3,
}

var byteToStr = func() (arr [len(strToByte) + 1]lock.Strength) {
	for str, b := range strToByte {
		arr[b] = str
	}
	return arr
}()

func getByteForStr(str int) (byte, error) {
	if str < 0 || str >= len(strToByte) {
		return 0, errors.Error("...")
	}
	b := strToByte[str]
	if b == 0 {
		return 0, errors.Error("...")
	}
	return b, nil
}

func getStrForByte(b byte) (lock.Strength, error) {
	// ... same thing. Maybe try generics.
}

You could even strongly type byte for more type safety. And add a mustGetByteForStr function for testing.

[sumeerbhola]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @arulajmani and @RaduBerinde)

---

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

var byteToReplicatedLockStrengthMap = func() [lock.NumLockStrength]lock.Strength {
	var m [lock.NumLockStrength]lock.Strength
	m[3] = lock.Intent

If both lock.Exclusive and lock.Intent are using the same byte 3, how can one map from byte to lock.Strength?

Are we never persisting lock.Exclusive locks?

[nvanbenschoten]

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

---

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

Previously, sumeerbhola wrote…

If both lock.Exclusive and lock.Intent are using the same byte 3, how can one map from byte to lock.Strength?

Are we never persisting lock.Exclusive locks?

The intention is for lock.Exclusive to map to/from byte 2. This means that all prior encoded keys will now be decoded as lock.Intent. Thus far, we have never persisted lock.Exclusive locks.

[nvanbenschoten]

This PR is going to need to change a bit with #109082.

I also realized while typing that refactor out that we probably don't want to change the LockTableKey.Strength field itself. Instead, we can push the translation directly into LockTableKey.ToEngineKey and EngineKey.ToLockTableKey. I suspect that will make this change much smaller by hiding the translation behind the LockTableKey abstraction.

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

[arulajmani]

Instead, we can push the translation directly into LockTableKey.ToEngineKey and EngineKey.ToLockTableKey.

Ack, I'll make the change. Haven't looked at the other patch yet, but do you want me to build this on top of yours?

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

[arulajmani]

Rebased this + took your suggestion to push the translation into LockTableKey.ToEngineKey and EngineKey.ToLockTableKey. RFAL.

After coming back to this and implementing your suggestion, I'm wondering how you feel about renaming LockTableKey to ReplicatedLock? It helps make it clear that the Strength field has nothing to do with the key encoding.

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

---

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

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

The intention is for lock.Exclusive to map to/from byte 2. This means that all prior encoded keys will now be decoded as lock.Intent. Thus far, we have never persisted lock.Exclusive locks.

I added some words about this in the commit message to clarify.

---

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

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

This is close, but the way this usually looks in Go is a little bit more like:

var strToByte = [...]byte{
	lock.Shared:    1,
	lock.Exclusive: 2,
	lock.Intent:    3,
}

var byteToStr = func() (arr [len(strToByte) + 1]lock.Strength) {
	for str, b := range strToByte {
		arr[b] = str
	}
	return arr
}()

func getByteForStr(str int) (byte, error) {
	if str < 0 || str >= len(strToByte) {
		return 0, errors.Error("...")
	}
	b := strToByte[str]
	if b == 0 {
		return 0, errors.Error("...")
	}
	return b, nil
}

func getStrForByte(b byte) (lock.Strength, error) {
	// ... same thing. Maybe try generics.
}

You could even strongly type byte for more type safety. And add a mustGetByteForStr function for testing.

Thanks for sketching this out! I took ~most of your suggestions. I didn't add lock.Shared or lock.Exclusive to the map just yet.

Now that we're no longer storing StrengthByte in the LockTableKey struct, I didn't feel strongly typing byte would get us much. Happy to iterate over this.

[nvanbenschoten]

After coming back to this and implementing your suggestion, I'm wondering how you feel about renaming LockTableKey to ReplicatedLock? It helps make it clear that the Strength field has nothing to do with the key encoding.

Discussed on slack and decided to keep this as LockTableKey.

Reviewed 12 of 12 files at r2, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @arulajmani, @RaduBerinde, and @sumeerbhola)

---

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

I didn't feel strongly typing byte would get us much. Happy to iterate over this.

Agreed. I don't think it's needed now.

---

pkg/storage/engine_key.go line 258 at r2 (raw file):

}

// byteToReplicatedLockStrength is a mapping between the strength byte persisted

small nit: move this below replicatedLockStrengthToByte, since this is derived from that array.

---

pkg/storage/engine_key.go line 261 at r2 (raw file):

[len(replicatedLockStrengthToByte)]

I thought this had to be len(replicatedLockStrengthToByte)+1, but what you have here also works. It's actually a bit subtle why though, having to do with lock.Intent's value of 4. We should probably just make this a slice and be more explicit. Something like:

var byteToReplicatedLockStrength = func() (arr []lock.Strength) {
    maxByte := 0
    for _, b := range replicatedLockStrengthToByte {
        if b > maxBytes {
            b = maxByte
        }
    }
    arr = make([]lock.Strength, maxByte+1)
    for str, b := range replicatedLockStrengthToByte {
        if b != 0 {
            arr[b] = lock.Strength(str)
        }
    }
    return arr
}()

---

pkg/storage/engine_key.go line 278 at r2 (raw file):

// a lock's key encoding, given its lock strength.
func getByteForReplicatedLockStrength(str lock.Strength) byte {
	if str <= lock.None || int(str) >= len(replicatedLockStrengthToByte) {

nit: s/str <= lock.None/str < 0/, since the intention is to use str as an index into the array.

---

pkg/storage/engine_key.go line 291 at r2 (raw file):

// the strength byte from its key encoding.
func getReplicatedLockStrengthForByte(b byte) (lock.Strength, error) {
	if b <= 0 || int(b) >= len(byteToReplicatedLockStrength) {

s/<=/</

---

pkg/storage/engine_key.go line 295 at r2 (raw file):

	}
	str := byteToReplicatedLockStrength[b]
	if str <= lock.None || str > lock.Intent {

Should this just be if str == 0 {?

[arulajmani]

RFAL

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

---

pkg/storage/engine_key.go line 261 at r2 (raw file):

It's actually a bit subtle why though, having to do with lock.Intent's value of 4.

I agree. I also like this bit:

 if b != 0 { arr[b] = lock.Strength(str) {

it's much clearer than the old thing.

---

pkg/storage/engine_key.go line 291 at r2 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

s/<=/</

We can get rid of this entirely, because a byte can never be < 0.

---

pkg/storage/engine_key.go line 295 at r2 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Should this just be if str == 0 {?

Yeah, str == 0 makes more sense.

[nvanbenschoten]

Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @RaduBerinde and @sumeerbhola)

[arulajmani]

TFTR!

bors r=nvanbenschoten

[craig]

Build succeeded:

• Bazel Essential CI (Cockroach)