Merge #87931 #88132 #88139

87931: backupccl: tenants should be restored in their correct state r=dt a=adityamaru

Previously, all backed up tenants were unconditionally moved through an `ADD` and then `ACTIVE` state during a cluster/tenant restore. This behaviour appears incorrect. If the tenant was backed up in and adding or dropped state then it should be restored in the same state as well.

This change only moves `ACTIVE` backed up tenants through an `ADD` and then `ACTIVE` state thereby fixing this bug.

Fixes: #87915

Release note (bug fix): Cluster and tenant restores of dropped or adding tenants would incorrectly activate those tenants during restore.

88132: scbuild: add missing name check for ALTER PRIMARY KEY r=postamar a=postamar

Previously, this check was missing, leading the schema change to hang due to a validation failure in a non-revertible post-commit transaction.

This patch maintains the existing behavior of the legacy schema changer which doesn't distinguish between unique and non-unique indexes: all name collisions will be reported as constraint name collisions despite non-unique indexes not being constraints.

Fixes #88131.

Release justification: low-risk high-value bug fix
Release note: None

88139: sql: fix current_setting(..., true) for custom options r=ZhouXing19 a=rafiss

Release note (bug fix): The `current_setting` builtin function now properly does not result in an error when checking a custom session setting that does not exist and the `missing_ok` argument is true.

Co-authored-by: adityamaru <[email protected]>
Co-authored-by: Marius Posta <[email protected]>
Co-authored-by: Rafi Shamim <[email protected]>

pkg/ccl/backupccl/restore_job.go

@@ -1182,8 +1182,18 @@ func createImportingDescriptors(
 					return err
 				}
 				for _, tenant := range details.Tenants {
-					// Mark the tenant info as adding.
-					tenant.State = descpb.TenantInfo_ADD
+					switch tenant.State {
+					case descpb.TenantInfo_ACTIVE:
+						// If the tenant was backed up in an `ACTIVE` state then we create
+						// the restored record in an `ADDING` state and mark it `ACTIVE` at
+						// the end of the restore.
+						tenant.State = descpb.TenantInfo_ADD
+					case descpb.TenantInfo_DROP, descpb.TenantInfo_ADD:
+					// If the tenant was backed up in a `DROP` or `ADD` state then we must
+					// create the restored tenant record in that state as well.
+					default:
+						return errors.AssertionFailedf("unknown tenant state %v", tenant)
+					}
 					if err := sql.CreateTenantRecord(ctx, p.ExecCfg(), txn, &tenant, initialTenantZoneConfig); err != nil {
 						return err
 					}
@@ -2085,8 +2095,19 @@ func (r *restoreResumer) publishDescriptors(
 	}
 
 	for _, tenant := range details.Tenants {
-		if err := sql.ActivateTenant(ctx, r.execCfg, txn, tenant.ID); err != nil {
-			return err
+		switch tenant.State {
+		case descpb.TenantInfo_ACTIVE:
+			// If the tenant was backed up in an `ACTIVE` state then we must activate
+			// the tenant as the final step of the restore. The tenant has already
+			// been created at an earlier stage in the restore in an `ADD` state.
+			if err := sql.ActivateTenant(ctx, r.execCfg, txn, tenant.ID); err != nil {
+				return err
+			}
+		case descpb.TenantInfo_DROP, descpb.TenantInfo_ADD:
+		// If the tenant was backed up in a `DROP` or `ADD` state then we do not
+		// want to activate the tenant.
+		default:
+			return errors.AssertionFailedf("unknown tenant state %v", tenant)
 		}
 	}
 

pkg/ccl/backupccl/targets.go

@@ -38,6 +38,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/sql/sem/tree"
 	"github.com/cockroachdb/cockroach/pkg/util/hlc"
 	"github.com/cockroachdb/cockroach/pkg/util/timeutil"
+	"github.com/cockroachdb/cockroach/pkg/util/tracing"
 	"github.com/cockroachdb/errors"
 )
 
@@ -278,14 +279,18 @@ func fullClusterTargets(
 }
 
 func fullClusterTargetsRestore(
-	allDescs []catalog.Descriptor, lastBackupManifest backuppb.BackupManifest,
+	ctx context.Context, allDescs []catalog.Descriptor, lastBackupManifest backuppb.BackupManifest,
 ) (
 	[]catalog.Descriptor,
 	[]catalog.DatabaseDescriptor,
 	map[tree.TablePattern]catalog.Descriptor,
 	[]descpb.TenantInfoWithUsage,
 	error,
 ) {
+	ctx, span := tracing.ChildSpan(ctx, "backupccl.fullClusterTargetsRestore")
+	_ = ctx // ctx is currently unused, but this new ctx should be used below in the future.
+	defer span.Finish()
+
 	fullClusterDescs, fullClusterDBs, err := fullClusterTargets(allDescs)
 	var filteredDescs []catalog.Descriptor
 	var filteredDBs []catalog.DatabaseDescriptor
@@ -356,10 +361,12 @@ func selectTargets(
 	[]descpb.TenantInfoWithUsage,
 	error,
 ) {
+	ctx, span := tracing.ChildSpan(ctx, "backupccl.selectTargets")
+	defer span.Finish()
 	allDescs, lastBackupManifest := backupinfo.LoadSQLDescsFromBackupsAtTime(backupManifests, asOf)
 
 	if descriptorCoverage == tree.AllDescriptors {
-		return fullClusterTargetsRestore(allDescs, lastBackupManifest)
+		return fullClusterTargetsRestore(ctx, allDescs, lastBackupManifest)
 	}
 
 	if descriptorCoverage == tree.SystemUsers {

pkg/ccl/backupccl/testdata/backup-restore/restore-tenants

@@ -0,0 +1,60 @@
+new-server name=s1
+----
+
+# Create a few tenants.
+exec-sql
+SELECT crdb_internal.create_tenant(5);
+----
+
+exec-sql
+SELECT crdb_internal.create_tenant(6);
+----
+
+# Drop one of them.
+exec-sql
+SELECT crdb_internal.destroy_tenant(5);
+----
+
+query-sql
+SELECT id,active,crdb_internal.pb_to_json('cockroach.sql.sqlbase.TenantInfo', info, true) FROM system.tenants;
+----
+5 false {"id": "5", "state": "DROP"}
+6 true {"id": "6", "state": "ACTIVE"}
+
+exec-sql
+BACKUP INTO 'nodelocal://1/cluster'
+----
+
+exec-sql expect-error-regex=(tenant 5 is not active)
+BACKUP TENANT 5 INTO 'nodelocal://1/tenant5'
+----
+regex matches error
+
+exec-sql
+BACKUP TENANT 6 INTO 'nodelocal://1/tenant6'
+----
+
+new-server name=s2 share-io-dir=s1
+----
+
+exec-sql
+RESTORE FROM LATEST IN 'nodelocal://1/cluster'
+----
+
+# A dropped tenant should be restored as an inactive tenant.
+query-sql
+SELECT id,active,crdb_internal.pb_to_json('cockroach.sql.sqlbase.TenantInfo', info, true) FROM system.tenants;
+----
+5 false {"id": "5", "state": "DROP"}
+6 true {"id": "6", "state": "ACTIVE"}
+
+exec-sql
+RESTORE TENANT 6 FROM LATEST IN 'nodelocal://1/tenant6' WITH tenant = '7';
+----
+
+query-sql
+SELECT id,active,crdb_internal.pb_to_json('cockroach.sql.sqlbase.TenantInfo', info, true) FROM system.tenants;
+----
+5 false {"id": "5", "state": "DROP"}
+6 true {"id": "6", "state": "ACTIVE"}
+7 true {"id": "7", "state": "ACTIVE"}

pkg/sql/logictest/testdata/logic_test/alter_primary_key

@@ -1763,6 +1763,38 @@ SELECT column_name FROM [SHOW COLUMNS FROM t_rowid] ORDER BY column_name;
 k
 v
 
+subtest check_constraint_name
+
+statement ok
+CREATE TABLE t_name_check (a INT NOT NULL, CONSTRAINT ctcheck CHECK (a > 0))
+
+skipif config local-legacy-schema-changer
+statement error pgcode 42710 constraint with name "ctcheck" already exists
+ALTER TABLE t_name_check ADD CONSTRAINT ctcheck PRIMARY KEY (a)
+
+statement ok
+ALTER TABLE t_name_check ADD CONSTRAINT t_name_check_pkey PRIMARY KEY (a)
+
+statement ok
+DROP TABLE t_name_check
+
+statement ok
+CREATE TABLE t_name_check (a INT NOT NULL, CONSTRAINT ctuniq UNIQUE (a))
+
+skipif config local-legacy-schema-changer
+statement error pgcode 42710 constraint with name "ctuniq" already exists
+ALTER TABLE t_name_check ADD CONSTRAINT ctuniq PRIMARY KEY (a)
+
+statement ok
+DROP TABLE t_name_check
+
+statement ok
+CREATE TABLE t_name_check (a INT NOT NULL, INDEX idx (a))
+
+skipif config local-legacy-schema-changer
+statement error pgcode 42710 constraint with name "idx" already exists
+ALTER TABLE t_name_check ADD CONSTRAINT idx PRIMARY KEY (a)
+
 # The following subtest tests the case when the new primary key
 # intersects with the old primary key.
 subtest regression_85877

pkg/sql/logictest/testdata/logic_test/pg_builtins

@@ -268,6 +268,34 @@ SELECT current_setting('statement_timeout'), current_setting('search_path')
 query error unrecognized configuration parameter
 SELECT pg_catalog.current_setting('woo', false)
 
+# Check that current_setting handles custom settings correctly.
+query T
+SELECT current_setting('my.custom', true)
+----
+NULL
+
+statement ok
+PREPARE check_custom AS SELECT current_setting('my.custom', true)
+
+query T
+EXECUTE check_custom
+----
+NULL
+
+statement ok
+BEGIN;
+SET LOCAL my.custom = 'foo'
+
+# Check that the existence of my.custom is checked depending on the execution
+# context, and not at PREPARE time.
+query T
+EXECUTE check_custom
+----
+foo
+
+statement ok
+COMMIT
+
 # check error on unsupported session var.
 query error configuration setting.*not supported
 SELECT current_setting('vacuum_cost_delay', false)

pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table_alter_primary_key.go

@@ -133,6 +133,7 @@ func alterPrimaryKey(b BuildCtx, tn *tree.TableName, tbl *scpb.Table, t alterPri
 		}
 	}
 	out.apply(b.Drop)
+	checkIfConstraintNameAlreadyExists(b, tbl, t)
 	sharding := makeShardedDescriptor(b, t)
 	var sourcePrimaryIndexElem *scpb.PrimaryIndex
 	if rowidToDrop == nil {
@@ -499,6 +500,25 @@ func mustRetrieveKeyIndexColumns(
 	return indexColumns
 }
 
+func checkIfConstraintNameAlreadyExists(b BuildCtx, tbl *scpb.Table, t alterPrimaryKeySpec) {
+	if t.Name == "" {
+		return
+	}
+	// Check explicit constraint names.
+	publicTableElts := b.QueryByID(tbl.TableID).Filter(publicTargetFilter)
+	scpb.ForEachConstraintName(publicTableElts, func(_ scpb.Status, _ scpb.TargetStatus, e *scpb.ConstraintName) {
+		if e.Name == string(t.Name) {
+			panic(pgerror.Newf(pgcode.DuplicateObject, "constraint with name %q already exists", t.Name))
+		}
+	})
+	// Check index names.
+	scpb.ForEachIndexName(publicTableElts, func(_ scpb.Status, _ scpb.TargetStatus, n *scpb.IndexName) {
+		if n.Name == string(t.Name) {
+			panic(pgerror.Newf(pgcode.DuplicateObject, "constraint with name %q already exists", t.Name))
+		}
+	})
+}
+
 // makeShardedDescriptor construct a sharded descriptor for the new primary key.
 // Return nil if the new primary key is not hash-sharded.
 func makeShardedDescriptor(b BuildCtx, t alterPrimaryKeySpec) *catpb.ShardedDescriptor {

pkg/sql/vars.go

@@ -79,6 +79,11 @@ type sessionVar struct {
 	// either by SHOW or in the pg_catalog table.
 	Get func(evalCtx *extendedEvalContext, kv *kv.Txn) (string, error)
 
+	// Exists returns true if this custom session option exists in the current
+	// context. It's needed to support the current_setting builtin for custom
+	// options. It is only defined for custom options.
+	Exists func(evalCtx *extendedEvalContext, kv *kv.Txn) bool
+
 	// GetFromSessionData returns a string representation of a given variable to
 	// be used by BufferParamStatus. This is only required if the variable
 	// is expected to send updates through ParamStatusUpdate in pgwire.
@@ -2607,6 +2612,10 @@ func getCustomOptionSessionVar(varName string) (sv sessionVar, isCustom bool) {
 				}
 				return v, nil
 			},
+			Exists: func(evalCtx *extendedEvalContext, _ *kv.Txn) bool {
+				_, ok := evalCtx.SessionData().CustomOptions[varName]
+				return ok
+			},
 			Set: func(ctx context.Context, m sessionDataMutator, val string) error {
 				// TODO(#72026): do some memory accounting.
 				m.SetCustomOption(varName, val)
@@ -2629,6 +2638,11 @@ func (p *planner) GetSessionVar(
 	if err != nil || !ok {
 		return ok, "", err
 	}
+	if existsFn := v.Exists; existsFn != nil {
+		if missingOk && !existsFn(&p.extendedEvalCtx, p.Txn()) {
+			return false, "", nil
+		}
+	}
 	val, err := v.Get(&p.extendedEvalCtx, p.Txn())
 	return true, val, err
 }