keys,*: adopt SystemIDChecker

[postamar]

This commit removes many references to keys package constants 49, 50
and 54, and replaces them with functions that take a SystemIDChecker.
Existing functionality should remain unchanged.

This is a prerequisite to making the system descriptor ID space dynamic.

Release note: None

[cockroach-teamcity]

This change is 

[ajwerner]

Reviewed 71 of 71 files at r1, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @postamar)

---

pkg/ccl/backupccl/restore_planning.go, line 953 at r1 (raw file):

	}

	c := keys.DeprecatedSystemIDChecker()

c := p.ExecCfg().SystemIDChecker

---

pkg/ccl/changefeedccl/changefeedbase/validate.go, line 18 at r1 (raw file):

// ValidateTable validates that a table descriptor can be watched by a CHANGEFEED.
func ValidateTable(targets jobspb.ChangefeedTargets, tableDesc catalog.TableDescriptor) error {

nit: plumb in the checker

---

pkg/ccl/importccl/import_table_creation.go, line 46 at r1 (raw file):

Quoted 6 lines of code…

	// We need to choose arbitrary database and table IDs. These aren't important,
	// but they do match what would happen when creating a new database and
	// table on an empty cluster.
	defaultCSVParentID descpb.ID = descpb.ID(catalogkeys.MinNonDefaultUserDescriptorID(keys.DeprecatedSystemIDChecker()))
	defaultCSVTableID  descpb.ID = defaultCSVParentID + 2

Sadness, @adityamaru can you lend some eyes to tell us what we'll need to do to break this assumption?

---

pkg/ccl/workloadccl/format/sstable.go, line 49 at r1 (raw file):

		return nil, errors.Errorf("expected *tree.CreateTable got %T", stmt)
	}
	parentID := descpb.ID(keys.SystemDatabaseID)

Comment that this is because we need something here and it doesn't really matter what. We know that the system database exists and has a constant ID, so we pick that.

---

pkg/kv/kvserver/replica.go, line 980 at r1 (raw file):

	minUserDescID := keys.MinUserDescriptorID(keys.DeprecatedSystemIDChecker())
	return err == nil && roachpb.Key(rem).Compare(keys.SystemSQLCodec.TablePrefix(minUserDescID)) < 0

this is rather sad in that we call it rather frequently IIRC. Can you put a TODO here with my name on it to eliminate this or optimize it? Mentioning #73045 so the cross-reference exists in github.

---

pkg/sql/crdb_internal.go, line 4808 at r1 (raw file):

			return nil
		}
		idChecker := keys.DeprecatedSystemIDChecker()

you have the right one hanging off of the planner.execCfg

---

pkg/sql/catalog/descriptor.go, line 778 at r1 (raw file):

// IsSystemDescriptor returns true iff the descriptor is a system or a reserved
// descriptor.
func IsSystemDescriptor(desc Descriptor) bool {

Should we call this DeprecatedIsSystemDescriptor? TBH, I'm not convinced we need the fallback on the last line, in which case this logic is sound.

---

pkg/sql/catalog/bootstrap/metadata.go, line 129 at r1 (raw file):

	{
		value := roachpb.Value{}
		value.SetInt(int64(keys.MinUserDescriptorID(keys.DeprecatedSystemIDChecker())))

My sense is that this is an area where we shouldn't appeal to the checker but rather should recognize that the bootstrap layer actually does know the exact value to use here. The reason I say that is that by the time we need to invoke this thing, we won't have an implementation of the Checker which has been constructed.

---

pkg/sql/catalog/catalogkeys/keys.go, line 41 at r1 (raw file):

// MinNonDefaultUserDescriptorID returns the smallest possible user-created
// descriptor ID after a cluster is bootstrapped.
func MinNonDefaultUserDescriptorID(idChecker keys.SystemIDChecker) uint32 {

This will eventually need to change. I feel like what we should do is just move the defaultdb and postgres descriptors into the catalog/bootstrap data. The startup migration we've been maintaining to make them is feeling not worth it anymore.

[postamar]

Thanks for the review. What you say all makes sense.

[postamar]

I made some changes following the review comments. I expect to see some CI failures so I'm going to mark this PR as a draft PR for the time being.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner)

---

pkg/ccl/backupccl/restore_planning.go, line 953 at r1 (raw file):

Previously, ajwerner wrote…

c := p.ExecCfg().SystemIDChecker

Done.

---

pkg/ccl/changefeedccl/changefeedbase/validate.go, line 18 at r1 (raw file):

Previously, ajwerner wrote…

nit: plumb in the checker

Won't do. Let's rely on the fact that a descriptor has enough information to tell whether it's a system object or not. See changes tocatalog.IsSystemDescriptor.

---

pkg/ccl/importccl/import_table_creation.go, line 46 at r1 (raw file):

Previously, ajwerner wrote…

	// We need to choose arbitrary database and table IDs. These aren't important,
	// but they do match what would happen when creating a new database and
	// table on an empty cluster.
	defaultCSVParentID descpb.ID = descpb.ID(catalogkeys.MinNonDefaultUserDescriptorID(keys.DeprecatedSystemIDChecker()))
	defaultCSVTableID  descpb.ID = defaultCSVParentID + 2

Sadness, @adityamaru can you lend some eyes to tell us what we'll need to do to break this assumption?

I took a stab at this and I'm happy with the result. We'll see if the CI agrees.

---

pkg/ccl/workloadccl/format/sstable.go, line 49 at r1 (raw file):

Previously, ajwerner wrote…

Comment that this is because we need something here and it doesn't really matter what. We know that the system database exists and has a constant ID, so we pick that.

Done.

---

pkg/kv/kvserver/replica.go, line 980 at r1 (raw file):

Previously, ajwerner wrote…

	minUserDescID := keys.MinUserDescriptorID(keys.DeprecatedSystemIDChecker())
	return err == nil && roachpb.Key(rem).Compare(keys.SystemSQLCodec.TablePrefix(minUserDescID)) < 0

this is rather sad in that we call it rather frequently IIRC. Can you put a TODO here with my name on it to eliminate this or optimize it? Mentioning #73045 so the cross-reference exists in github.

Done. I now precompute the Compare arg.

---

pkg/sql/crdb_internal.go, line 4808 at r1 (raw file):

Previously, ajwerner wrote…

you have the right one hanging off of the planner.execCfg

Done.

---

pkg/sql/catalog/descriptor.go, line 778 at r1 (raw file):

Previously, ajwerner wrote…

Should we call this DeprecatedIsSystemDescriptor? TBH, I'm not convinced we need the fallback on the last line, in which case this logic is sound.

I removed the fallback. Not sure why it's there, and in any case, it doesn't seem like it should. I'll leave this function def be, as it's quite useful. A descriptor should be able to know if it's "system" or not.

---

pkg/sql/catalog/bootstrap/metadata.go, line 129 at r1 (raw file):

Previously, ajwerner wrote…

My sense is that this is an area where we shouldn't appeal to the checker but rather should recognize that the bootstrap layer actually does know the exact value to use here. The reason I say that is that by the time we need to invoke this thing, we won't have an implementation of the Checker which has been constructed.

Done.

---

pkg/sql/catalog/catalogkeys/keys.go, line 41 at r1 (raw file):

Previously, ajwerner wrote…

This will eventually need to change. I feel like what we should do is just move the defaultdb and postgres descriptors into the catalog/bootstrap data. The startup migration we've been maintaining to make them is feeling not worth it anymore.

I agree but this feels out of scope for this change. Did you want me to do anything specific?

[ajwerner]

TestImportPgDumpSchemas seems unhappy, nothing immediately popped out to me as wrong but I could have read more closely. Hopefully the test failures guide you.

Other than that,

Reviewed 37 of 37 files at r2, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @ajwerner and @postamar)

---

pkg/kv/kvserver/replica.go, line 980 at r1 (raw file):

Previously, postamar (Marius Posta) wrote…

Done. I now precompute the Compare arg.

Can you hang this value off of the Store as opposed to the Replica? Replicas are one of the most common objects in the system and each has a store. Trimming fat from that struct will come one day. Seems worth the pointer chase for this.

---

pkg/sql/catalog/catalogkeys/keys.go, line 41 at r1 (raw file):

Previously, postamar (Marius Posta) wrote…

I agree but this feels out of scope for this change. Did you want me to do anything specific?

All good for this change. Filed #73813 to attack the broader problem.

[postamar]

bors r+

[craig]

Build failed (retrying...):

• GitHub CI (Cockroach)

[craig]

This PR was included in a batch that was canceled, it will be automatically retried

[craig]

Build succeeded:

• GitHub CI (Cockroach)