Merge #159204 #159227

craig[bot] · rafiss · craig[bot] · commit ceb320e0695d · 2025-12-11T15:15:16.000Z
159204: jobs: move JobSchedulerEnv helper function r=rafiss a=rafiss Move this simple function to avoid a dependency cycle in an upcoming commit. Epic: None Release note: None 159227: schemachanger: fix EXPLAIN output of storage param operations r=rafiss a=rafiss Since the protobuf was embedded into the scop for mutating storage parameters, it was not being rendered properly in EXPLAIN output. Fix it by making an explicit field instead. informs #155990 Release note: None Co-authored-by: Rafi Shamim <rafi@cockroachlabs.com>
diff --git a/pkg/backup/alter_backup_schedule.go b/pkg/backup/alter_backup_schedule.go
@@ -49,7 +49,7 @@ func loadSchedules(
 	}
 
 	execCfg := p.ExecCfg()
-	env := sql.JobSchedulerEnv(execCfg.JobsKnobs())
+	env := jobs.JobSchedulerEnv(execCfg.JobsKnobs())
 	schedules := jobs.ScheduledJobTxn(p.InternalSQLTxn())
 	schedule, err := schedules.Load(ctx, env, scheduleID)
 	if err != nil {
@@ -434,7 +434,7 @@ func processFullBackupRecurrence(
 		return s, nil
 	}
 
-	env := sql.JobSchedulerEnv(p.ExecCfg().JobsKnobs())
+	env := jobs.JobSchedulerEnv(p.ExecCfg().JobsKnobs())
 	scheduledJobs := jobs.ScheduledJobTxn(p.InternalSQLTxn())
 	if fullBackupAlways {
 		if s.incJob == nil {
@@ -535,7 +535,7 @@ func validateFullIncrementalFrequencies(p sql.PlanHookState, s scheduleDetails)
 	if s.incJob == nil {
 		return nil
 	}
-	env := sql.JobSchedulerEnv(p.ExecCfg().JobsKnobs())
+	env := jobs.JobSchedulerEnv(p.ExecCfg().JobsKnobs())
 	now := env.Now()
 
 	fullFreq, err := frequencyFromCron(now, s.fullJob.ScheduleExpr())
@@ -594,7 +594,7 @@ func processInto(p sql.PlanHookState, spec *alterBackupScheduleSpec, s scheduleD
 	// so we can unpause incrementals. This mirrors the behavior of
 	// CREATE SCHEDULE FOR BACKUP.
 	if !incPaused {
-		env := sql.JobSchedulerEnv(p.ExecCfg().JobsKnobs())
+		env := jobs.JobSchedulerEnv(p.ExecCfg().JobsKnobs())
 		s.fullJob.SetNextRun(env.Now())
 	}
 
@@ -608,7 +608,7 @@ func processNextRunNow(
 		return nil
 	}
 
-	env := sql.JobSchedulerEnv(p.ExecCfg().JobsKnobs())
+	env := jobs.JobSchedulerEnv(p.ExecCfg().JobsKnobs())
 
 	// Trigger the full schedule, unless there is an inc schedule and the user did
 	// not explicitly specify the full.
diff --git a/pkg/backup/create_scheduled_backup.go b/pkg/backup/create_scheduled_backup.go
@@ -180,7 +180,7 @@ func doCreateBackupSchedules(
 		}
 	}
 
-	env := sql.JobSchedulerEnv(p.ExecCfg().JobsKnobs())
+	env := jobs.JobSchedulerEnv(p.ExecCfg().JobsKnobs())
 
 	// Evaluate incremental and full recurrence.
 	incRecurrence, err := schedulebase.ComputeScheduleRecurrence(env.Now(), eval.recurrence)
diff --git a/pkg/backup/restore_job.go b/pkg/backup/restore_job.go
@@ -2922,7 +2922,7 @@ func (r *restoreResumer) dropDescriptors(
 	// immediately.
 	dropTime := int64(1)
 	scheduledJobs := jobs.ScheduledJobTxn(txn)
-	env := sql.JobSchedulerEnv(r.execCfg.JobsKnobs())
+	env := jobs.JobSchedulerEnv(r.execCfg.JobsKnobs())
 	for i := range mutableTables {
 		tableToDrop := mutableTables[i]
 		tablesToGC = append(tablesToGC, tableToDrop.ID)
diff --git a/pkg/ccl/changefeedccl/scheduled_changefeed.go b/pkg/ccl/changefeedccl/scheduled_changefeed.go
@@ -560,7 +560,7 @@ func doCreateChangefeedSchedule(
 	resultsCh chan<- tree.Datums,
 ) error {
 
-	env := sql.JobSchedulerEnv(p.ExecCfg().JobsKnobs())
+	env := jobs.JobSchedulerEnv(p.ExecCfg().JobsKnobs())
 
 	if knobs, ok := p.ExecCfg().DistSQLSrv.TestingKnobs.JobsTestingKnobs.(*jobs.TestingKnobs); ok {
 		if knobs.JobSchedulerEnv != nil {
diff --git a/pkg/jobs/scheduled_job.go b/pkg/jobs/scheduled_job.go
@@ -68,6 +68,14 @@ func NewScheduledJob(env scheduledjobs.JobSchedulerEnv) *ScheduledJob {
 	}
 }
 
+// JobSchedulerEnv returns JobSchedulerEnv.
+func JobSchedulerEnv(knobs *TestingKnobs) scheduledjobs.JobSchedulerEnv {
+	if knobs != nil && knobs.JobSchedulerEnv != nil {
+		return knobs.JobSchedulerEnv
+	}
+	return scheduledjobs.ProdJobSchedulerEnv
+}
+
 // scheduledJobNotFoundError is returned from load when the scheduled job does
 // not exist.
 type scheduledJobNotFoundError struct {
diff --git a/pkg/sql/GEMINI.md b/pkg/sql/GEMINI.md
@@ -0,0 +1,147 @@
+# SQL Package Development Guide
+
+## Schema Changers: Legacy vs Declarative
+
+CockroachDB has two schema change systems: the legacy schema changer and the newer declarative schema changer. Understanding their differences is crucial for DDL development work.
+
+### Architectural Overview
+
+**Legacy Schema Changer** (`/pkg/sql/schema_changer.go`):
+- **Hard-coded state transitions**: Uses fixed sequences of descriptor mutations stored in the `mutations` slice
+- **Imperative approach**: Logic written procedurally with specific code paths for each DDL operation
+- **Limited state model**: Uses states like `DELETE_ONLY`, `WRITE_ONLY`, `BACKFILL_ONLY` from `DescriptorMutation`
+- **Job type**: Uses `SCHEMA_CHANGE` and `TYPEDESC_SCHEMA_CHANGE` job types
+
+**Declarative Schema Changer** (`/pkg/sql/schemachanger/`):
+- **Element-based modeling**: Schema changes modeled as elements (columns, indexes, constraints) with target statuses
+- **Declarative planning**: Uses rules and dependency graphs to generate execution plans
+- **State stored in descriptors**: Uses `declarative_schema_changer_state` field instead of `mutations` slice
+- **Job type**: Uses `NEW_SCHEMA_CHANGE` job type
+
+### State Management Differences
+
+**Legacy Schema Changer:**
+```go
+// Uses DescriptorMutation in mutations slice
+type DescriptorMutation struct {
+    State     DescriptorMutation_State  // DELETE_ONLY, WRITE_ONLY, etc.
+    Direction DescriptorMutation_Direction // ADD, DROP
+    // ... specific column/index descriptors
+}
+```
+
+**Declarative Schema Changer:**
+```proto
+// Uses element model with target statuses
+message Target {
+    ElementProto element_proto = 1;
+    Status target_status = 3;  // PUBLIC, ABSENT, TRANSIENT_ABSENT
+}
+```
+
+### Element Model (Declarative)
+
+Elements are defined in `/pkg/sql/schemachanger/scpb/elements.proto`:
+- **Table-level**: `Table`, `TableComment`, `TableData`
+- **Column elements**: `Column`, `ColumnName`, `ColumnType`, `ColumnDefaultExpression`
+- **Index elements**: `PrimaryIndex`, `SecondaryIndex`, `IndexColumn`
+- **Constraint elements**: `CheckConstraint`, `ForeignKeyConstraint`
+
+Status transitions: `ABSENT` -> `DELETE_ONLY` -> `WRITE_ONLY` -> `BACKFILL_ONLY` -> `VALIDATED` -> `PUBLIC`
+
+### Planning and Execution
+
+**Legacy Schema Changer:**
+- Planning and execution tightly coupled
+- Each DDL statement has custom imperative logic
+- State transitions are hard-coded sequences
+- Difficult to handle complex multi-statement transactions
+
+**Declarative Schema Changer:**
+- Clear separation between planning and execution phases
+- Uses dependency graphs and rules (`/pkg/sql/schemachanger/scplan/internal/rules/`)
+- Can handle complex multi-statement transactions declaratively
+- Better composition of multiple DDL operations
+
+### Error Handling and Rollback
+
+**Legacy Schema Changer:**
+- Known rollback issues, especially with data loss during failed multi-operation schema changes
+- Example: dropping columns alongside adding indexes can permanently lose data on rollback
+
+**Declarative Schema Changer:**
+- Designed for correct rollbacks by reversing target statuses
+- Uses `OnFailOrCancel` method to revert by flipping directions (ADD becomes DROP)
+- Better handling of complex failure scenarios
+
+### Configuration
+
+Enable declarative schema changer via:
+```sql
+-- Session level
+SET use_declarative_schema_changer = 'on';  -- Options: 'off', 'on', 'unsafe', 'unsafe_always'
+
+-- Cluster-wide default
+SET CLUSTER SETTING sql.defaults.use_declarative_schema_changer = 'on';
+```
+
+### Development Patterns
+
+**Adding New DDL Operations:**
+
+*Legacy Schema Changer:*
+- Add case statements in main schema changer loop
+- Implement specific state transition logic
+- Hard-code mutation sequences
+
+*Declarative Schema Changer:*
+- Define new elements in `elements.proto`
+- Add rules for state transitions in `scplan/internal/rules/`
+- Implement operations in `scop/` and execution in `scexec/`
+- More modular and composable approach
+
+### Code Organization
+
+**Legacy Schema Changer:**
+- `/pkg/sql/schema_changer.go` - Main implementation
+- `/pkg/sql/schema_changer_state.go` - State management
+- `/pkg/sql/type_change.go` - Type changes
+
+**Declarative Schema Changer:**
+- `scpb/` - Protocol buffer definitions for elements and state
+- `scbuild/` - Building targets from DDL ASTs
+- `scplan/` - Planning state transitions and dependencies
+- `scexec/` - Executing operations
+- `scop/` - Operation definitions
+- `screl/` - Relational model for elements
+
+### When to Use Each
+
+**Legacy Schema Changer:**
+- Still required for some DDL operations not yet implemented in declarative changer
+- May be faster for simple, single-operation schema changes
+- More predictable performance due to hard-coded paths
+
+**Declarative Schema Changer:**
+- Better for complex multi-statement transactions
+- More overhead in planning phase but better execution for complex scenarios
+- Required for future transactional schema changes
+- Better correctness guarantees and rollback handling
+
+### Migration Path
+
+CockroachDB is gradually migrating from legacy to declarative:
+1. **Coexistence period**: Both schema changers run simultaneously
+2. **Feature-by-feature migration**: DDL operations migrated individually
+3. **Version compatibility**: Old jobs must continue during upgrades
+4. **Extensive testing**: Test coverage in `schemachanger_test.go` and related files
+
+### Key Benefits of Declarative Approach
+
+1. **Correctness**: Better handling of complex scenarios and rollbacks
+2. **Extensibility**: Easier to add new DDL operations
+3. **Composability**: Can handle multiple DDL operations in single transaction
+4. **Testing**: Better separation of concerns allows more targeted testing
+5. **Future-ready**: Foundation for transactional schema changes
+
+The declarative schema changer represents a significant architectural improvement addressing fundamental limitations of the legacy approach, particularly around correctness, extensibility, and complex transaction handling.
diff --git a/pkg/sql/alter_table.go b/pkg/sql/alter_table.go
@@ -2161,7 +2161,7 @@ func handleTTLStorageParamChange(
 
 		// Update cron schedule if required.
 		if before.DeletionCron != after.DeletionCron {
-			env := JobSchedulerEnv(params.ExecCfg().JobsKnobs())
+			env := jobs.JobSchedulerEnv(params.ExecCfg().JobsKnobs())
 			schedules := jobs.ScheduledJobTxn(params.p.InternalSQLTxn())
 			s, err := schedules.Load(
 				params.ctx,
diff --git a/pkg/sql/catalog/externalcatalog/external_catalog.go b/pkg/sql/catalog/externalcatalog/external_catalog.go
@@ -197,7 +197,7 @@ func DropIngestedExternalCatalog(
 	// immediately.
 	dropTime := int64(1)
 	scheduledJobs := jobs.ScheduledJobTxn(txn)
-	env := sql.JobSchedulerEnv(execCfg.JobsKnobs())
+	env := jobs.JobSchedulerEnv(execCfg.JobsKnobs())
 	for i := range mutableTables {
 		tableToDrop := mutableTables[i]
 		tablesToGC = append(tablesToGC, tableToDrop.ID)
diff --git a/pkg/sql/check.go b/pkg/sql/check.go
@@ -736,7 +736,7 @@ func (p *planner) validateTTLScheduledJobInTable(
 	ttl := tableDesc.GetRowLevelTTL()
 
 	execCfg := p.ExecCfg()
-	env := JobSchedulerEnv(execCfg.JobsKnobs())
+	env := jobs.JobSchedulerEnv(execCfg.JobsKnobs())
 
 	wrapError := func(origErr error) error {
 		return errors.WithHintf(
diff --git a/pkg/sql/control_schedules.go b/pkg/sql/control_schedules.go
@@ -43,17 +43,9 @@ func collectTelemetry(command tree.ScheduleCommand) {
 	}
 }
 
-// JobSchedulerEnv returns JobSchedulerEnv.
-func JobSchedulerEnv(knobs *jobs.TestingKnobs) scheduledjobs.JobSchedulerEnv {
-	if knobs != nil && knobs.JobSchedulerEnv != nil {
-		return knobs.JobSchedulerEnv
-	}
-	return scheduledjobs.ProdJobSchedulerEnv
-}
-
 // loadSchedule loads schedule information as the node user.
 func loadSchedule(params runParams, scheduleID tree.Datum) (*jobs.ScheduledJob, error) {
-	env := JobSchedulerEnv(params.ExecCfg().JobsKnobs())
+	env := jobs.JobSchedulerEnv(params.ExecCfg().JobsKnobs())
 	schedule := jobs.NewScheduledJob(env)
 
 	// Load schedule expression.  This is needed for resume command, but we
@@ -89,7 +81,7 @@ func loadSchedule(params runParams, scheduleID tree.Datum) (*jobs.ScheduledJob,
 func DeleteSchedule(
 	ctx context.Context, execCfg *ExecutorConfig, txn isql.Txn, scheduleID jobspb.ScheduleID,
 ) error {
-	env := JobSchedulerEnv(execCfg.JobsKnobs())
+	env := jobs.JobSchedulerEnv(execCfg.JobsKnobs())
 	_, err := txn.ExecEx(
 		ctx,
 		"delete-schedule",
@@ -168,7 +160,7 @@ func (n *controlSchedulesNode) startExec(params runParams) error {
 			if schedule.IsPaused() {
 				err = errors.Newf("cannot execute a paused schedule; use RESUME SCHEDULE instead")
 			} else {
-				env := JobSchedulerEnv(params.ExecCfg().JobsKnobs())
+				env := jobs.JobSchedulerEnv(params.ExecCfg().JobsKnobs())
 				schedule.SetNextRun(env.Now())
 				err = jobs.ScheduledJobTxn(params.p.InternalSQLTxn()).
 					Update(params.ctx, schedule)
diff --git a/pkg/sql/create_table.go b/pkg/sql/create_table.go
@@ -2664,7 +2664,7 @@ func CreateRowLevelTTLScheduledJob(
 	}
 
 	telemetry.Inc(sqltelemetry.RowLevelTTLCreated)
-	env := JobSchedulerEnv(knobs)
+	env := jobs.JobSchedulerEnv(knobs)
 	j, err := newRowLevelTTLScheduledJob(env, owner, tblDesc, clusterID, version)
 	if err != nil {
 		return nil, err
diff --git a/pkg/sql/schema_changer.go b/pkg/sql/schema_changer.go
@@ -1807,7 +1807,7 @@ func (sc *SchemaChanger) done(ctx context.Context) error {
 					if scTable.RowLevelTTL.ScheduleID != 0 {
 						_, err := scheduledJobs.Load(
 							ctx,
-							JobSchedulerEnv(sc.execCfg.JobsKnobs()),
+							jobs.JobSchedulerEnv(sc.execCfg.JobsKnobs()),
 							scTable.RowLevelTTL.ScheduleID,
 						)
 						if err != nil {
@@ -1836,7 +1836,7 @@ func (sc *SchemaChanger) done(ctx context.Context) error {
 				} else if m.Dropped() {
 					if scTable.HasRowLevelTTL() {
 						if err := scheduledJobs.DeleteByID(
-							ctx, JobSchedulerEnv(sc.execCfg.JobsKnobs()),
+							ctx, jobs.JobSchedulerEnv(sc.execCfg.JobsKnobs()),
 							scTable.GetRowLevelTTL().ScheduleID,
 						); err != nil {
 							return err
diff --git a/pkg/sql/schemachanger/scexec/scmutationexec/table.go b/pkg/sql/schemachanger/scexec/scmutationexec/table.go
@@ -35,23 +35,23 @@ func (i *immediateVisitor) SetTableSchemaLocked(
 func (i *immediateVisitor) SetTableStorageParam(
 	ctx context.Context, op scop.SetTableStorageParam,
 ) error {
-	tbl, err := i.checkOutTable(ctx, op.TableID)
+	tbl, err := i.checkOutTable(ctx, op.Param.TableID)
 	if err != nil {
 		return err
 	}
 	setter := tablestorageparam.NewSetter(tbl, false /* isNewObject */)
-	return setter.SetToStringValue(ctx, op.Name, op.Value)
+	return setter.SetToStringValue(ctx, op.Param.Name, op.Param.Value)
 }
 
 func (i *immediateVisitor) ResetTableStorageParam(
 	ctx context.Context, op scop.ResetTableStorageParam,
 ) error {
-	tbl, err := i.checkOutTable(ctx, op.TableID)
+	tbl, err := i.checkOutTable(ctx, op.Param.TableID)
 	if err != nil {
 		return err
 	}
 	setter := tablestorageparam.NewSetter(tbl, false /* isNewObject */)
-	return setter.ResetToZeroValue(ctx, op.Name)
+	return setter.ResetToZeroValue(ctx, op.Param.Name)
 }
 
 func (d *deferredVisitor) UpdateTTLScheduleMetadata(
diff --git a/pkg/sql/schemachanger/scop/immediate_mutation.go b/pkg/sql/schemachanger/scop/immediate_mutation.go
@@ -1263,11 +1263,11 @@ type SetTableSchemaLocked struct {
 // SetTableStorageParam sets a storage parameter on a table.
 type SetTableStorageParam struct {
 	immediateMutationOp
-	scpb.TableStorageParam
+	Param scpb.TableStorageParam
 }
 
 // ResetTableStorageParam resets a storage parameter on a table to its default.
 type ResetTableStorageParam struct {
 	immediateMutationOp
-	scpb.TableStorageParam
+	Param scpb.TableStorageParam
 }
diff --git a/pkg/sql/schemachanger/scpb/elements.proto b/pkg/sql/schemachanger/scpb/elements.proto
@@ -970,7 +970,7 @@ message TableSchemaLocked {
   uint32 table_id = 1 [(gogoproto.customname) = "TableID", (gogoproto.casttype) = "github.com/cockroachdb/cockroach/pkg/sql/sem/catid.DescID"];
 }
 
-// TableStorageParam models storage parameter `schema_locked` of a table.
+// TableStorageParam models the storage parameters of a table other than `schema_locked` and RowLevelTTL-related ones.
 message TableStorageParam {
   uint32 table_id = 1 [(gogoproto.customname) = "TableID", (gogoproto.casttype) = "github.com/cockroachdb/cockroach/pkg/sql/sem/catid.DescID"];
   string name = 2;
diff --git a/pkg/sql/schemachanger/scplan/internal/opgen/opgen_table_storage_params.go b/pkg/sql/schemachanger/scplan/internal/opgen/opgen_table_storage_params.go
@@ -18,7 +18,7 @@ func init() {
 			to(scpb.Status_PUBLIC,
 				emit(func(this *scpb.TableStorageParam) *scop.SetTableStorageParam {
 					return &scop.SetTableStorageParam{
-						TableStorageParam: *protoutil.Clone(this).(*scpb.TableStorageParam),
+						Param: *protoutil.Clone(this).(*scpb.TableStorageParam),
 					}
 				}),
 			),
@@ -28,7 +28,7 @@ func init() {
 			to(scpb.Status_ABSENT,
 				emit(func(this *scpb.TableStorageParam) *scop.ResetTableStorageParam {
 					return &scop.ResetTableStorageParam{
-						TableStorageParam: *protoutil.Clone(this).(*scpb.TableStorageParam),
+						Param: *protoutil.Clone(this).(*scpb.TableStorageParam),
 					}
 				}),
 			),
diff --git a/pkg/sql/schemachanger/scplan/testdata/alter_table_set_storage_param b/pkg/sql/schemachanger/scplan/testdata/alter_table_set_storage_param
diff --git a/pkg/sql/schemachanger/testdata/end_to_end/alter_table_set_storage_param/alter_table_set_storage_param.explain b/pkg/sql/schemachanger/testdata/end_to_end/alter_table_set_storage_param/alter_table_set_storage_param.explain
diff --git a/pkg/sql/schemachanger/testdata/end_to_end/alter_table_set_storage_param/alter_table_set_storage_param__rollback_1_of_1.explain b/pkg/sql/schemachanger/testdata/end_to_end/alter_table_set_storage_param/alter_table_set_storage_param__rollback_1_of_1.explain
diff --git a/pkg/sql/show_create_schedule.go b/pkg/sql/show_create_schedule.go

Original file line number	Diff line number	Diff line change
`@@ -180,7 +180,7 @@ func doCreateBackupSchedules(`
`180`	`180`	`}`
`181`	`181`	`}`
`182`	`182`
`183`		`- env := sql.JobSchedulerEnv(p.ExecCfg().JobsKnobs())`
	`183`	`+ env := jobs.JobSchedulerEnv(p.ExecCfg().JobsKnobs())`
`184`	`184`
`185`	`185`	`// Evaluate incremental and full recurrence.`
`186`	`186`	`incRecurrence, err := schedulebase.ComputeScheduleRecurrence(env.Now(), eval.recurrence)`
Original file line number	Diff line number	Diff line change
`@@ -68,6 +68,14 @@ func NewScheduledJob(env scheduledjobs.JobSchedulerEnv) *ScheduledJob {`
`68`	`68`	`}`
`69`	`69`	`}`
`70`	`70`
	`71`	`+// JobSchedulerEnv returns JobSchedulerEnv.`
	`72`	`+func JobSchedulerEnv(knobs *TestingKnobs) scheduledjobs.JobSchedulerEnv {`
	`73`	`+ if knobs != nil && knobs.JobSchedulerEnv != nil {`
	`74`	`+ return knobs.JobSchedulerEnv`
	`75`	`+ }`
	`76`	`+ return scheduledjobs.ProdJobSchedulerEnv`
	`77`	`+}`
	`78`	`+`
`71`	`79`	`// scheduledJobNotFoundError is returned from load when the scheduled job does`
`72`	`80`	`// not exist.`
`73`	`81`	`type scheduledJobNotFoundError struct {`