Glad to see this! Do we have an issue to track post phase 2 work for tenant zone configs once #49445 is closed? We should create one if not.

I'll rerun the logic tests to see which blacklists can be removed.

Reviewed 1 of 1 files at r1, 4 of 4 files at r2, 5 of 5 files at r3, 44 of 44 files at r4.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @dt, @nvanbenschoten, and @tbg)

pkg/sql/drop_table.go, line 354 at r4 (raw file):

	// this faster mechanism.
	if tableDesc.IsTable() && !tableDesc.IsInterleaved() {
		// TODO BEFORE MERGING: confirm that we can delete this.

I think so. Based on 7abac2f, this code was added as a sanity check. OOC, how would this inability to get a zone config for a dropped tenant table have manifested itself otherwise? Would the default TTL be applied?

pkg/sql/logictest/testdata/logic_test/tenant_unsupported, line 37 at r4 (raw file):

# https://github.com/cockroachdb/cockroach/issues/49318 is fixed.
# statement error operation is unsupported
# ALTER TABLE kv CONFIGURE ZONE USING num_replicas = 123

You can probably delete this.

looks good!

Now that secondary tenant ranges have a zone config to call their own, we can make sense of calls from SQL into the zone configuration infrastructure. We gate off calls that don't make sense for secondary tenants and clean up hooks in SQL that handle zone config manipulation.

This left me slightly confused. Secondary tenants don't really get to do anything with the zone configs, or am I missing something? All they will see is the default zone config (configured at startup). Your description made it sound as if tenants can do... something with the zone config, but I don't think you even let them list the "background" config that applies to them.

Reviewed 1 of 1 files at r1, 4 of 4 files at r2, 5 of 5 files at r3, 44 of 44 files at r4.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @dt and @nvanbenschoten)

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

	// No maximum specified; maximum ID is the last entry in the descriptor
	// table.

or the largest pseudo ID, whichever is larger.

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

	// Maximum specified: need to search the descriptor table. Binary search
	// through all descriptor table values to find the first descriptor with ID
	// >= maxID.

and pick either it or maxPseudoID.

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

		}
		var id uint32
		id, err = keys.TODOSQLCodec.DecodeDescMetadataID(searchSlice[i].Key)

This can't be intentional, at least not without a comment.

pkg/keys/constants.go, line 399 at r4 (raw file):

// PseudoTableIDs is the list of ids from above that are not real tables (i.e.
// there's no table descriptor). They're grouped here because the cluster
// bootstrap process needs to create splits for them; splits for the tables

Can you revisit the "because the bootstrap process needs to create splits for them"? I think I remember that it does create splits for the tables with these IDs (which don't exist). I think something is fishy here, it's not like this package has any notion of where the split points that correspond to these pseudoIDs are. Probably the comment makes no sense.

pkg/sql/zone_config.go, line 159 at r2 (raw file):

func ZoneConfigHook(
	cfg *config.SystemConfig, id uint32,
) (*zonepb.ZoneConfig, *zonepb.ZoneConfig, bool, error) {

While you're here, explain the retvals?

TFTRs!

Do we have an issue to track post phase 2 work for tenant zone configs once #49445 is closed?

I opened #49854 to track this.

This left me slightly confused.

Reworded.

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @asubiotto, @dt, and @tbg)

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

Done.

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

Done.

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

This was a drive-by fix. The code was subtly broken before. We could hit an error while decoding and then overwrite it on the next iteration. I added a test that tests a case that would have previously been handled incorrectly.

pkg/keys/constants.go, line 399 at r4 (raw file):

I'm not sure I understand the ask. We do create splits for these tables, even though they don't really exist. Do you take issue with the keys package mentioning split points?

pkg/sql/drop_table.go, line 354 at r4 (raw file):

Thanks for confirming. It looks like an inability to get a zone config for a dropped tenant table will now manifest as an error being thrown by updateStatusForGCElements from the call to GetZoneConfigForObject. I don't think there's ever a case where a table wouldn't have an associated zone config though, and David seemed to think this was pointless, so I'm going to remove it.

pkg/sql/zone_config.go, line 159 at r2 (raw file):

Done.

pkg/sql/logictest/testdata/logic_test/tenant_unsupported, line 37 at r4 (raw file):

Done.

Reviewable status: complete! 2 of 0 LGTMs obtained (waiting on @asubiotto, @dt, and @tbg)

Reviewed 47 of 47 files at r5, 4 of 4 files at r6, 5 of 5 files at r7, 3 of 3 files at r8, 44 of 44 files at r9.
Reviewable status: complete! 2 of 0 LGTMs obtained (waiting on @asubiotto, @dt, and @tbg)