Merge pull request #93434 from Xiang-Gu/backport22.2-92774

Fixes #91528

pkg/sql/schemachanger/scdeps/exec_deps.go

@@ -419,6 +419,11 @@ func (d *execDeps) User() username.SQLUsername {
 	return d.user
 }
 
+// ClusterSettings implements the scexec.Dependencies interface.
+func (d *execDeps) ClusterSettings() *cluster.Settings {
+	return d.settings
+}
+
 // DescriptorMetadataUpdater implements the scexec.Dependencies interface.
 func (d *execDeps) DescriptorMetadataUpdater(ctx context.Context) scexec.DescriptorMetadataUpdater {
 	return d.metadataUpdater

pkg/sql/schemachanger/scexec/BUILD.bazel

@@ -22,6 +22,7 @@ go_library(
         "//pkg/keys",
         "//pkg/roachpb",
         "//pkg/security/username",
+        "//pkg/settings/cluster",
         "//pkg/sql/catalog",
         "//pkg/sql/catalog/descpb",
         "//pkg/sql/catalog/nstree",

pkg/sql/schemachanger/scexec/dependencies.go

@@ -19,6 +19,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/keys"
 	"github.com/cockroachdb/cockroach/pkg/roachpb"
 	"github.com/cockroachdb/cockroach/pkg/security/username"
+	"github.com/cockroachdb/cockroach/pkg/settings/cluster"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb"
 	"github.com/cockroachdb/cockroach/pkg/sql/schemachanger/scexec/scmutationexec"
@@ -48,6 +49,7 @@ type Dependencies interface {
 	// Statements returns the statements behind this schema change.
 	Statements() []string
 	User() username.SQLUsername
+	ClusterSettings() *cluster.Settings
 }
 
 // Catalog encapsulates the catalog-related dependencies for the executor.

pkg/sql/schemachanger/scplan/internal/scstage/build.go

@@ -37,15 +37,17 @@ func BuildStages(
 	phase scop.Phase,
 	g *scgraph.Graph,
 	scJobIDSupplier func() jobspb.JobID,
+	enforcePlannerSanityChecks bool,
 ) []Stage {
 	c := buildContext{
-		rollback:               init.InRollback,
-		g:                      g,
-		isRevertibilityIgnored: true,
-		targetState:            init.TargetState,
-		startingStatuses:       init.Current,
-		startingPhase:          phase,
-		descIDs:                screl.AllTargetDescIDs(init.TargetState),
+		rollback:                   init.InRollback,
+		g:                          g,
+		isRevertibilityIgnored:     true,
+		targetState:                init.TargetState,
+		startingStatuses:           init.Current,
+		startingPhase:              phase,
+		descIDs:                    screl.AllTargetDescIDs(init.TargetState),
+		enforcePlannerSanityChecks: enforcePlannerSanityChecks,
 	}
 	// Try building stages while ignoring revertibility constraints.
 	// This is fine as long as there are no post-commit stages.
@@ -92,14 +94,15 @@ func BuildStages(
 // buildContext contains the global constants for building the stages.
 // Only the BuildStages function mutates it, it's read-only everywhere else.
 type buildContext struct {
-	rollback               bool
-	g                      *scgraph.Graph
-	scJobID                jobspb.JobID
-	isRevertibilityIgnored bool
-	targetState            scpb.TargetState
-	startingStatuses       []scpb.Status
-	startingPhase          scop.Phase
-	descIDs                catalog.DescriptorIDSet
+	rollback                   bool
+	g                          *scgraph.Graph
+	scJobID                    jobspb.JobID
+	isRevertibilityIgnored     bool
+	targetState                scpb.TargetState
+	startingStatuses           []scpb.Status
+	startingPhase              scop.Phase
+	descIDs                    catalog.DescriptorIDSet
+	enforcePlannerSanityChecks bool
 }
 
 func buildStages(bc buildContext) (stages []Stage) {
@@ -432,10 +435,13 @@ func (sb stageBuilder) hasUnmeetableOutboundDeps(n *screl.Node) (ret bool) {
 	// Mark this node as having been visited in this traversal.
 	sb.visited[n] = sb.visitEpoch
 	// Do some sanity checks.
-	if _, isFulfilling := sb.bs.fulfilled[n]; isFulfilling {
-		// This should never happen.
-		panic(errors.AssertionFailedf("%s should not yet be scheduled for this stage",
-			screl.NodeString(n)))
+	if _, isFulfilled := sb.bs.fulfilled[n]; isFulfilled {
+		if sb.bc.enforcePlannerSanityChecks {
+			panic(errors.AssertionFailedf("%s should not yet be scheduled for this stage",
+				screl.NodeString(n)))
+		} else {
+			return false
+		}
 	}
 	// Look up the current target state for this node, via the lookup table.
 	if t := sb.lut[n.Target]; t == nil {

pkg/sql/schemachanger/scplan/plan.go

@@ -42,6 +42,10 @@ type Params struct {
 	// SchemaChangerJobIDSupplier is used to return the JobID for a
 	// job if one should exist.
 	SchemaChangerJobIDSupplier func() jobspb.JobID
+
+	// enforcePlannerSanityCheck, if true, strictly enforces sanity checks in the
+	// declarative schema changer planner.
+	EnforcePlannerSanityCheck bool
 }
 
 // Exported internal types
@@ -105,9 +109,7 @@ func makePlan(ctx context.Context, p *Plan) (err error) {
 	}
 	{
 		start := timeutil.Now()
-		p.Stages = scstage.BuildStages(
-			ctx, p.CurrentState, p.Params.ExecutionPhase, p.Graph, p.Params.SchemaChangerJobIDSupplier,
-		)
+		p.Stages = scstage.BuildStages(ctx, p.CurrentState, p.Params.ExecutionPhase, p.Graph, p.Params.SchemaChangerJobIDSupplier, p.Params.EnforcePlannerSanityCheck)
 		if log.ExpensiveLogEnabled(ctx, 2) {
 			log.Infof(ctx, "stage generation took %v", timeutil.Since(start))
 		}

pkg/sql/schemachanger/scrun/BUILD.bazel

@@ -13,6 +13,7 @@ go_library(
         "//pkg/jobs",
         "//pkg/jobs/jobspb",
         "//pkg/roachpb",
+        "//pkg/settings",
         "//pkg/settings/cluster",
         "//pkg/sql/catalog",
         "//pkg/sql/catalog/descpb",
@@ -21,6 +22,7 @@ go_library(
         "//pkg/sql/schemachanger/scop",
         "//pkg/sql/schemachanger/scpb",
         "//pkg/sql/schemachanger/scplan",
+        "//pkg/util/buildutil",
         "@com_github_cockroachdb_errors//:errors",
         "@com_github_cockroachdb_redact//:redact",
     ],

pkg/sql/schemachanger/scrun/scrun.go

@@ -17,6 +17,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/jobs"
 	"github.com/cockroachdb/cockroach/pkg/jobs/jobspb"
 	"github.com/cockroachdb/cockroach/pkg/roachpb"
+	"github.com/cockroachdb/cockroach/pkg/settings"
 	"github.com/cockroachdb/cockroach/pkg/settings/cluster"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb"
@@ -25,10 +26,18 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/sql/schemachanger/scop"
 	"github.com/cockroachdb/cockroach/pkg/sql/schemachanger/scpb"
 	"github.com/cockroachdb/cockroach/pkg/sql/schemachanger/scplan"
+	"github.com/cockroachdb/cockroach/pkg/util/buildutil"
 	"github.com/cockroachdb/errors"
 	"github.com/cockroachdb/redact"
 )
 
+var enforcePlannerSanityCheck = settings.RegisterBoolSetting(
+	settings.TenantWritable,
+	"sql.schemachanger.strict_planning_sanity_check.enabled",
+	"enforce strict sanity checks in the declarative schema changer planner",
+	buildutil.CrdbTestBuild,
+)
+
 // RunStatementPhase executes in-transaction schema changes for the targeted
 // state. These are the immediate changes which take place at DDL statement
 // execution time (scop.StatementPhase).
@@ -67,6 +76,7 @@ func runTransactionPhase(
 	sc, err := scplan.MakePlan(ctx, state, scplan.Params{
 		ExecutionPhase:             phase,
 		SchemaChangerJobIDSupplier: deps.TransactionalJobRegistry().SchemaChangerJobID,
+		EnforcePlannerSanityCheck:  enforcePlannerSanityCheck.Get(&deps.ClusterSettings().SV),
 	})
 	if err != nil {
 		return scpb.CurrentState{}, jobspb.InvalidJobID, err