sql: fix mixed-version behaviour of create_tenant()

Previously, running create_tenant on the latest binary in
a mixed-version cluster would bootstrap the system schema using the
logic from the latest binary, in which all system database migration
upgrade steps have been "baked in".

This might not be compatible with the the tenant cluster's version which
is (correctly) set to the active host cluster version, the idea there
being that tenant clusters versions cannot be greater than host cluster
versions.

This commit fixes this by version-gating the tenant cluster system
schema bootstrapping, and using hard-coded values when bootstrapping
into the old version.

Informs cockroachdb#94773.

Release note: None

pkg/ccl/kvccl/kvtenantccl/tenant_upgrade_test.go

@@ -82,6 +82,7 @@ func TestTenantUpgrade(t *testing.T) {
 			cleanupPGUrl()
 		}
 	}
+	expectedInitialTenantVersion, _, _ := v0v1v2()
 	mkTenant := func(t *testing.T, id uint64) (tenantDB *gosql.DB, cleanup func()) {
 		settings := cluster.MakeTestingClusterSettingsWithVersions(
 			clusterversion.TestingBinaryVersion,
@@ -90,7 +91,7 @@ func TestTenantUpgrade(t *testing.T) {
 		)
 		// Initialize the version to the minimum it could be.
 		require.NoError(t, clusterversion.Initialize(ctx,
-			clusterversion.TestingBinaryMinSupportedVersion, &settings.SV))
+			expectedInitialTenantVersion, &settings.SV))
 		tenantArgs := base.TestTenantArgs{
 			TenantID:     roachpb.MakeTenantID(id),
 			TestingKnobs: base.TestingKnobs{},
@@ -109,7 +110,7 @@ func TestTenantUpgrade(t *testing.T) {
 
 		// Ensure that the tenant works.
 		initialTenantRunner.CheckQueryResults(t, "SHOW CLUSTER SETTING version",
-			[][]string{{clusterversion.TestingBinaryMinSupportedVersion.String()}})
+			[][]string{{expectedInitialTenantVersion.String()}})
 		initialTenantRunner.Exec(t, "CREATE TABLE t (i INT PRIMARY KEY)")
 		initialTenantRunner.Exec(t, "INSERT INTO t VALUES (1), (2)")
 
@@ -172,11 +173,9 @@ func TestTenantUpgrade(t *testing.T) {
 
 }
 
-// Returns two versions v0, v1, v2 which correspond to adjacent releases. v0 will
-// equal the TestingBinaryMinSupportedVersion to avoid rot in tests using this
-// (as we retire old versions).
+// Returns two versions v0, v1, v2 which correspond to adjacent releases.
 func v0v1v2() (roachpb.Version, roachpb.Version, roachpb.Version) {
-	v0 := clusterversion.TestingBinaryMinSupportedVersion
+	v0 := clusterversion.ByKey(clusterversion.V22_1)
 	v1 := clusterversion.TestingBinaryVersion
 	v2 := clusterversion.TestingBinaryVersion
 	if v1.Internal > 2 {

pkg/cmd/roachtest/tests/multitenant_upgrade.go

@@ -164,6 +164,7 @@ func runMultiTenantUpgrade(ctx context.Context, t test.Test, c cluster.Cluster,
 	t.Status("verifying that the tenant 13 server works and is at the earlier version")
 
 	verifySQL(t, tenant13.pgURL,
+		mkStmt(`CREATE USER my_user`),
 		mkStmt(`CREATE TABLE foo (id INT PRIMARY KEY, v STRING)`),
 		mkStmt(`INSERT INTO foo VALUES($1, $2)`, 1, "bar"),
 		mkStmt(`SELECT * FROM foo LIMIT 1`).

pkg/sql/catalog/bootstrap/BUILD.bazel

@@ -3,7 +3,10 @@ load("@io_bazel_rules_go//go:def.bzl", "go_library", "go_test")
 
 go_library(
     name = "bootstrap",
-    srcs = ["metadata.go"],
+    srcs = [
+        "metadata.go",
+        "previous_release.go",
+    ],
     importpath = "github.com/cockroachdb/cockroach/pkg/sql/catalog/bootstrap",
     visibility = ["//visibility:public"],
     deps = [

pkg/sql/tenant.go

@@ -247,13 +247,36 @@ func (p *planner) CreateTenant(ctx context.Context, tenID uint64) error {
 	}
 
 	// Initialize the tenant's keyspace.
+	var tenantVersion clusterversion.ClusterVersion
 	codec := keys.MakeSQLCodec(roachpb.MakeTenantID(tenID))
-	schema := bootstrap.MakeMetadataSchema(
-		codec,
-		initialTenantZoneConfig, /* defaultZoneConfig */
-		initialTenantZoneConfig, /* defaultSystemZoneConfig */
-	)
-	kvs, splits := schema.GetInitialValues()
+	var kvs []roachpb.KeyValue
+	var splits []roachpb.RKey
+	if p.EvalContext().Settings.Version.IsActive(ctx, clusterversion.V22_2) {
+		// The cluster is running the latest version.
+		// Use this version to create the tenant and bootstrap it using the host
+		// cluster's bootstrapping logic.
+		tenantVersion.Version = clusterversion.ByKey(clusterversion.V22_2)
+		schema := bootstrap.MakeMetadataSchema(
+			codec,
+			initialTenantZoneConfig, /* defaultZoneConfig */
+			initialTenantZoneConfig, /* defaultSystemZoneConfig */
+		)
+		kvs, splits = schema.GetInitialValues()
+	} else {
+		// The cluster is not running the latest version.
+		// Use the previous major version to create the tenant and bootstrap it
+		// just like the previous major version binary would, using hardcoded
+		// initial values.
+		tenantVersion.Version = clusterversion.ByKey(clusterversion.V22_1)
+		kvs, splits, err = bootstrap.InitialValuesForTenantV221(
+			codec,
+			initialTenantZoneConfig, /* defaultZoneConfig */
+			initialTenantZoneConfig, /* defaultSystemZoneConfig */
+		)
+		if err != nil {
+			return err
+		}
+	}
 
 	{
 		// Populate the version setting for the tenant. This will allow the tenant
@@ -263,8 +286,7 @@ func (p *planner) CreateTenant(ctx context.Context, tenID uint64) error {
 		// using code which may be too new. The expectation is that the tenant
 		// clusters will be updated to a version only after the system tenant has
 		// been upgraded.
-		v := p.EvalContext().Settings.Version.ActiveVersion(ctx)
-		tenantSettingKV, err := generateTenantClusterSettingKV(codec, v)
+		tenantSettingKV, err := generateTenantClusterSettingKV(codec, tenantVersion)
 		if err != nil {
 			return err
 		}

pkg/sql/tenant_test.go

@@ -12,13 +12,18 @@ package sql_test
 
 import (
 	"context"
+	gosql "database/sql"
 	"testing"
 
 	"github.com/cockroachdb/cockroach/pkg/base"
+	"github.com/cockroachdb/cockroach/pkg/clusterversion"
 	"github.com/cockroachdb/cockroach/pkg/keys"
+	"github.com/cockroachdb/cockroach/pkg/kv"
 	"github.com/cockroachdb/cockroach/pkg/roachpb"
+	"github.com/cockroachdb/cockroach/pkg/server"
 	"github.com/cockroachdb/cockroach/pkg/testutils/serverutils"
 	"github.com/cockroachdb/cockroach/pkg/testutils/sqlutils"
+	"github.com/cockroachdb/cockroach/pkg/testutils/testcluster"
 	"github.com/cockroachdb/cockroach/pkg/util/leaktest"
 	"github.com/cockroachdb/cockroach/pkg/util/log"
 	"github.com/stretchr/testify/require"
@@ -29,12 +34,38 @@ import (
 func TestDestroyTenantSynchronous(t *testing.T) {
 	defer leaktest.AfterTest(t)()
 	defer log.Scope(t).Close(t)
-
 	ctx := context.Background()
-	s, sqlDB, kvDB := serverutils.StartServer(t, base.TestServerArgs{})
-	defer s.Stopper().Stop(ctx)
 
+	t.Run("latest version", func(t *testing.T) {
+		s, sqlDB, kvDB := serverutils.StartServer(t, base.TestServerArgs{})
+		defer s.Stopper().Stop(ctx)
+		testDestroyTenantSynchronous(ctx, t, sqlDB, kvDB)
+	})
+
+	t.Run("mixed version", func(t *testing.T) {
+		clusterArgs := base.TestClusterArgs{
+			ServerArgs: base.TestServerArgs{
+				Knobs: base.TestingKnobs{
+					Server: &server.TestingKnobs{
+						DisableAutomaticVersionUpgrade: make(chan struct{}),
+						BinaryVersionOverride:          clusterversion.ByKey(clusterversion.V22_1),
+					},
+				},
+			},
+		}
+		var (
+			tc    = testcluster.StartTestCluster(t, 1, clusterArgs)
+			s     = tc.Server(0)
+			sqlDB = tc.ServerConn(0)
+		)
+		defer tc.Stopper().Stop(ctx)
+		testDestroyTenantSynchronous(ctx, t, sqlDB, s.DB())
+	})
+}
+
+func testDestroyTenantSynchronous(ctx context.Context, t *testing.T, sqlDB *gosql.DB, kvDB *kv.DB) {
 	tenantID := roachpb.MakeTenantID(10)
+
 	codec := keys.MakeSQLCodec(tenantID)
 	const tenantStateQuery = `
 SELECT id, active FROM system.tenants WHERE id = 10

pkg/startupmigrations/BUILD.bazel

@@ -21,6 +21,8 @@ go_library(
         "//pkg/sql",
         "//pkg/sql/catalog/catalogkeys",
         "//pkg/sql/catalog/descpb",
+        "//pkg/sql/pgwire/pgcode",
+        "//pkg/sql/pgwire/pgerror",
         "//pkg/sql/sem/tree",
         "//pkg/sql/sessiondata",
         "//pkg/startupmigrations/leasemanager",

pkg/startupmigrations/migrations.go

@@ -26,6 +26,8 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/sql"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/catalogkeys"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb"
+	"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgcode"
+	"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror"
 	"github.com/cockroachdb/cockroach/pkg/sql/sem/tree"
 	"github.com/cockroachdb/cockroach/pkg/sql/sessiondata"
 	"github.com/cockroachdb/cockroach/pkg/startupmigrations/leasemanager"
@@ -394,7 +396,7 @@ func (r runner) execAsRootWithRetry(
 	// arbitrarily long time.
 	var err error
 	for retry := retry.Start(retry.Options{MaxRetries: 5}); retry.Next(); {
-		err := r.execAsRoot(ctx, opName, stmt, qargs...)
+		err = r.execAsRoot(ctx, opName, stmt, qargs...)
 		if err == nil {
 			break
 		}
@@ -782,15 +784,37 @@ func addRootUser(ctx context.Context, r runner) error {
 	const upsertRootStmt = `
 	        UPSERT INTO system.users (username, "hashedPassword", "isRole", "user_id") VALUES ($1, '', false,  1)
 	        `
-	return r.execAsRootWithRetry(ctx, "addRootUser", upsertRootStmt, username.RootUser)
+	err := r.execAsRootWithRetry(ctx, "addRootUser", upsertRootStmt, username.RootUser)
+	if pgerror.GetPGCode(err) == pgcode.UndefinedColumn {
+		// It's legitimately possible for this UPSERT to fail in tenant clusters
+		// whose system schema was bootstrapped using values from V22.2. In that
+		// schema, the user_id column doesn't exist yet and version gates are
+		// useless at this juncture.
+		const upsertRootStmtV221 = `
+	        UPSERT INTO system.users (username, "hashedPassword", "isRole") VALUES ($1, '', false)
+	        `
+		return r.execAsRootWithRetry(ctx, "addRootUser", upsertRootStmtV221, username.RootUser)
+	}
+	return err
 }
 
 func addAdminRole(ctx context.Context, r runner) error {
 	// Upsert the admin role into the table. We intentionally override any existing entry.
 	const upsertAdminStmt = `
           UPSERT INTO system.users (username, "hashedPassword", "isRole", "user_id") VALUES ($1, '', true,  2)
           `
-	return r.execAsRootWithRetry(ctx, "addAdminRole", upsertAdminStmt, username.AdminRole)
+	err := r.execAsRootWithRetry(ctx, "addAdminRole", upsertAdminStmt, username.AdminRole)
+	if pgerror.GetPGCode(err) == pgcode.UndefinedColumn {
+		// It's legitimately possible for this UPSERT to fail in tenant clusters
+		// whose system schema was bootstrapped using values from V22.2. In that
+		// schema, the user_id column doesn't exist yet and version gates are
+		// useless at this juncture.
+		const upsertAdminStmtV221 = `
+	        UPSERT INTO system.users (username, "hashedPassword", "isRole") VALUES ($1, '', false)
+	        `
+		return r.execAsRootWithRetry(ctx, "addAdminRole", upsertAdminStmtV221, username.AdminRole)
+	}
+	return err
 }
 
 func addRootToAdminRole(ctx context.Context, r runner) error {