sqlbase: ShouldSplitAtIDHook should return false for unknown IDs

[ridwanmsharif]

If descriptor value is not found for a given ID, the hook
should immediately say that it shouldn't be considered for a split.

I think this resolves #31128.

Release note: None

[cockroach-teamcity]

This change is 

[cockroach-teamcity]

This change is 

[petermattis]

Reviewable status: complete! 0 of 0 LGTMs obtained

---

pkg/config/system.go, line 117 at r1 (raw file):

		val.SetBytes(nil)
		return val
	}

I'm not seeing what motivated this change. Likely I'm just missing something obvious.

---

pkg/config/system_test.go, line 384 at r1 (raw file):

	if sqlbase.SplitAtIDHook(invalidDescriptor, cfg) {
		t.Errorf("should not split at ID that is not a descriptor")
	}

Any reason for this test to be here instead of sql/sqlbase?

---

pkg/sql/sqlbase/system.go, line 40 at r1 (raw file):

		// Check if the descriptor ID is not one of the reserved
		// IDs that refer to ranges but not any actual descriptors.
		return id < keys.MinUserDescID

You could do this check before calling cfg.GetDesc, right?

[benesch]

Reviewed 5 of 5 files at r1.
Reviewable status: complete! 0 of 0 LGTMs obtained

---

pkg/config/system.go, line 105 at r1 (raw file):

func (s *SystemConfig) GetDesc(key roachpb.Key, id uint32) *roachpb.Value {
	getVal := s.GetValue(key)
	if getVal != nil {

style suggestion:

if getVal := s.GetValue(key); getVal != nil {
  return getVal
}

---

pkg/config/system.go, line 113 at r1 (raw file):

	if ok {
		// Getting here outside tests is impossible.

I think some more explanation is warranted.

// A test installed a zone config for this ID, but no descriptor. Synthesize an
// empty descriptor to force a split to occur, or else the zone config won't
// apply to any ranges. Most tests that use TestingSetZoneConfig are too
// low-level to create tables and zone configs through the proper channels.
//
// Getting here outside tests is impossible.

---

pkg/config/system.go, line 116 at r1 (raw file):

		val := &roachpb.Value{}
		val.SetBytes(nil)
		return val

Let's use that fancy new function we discovered.

return roachpb.MakeValueFromBytes(nil)

---

pkg/config/system_test.go, line 331 at r1 (raw file):

		{userSQL, tkey(start), roachpb.RKeyMax, tkey(start + 1)},
		{userSQL, tkey(start), tkey(start + 10), tkey(start + 1)},
		{userSQL, tkey(start - 1), tkey(start + 10), tkey(start)},

I think you can add a case here to exercise the new logic:

{userSQL, tkey(start+1), tkey(start+5), nil)

I might be off by one.

---

pkg/config/system_test.go, line 384 at r1 (raw file):

Previously, petermattis (Peter Mattis) wrote…

Any reason for this test to be here instead of sql/sqlbase?

Also, I don't think this test buys you much. I'd remove it in favor of adding a new case to TestComputeSplitKeyTableIDs.

---

pkg/config/testutil.go, line 88 at r1 (raw file):

		return &zone, nil, true, nil
	}
	return nil, nil, true, nil

This seems suspicious.

---

pkg/storage/client_split_test.go, line 1230 at r1 (raw file):

	userTableMax := keys.MinUserDescID + 4
	var exceptions []int

Might as well make this a map[int]struct{} so you don't need to iterate over it.

---

pkg/storage/client_split_test.go, line 1289 at r1 (raw file):

				}
			}
			if shouldAddKey {

if _, ok := exceptions[id]; !ok {
  expKeys = append(expKeys, ...)
}

---

pkg/storage/client_split_test.go, line 1317 at r1 (raw file):

	// Write another, disjoint (+3) descriptor for a user table.
	userTableMax += 3
	exceptions = []int{userTableMax - 1, userTableMax - 2}

exceptions = map[int]struct{}{userTableMax - 1: {}, userTableMax - 2: {}}

[benesch]

Reviewable status: complete! 0 of 0 LGTMs obtained

---

pkg/config/system.go, line 102 at r1 (raw file):

// GetDesc looks for the descriptor value for an ID, if a zone is created in
// a test without creating a Descriptor, a dummy descriptor is returned.

The redundancy in this API is unfortunate. I'm not seeing a great alternative. Though you could derive id from keys.DecodeDescMetadataID, then you have to deal with the fact that that operation can fail. Hmm. Consider adding a line like the following:

// key must be the result of calling sqlbase.MakeDescMetadataKey on id.

---

pkg/sql/sqlbase/system.go, line 40 at r1 (raw file):

Previously, petermattis (Peter Mattis) wrote…

You could do this check before calling cfg.GetDesc, right?

In fact, now that I think about it, it might even be cleaner not to call SplitAtIDHook at all if id < keys.MinUserDescID.

[ridwanmsharif]

Reviewable status: complete! 0 of 0 LGTMs obtained

---

pkg/config/system.go, line 102 at r1 (raw file):

Previously, benesch (Nikhil Benesch) wrote…

The redundancy in this API is unfortunate. I'm not seeing a great alternative. Though you could derive id from keys.DecodeDescMetadataID, then you have to deal with the fact that that operation can fail. Hmm. Consider adding a line like the following:

// key must be the result of calling sqlbase.MakeDescMetadataKey on id.

Using just the key and decoding the id from it now. If an error does come through decoding it, it must mean we can't find a descriptor for it. If I'm wrong, let me know.

---

pkg/config/system.go, line 113 at r1 (raw file):

Previously, benesch (Nikhil Benesch) wrote…

I think some more explanation is warranted.

// A test installed a zone config for this ID, but no descriptor. Synthesize an
// empty descriptor to force a split to occur, or else the zone config won't
// apply to any ranges. Most tests that use TestingSetZoneConfig are too
// low-level to create tables and zone configs through the proper channels.
//
// Getting here outside tests is impossible.

Done.

---

pkg/config/system.go, line 116 at r1 (raw file):

Previously, benesch (Nikhil Benesch) wrote…

Let's use that fancy new function we discovered.

return roachpb.MakeValueFromBytes(nil)

Done.

---

pkg/config/system.go, line 117 at r1 (raw file):

Previously, petermattis (Peter Mattis) wrote…

I'm not seeing what motivated this change. Likely I'm just missing something obvious.

Its to get the tests to behave in a somewhat sane fashion. They try to set zone configs but can't really create the descriptors for them the proper way. This lets them work just the way they are bu directly accessing the testingZoneConfig map and getting a dummy non-nil descriptor for them.

---

pkg/config/system_test.go, line 331 at r1 (raw file):

Previously, benesch (Nikhil Benesch) wrote…

I think you can add a case here to exercise the new logic:

{userSQL, tkey(start+1), tkey(start+5), nil)

I might be off by one.

Done.

---

pkg/config/system_test.go, line 384 at r1 (raw file):

Previously, benesch (Nikhil Benesch) wrote…

Also, I don't think this test buys you much. I'd remove it in favor of adding a new case to TestComputeSplitKeyTableIDs.

Done.

---

pkg/config/testutil.go, line 88 at r1 (raw file):

Previously, benesch (Nikhil Benesch) wrote…

This seems suspicious.

was just residue when I was fixing some tests. Removed now.

---

pkg/sql/sqlbase/system.go, line 40 at r1 (raw file):

Previously, benesch (Nikhil Benesch) wrote…

In fact, now that I think about it, it might even be cleaner not to call SplitAtIDHook at all if id < keys.MinUserDescID.

Done.

---

pkg/storage/client_split_test.go, line 1230 at r1 (raw file):

Previously, benesch (Nikhil Benesch) wrote…

Might as well make this a map[int]struct{} so you don't need to iterate over it.

Done.

---

pkg/storage/client_split_test.go, line 1289 at r1 (raw file):

Previously, benesch (Nikhil Benesch) wrote…

if _, ok := exceptions[id]; !ok {
  expKeys = append(expKeys, ...)
}

Done.

---

pkg/storage/client_split_test.go, line 1317 at r1 (raw file):

Previously, benesch (Nikhil Benesch) wrote…

exceptions = map[int]struct{}{userTableMax - 1: {}, userTableMax - 2: {}}

Done.

[benesch]

Reviewed 5 of 5 files at r2.
Reviewable status: complete! 0 of 0 LGTMs obtained

---

pkg/config/system.go, line 102 at r1 (raw file):

Previously, ridwanmsharif (Ridwan Sharif) wrote…

Using just the key and decoding the id from it now. If an error does come through decoding it, it must mean we can't find a descriptor for it. If I'm wrong, let me know.

It's actually programmer error if DecodeDescMetadataID fails—it means that someone called GetDesc with a key that was not a descriptor key. I'd panic instead.

---

pkg/config/system.go, line 101 at r2 (raw file):

}

// GetDesc looks for the descriptor value for an ID, if a zone is created in

This comment is stale now that the function doesn't take an ID param.

---

pkg/config/system.go, line 111 at r2 (raw file):

	if err != nil {
		// No ID found for key. No roachpb.Value corresponds to this key.
		return nil

As mentioned above:

panic(err)

---

pkg/config/system.go, line 121 at r2 (raw file):

		// A test installed a zone config for this ID, but no descriptor.
		// Synthesize an empty descriptor to force split to occur, or else the
		// one config won't apply to ny ranges. Most tests that use

s/one/zone/ and s/ny/any/

---

pkg/config/system_test.go, line 364 at r2 (raw file):

		{subzoneSQL, tkey(start+5, "e"), tkey(start + 6), nil},

		// Testing that no spits are required for IDs that

s/spits/splits/

---

pkg/sql/sqlbase/system.go, line 38 at r2 (raw file):

	descVal := cfg.GetDesc(MakeDescMetadataKey(ID(id)))
	if descVal == nil {
		return id < keys.MinUserDescID

I think should should just be return false now?

[ridwanmsharif]

Reviewable status: complete! 0 of 0 LGTMs obtained

---

pkg/config/system.go, line 101 at r2 (raw file):

Previously, benesch (Nikhil Benesch) wrote…

This comment is stale now that the function doesn't take an ID param.

Done.

---

pkg/config/system.go, line 111 at r2 (raw file):

Previously, benesch (Nikhil Benesch) wrote…

As mentioned above:

panic(err)

Done.

---

pkg/config/system.go, line 121 at r2 (raw file):

Previously, benesch (Nikhil Benesch) wrote…

s/one/zone/ and s/ny/any/

Done.

---

pkg/config/system_test.go, line 364 at r2 (raw file):

Previously, benesch (Nikhil Benesch) wrote…

s/spits/splits/

Done.

---

pkg/sql/sqlbase/system.go, line 38 at r2 (raw file):

Previously, benesch (Nikhil Benesch) wrote…

I think should should just be return false now?

Done.

[benesch]

nice work!

@petermattis did you want a test that induces a txn retry during a create table, or is the coverage here good enough?

Reviewed 3 of 3 files at r3.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale)

[petermattis]

did you want a test that induces a txn retry during a create table, or is the coverage here good enough?

The coverage here is good enough. I'm feeling fairly confident this was the reason for the extra range being created.

Reviewable status: complete! 1 of 0 LGTMs obtained (and 1 stale)

---

pkg/config/system.go, line 117 at r1 (raw file):

Previously, ridwanmsharif (Ridwan Sharif) wrote…

Its to get the tests to behave in a somewhat sane fashion. They try to set zone configs but can't really create the descriptors for them the proper way. This lets them work just the way they are bu directly accessing the testingZoneConfig map and getting a dummy non-nil descriptor for them.

Ack. The expanded comment helps as well.

---

pkg/config/system.go, line 448 at r3 (raw file):

		// IDs that refer to ranges but not any actual descriptors.
		shouldSplit = true
		if ID >= keys.MinUserDescID {

This is kind of strange. Not all of the IDs less than MinUserDescID need to be split (many of them don't exist). The reason they aren't split is due to how we figure out the largest object ID less than MinUserDescID. I'm just mentioning this in passing as I don't think it is necessary to fix in this PR.

[ridwanmsharif]

bors r+

[ridwanmsharif]

bors r+

[craig]

Build succeeded

• GitHub CI (Cockroach)

[craig]

Build succeeded

• GitHub CI (Cockroach)