sql,upgrade: pre-create an application tenant

[knz]

Needed for #84608.
Epic: CRDB-14537

This commit introduces a predefined "app" tenant. This tenant is identified by name: its ID is allocated dynamically to be different from any pre-existing tenant.

Some of the tests here are found to not-work with a predefined app tenant. This will be handled separately in #91122.

[cockroach-teamcity]

This change is 

[knz]

I wonder if it wouldn't make sense to make the app tenant ID for new clusters a metamorphic variable.

[ajwerner]

This seems reasonable

[knz]

thank you

[knz]

I think this is good for (a first round of) review now.

[postamar]

This seems close to me. I don't feel I have the required expertise when it comes to splits so perhaps someone else should check to make sure.

Perhaps also add an assertion in the tenant upgrade step to check that the 'app' tenant has been created?

Reviewed 5 of 6 files at r1, 26 of 27 files at r2, 2 of 2 files at r3, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @irfansharif, @knz, @renatolabs, @rhu713, and @smg260)

---

pkg/server/testing_knobs.go line 50 at r3 (raw file):

	// DisableAppTenantAutoCreation disables the auto-creation of an app tenant
	// in fresh clusters.
	DisableAppTenantAutoCreation bool

Shouldn't this be made incompatible with BinaryVersionOverride somehow? Otherwise you can set this to true and still have the tenant creation upgrade step run.

---

pkg/sql/catalog/bootstrap/metadata.go line 70 at r3 (raw file):

	predefineSecondaryTenant bool,
) MetadataSchema {
	ms := MakeMetadataSchema(keys.SystemSQLCodec, defaultZoneConfig, defaultSystemZoneConfig)

nit: at this point, should MakeInitialKeyspace (and perhaps also MakeMetadataSchema) adopt the ...Option pattern instead?

https://cockroachlabs.atlassian.net/wiki/spaces/CRDB/pages/181371303/Go+Golang+coding+guidelines#Functional-Options

---

pkg/sql/catalog/bootstrap/metadata.go line 641 at r3 (raw file):

	// Ensure that the secondary tenant starts at a split point.
	// ms.otherSplits = append(ms.otherSplits, roachpb.RKey(codec2.TenantPrefix()))

Shouldn't this line be either removed or uncommented? Note that I haven't checked whether that split on the tenant prefix was added or not.

---

pkg/upgrade/upgrades/tenant_table_migration.go line 110 at r3 (raw file):

     AND NOT EXISTS (SELECT 1 FROM system.tenants t WHERE t.name=$1)
   LIMIT 1
) SELECT "".crdb_internal.create_tenant(newid, $1) FROM tid

Wouldn't SELECT "".crdb_internal.create_tenant(max(id)+1, $1) FROM system.tenants HAVING max(name=$1) be equivalent? I'm probably missing something subtle here. In any case I'm struggling to understand this query, FWIW.

---

pkg/upgrade/upgrades/tenant_table_migration.go line 121 at r3 (raw file):

	}

	_, err = d.InternalExecutor.ExecEx(ctx, "add-app-entry", nil,

nit: s,nil,nil /* txn */,

[knz]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @irfansharif, @postamar, @renatolabs, @rhu713, and @smg260)

---

pkg/upgrade/upgrades/tenant_table_migration.go line 110 at r3 (raw file):

Previously, postamar (Marius Posta) wrote…

Wouldn't SELECT "".crdb_internal.create_tenant(max(id)+1, $1) FROM system.tenants HAVING max(name=$1) be equivalent? I'm probably missing something subtle here. In any case I'm struggling to understand this query, FWIW.

The query as written re-uses holes generated by deletion.

In fact I do not understand your HAVING clause :)

[postamar]

The query as written re-uses holes generated by deletion.

Ah, that's what I was missing. Is this desirable, though? Do we expect to exhaust the tenant ID space if we don't re-use them? My naive opinion is that this seems unlikely and re-using IDs is more trouble than it's worth.

In fact I do not understand your HAVING clause :)

Well, my query is incorrect anyway, sorry for that. The max was a bad way to express bool_or. Never mind, I was feeling cleverer than I really was!

edit: if we don't want to recycle IDs, possibly SELECT crdb_internal.create_tenant(max(id)+1, $1) FROM system.tenants HAVING bool_and(name<>$1) might work.

[knz]

subsumed by #98466