sql/schemachanger: adopt metadata updater for zone config updates

Previously, the code to update the zone configs was separate
between the declarative schema changer and the legacy schema
changer. This was inadequate because we have a opportunity
to share logic here. To address this the logic to update and
delete zone configurations is now shared.

Release note: None

pkg/ccl/backupccl/restore_job.go

@@ -896,6 +896,7 @@ func createImportingDescriptors(
 								regionConfig,
 								txn,
 								p.ExecCfg(),
+								descsCol,
 							); err != nil {
 								return err
 							}
@@ -1045,6 +1046,7 @@ func createImportingDescriptors(
 							ctx,
 							txn,
 							p.ExecCfg(),
+							descsCol,
 							regionConfig,
 							mutTable,
 							sql.ApplyZoneConfigForMultiRegionTableOptionTableAndIndexes,

pkg/sql/alter_database.go

@@ -530,6 +530,7 @@ func removeLocalityConfigFromAllTablesInDB(
 					ctx,
 					p.txn,
 					p.ExecCfg(),
+					p.Descriptors(),
 					multiregion.RegionConfig{}, // pass dummy config as it is not used.
 					tbDesc,
 					applyZoneConfigForMultiRegionTableOptionRemoveGlobalZoneConfig,
@@ -600,6 +601,7 @@ func (n *alterDatabaseDropRegionNode) startExec(params runParams) error {
 			n.desc.ID,
 			params.p.txn,
 			params.p.execCfg,
+			params.p.Descriptors(),
 		); err != nil {
 			return err
 		}
@@ -756,6 +758,7 @@ func (n *alterDatabasePrimaryRegionNode) switchPrimaryRegion(params runParams) e
 		updatedRegionConfig,
 		params.p.txn,
 		params.p.execCfg,
+		params.p.Descriptors(),
 	); err != nil {
 		return err
 	}
@@ -1129,6 +1132,7 @@ func (n *alterDatabaseSurvivalGoalNode) startExec(params runParams) error {
 		regionConfig,
 		params.p.txn,
 		params.p.execCfg,
+		params.p.Descriptors(),
 	); err != nil {
 		return err
 	}
@@ -1258,6 +1262,7 @@ func (n *alterDatabasePlacementNode) startExec(params runParams) error {
 		regionConfig,
 		params.p.txn,
 		params.p.execCfg,
+		params.p.Descriptors(),
 	); err != nil {
 		return err
 	}

pkg/sql/alter_index.go

@@ -127,6 +127,7 @@ func (n *alterIndexNode) startExec(params runParams) error {
 					params.ctx,
 					params.p.txn,
 					n.tableDesc,
+					params.p.Descriptors(),
 					n.index.GetID(),
 					oldPartitioning,
 					n.index.GetPartitioning(),

pkg/sql/alter_table.go

@@ -698,6 +698,7 @@ func (n *alterTableNode) startExec(params runParams) error {
 					params.ctx,
 					params.p.txn,
 					n.tableDesc,
+					params.p.Descriptors(),
 					n.tableDesc.GetPrimaryIndexID(),
 					oldPartitioning,
 					n.tableDesc.GetPrimaryIndex().GetPartitioning(),

pkg/sql/alter_table_locality.go

@@ -599,6 +599,7 @@ func (n *alterTableSetLocalityNode) writeNewTableLocalityAndZoneConfig(
 		params.ctx,
 		params.p.txn,
 		params.p.ExecCfg(),
+		params.p.Descriptors(),
 		regionConfig,
 		n.tableDesc,
 		ApplyZoneConfigForMultiRegionTableOptionTableAndIndexes,

pkg/sql/backfill.go

@@ -2326,7 +2326,7 @@ func runSchemaChangesInTxn(
 				}
 			} else if idx := m.AsIndex(); idx != nil {
 				if err := indexTruncateInTxn(
-					ctx, planner.Txn(), planner.ExecCfg(), planner.EvalContext(), immutDesc, idx, traceKV,
+					ctx, planner.Txn(), planner.ExecCfg(), planner.Descriptors(), planner.EvalContext(), immutDesc, idx, traceKV,
 				); err != nil {
 					return err
 				}
@@ -2698,6 +2698,7 @@ func indexTruncateInTxn(
 	ctx context.Context,
 	txn *kv.Txn,
 	execCfg *ExecutorConfig,
+	descriptors *descs.Collection,
 	evalCtx *eval.Context,
 	tableDesc catalog.TableDescriptor,
 	idx catalog.Index,
@@ -2724,7 +2725,7 @@ func indexTruncateInTxn(
 		}
 	}
 	// Remove index zone configs.
-	return RemoveIndexZoneConfigs(ctx, txn, execCfg, tableDesc, []uint32{uint32(idx.GetID())})
+	return RemoveIndexZoneConfigs(ctx, txn, execCfg, descriptors, tableDesc, []uint32{uint32(idx.GetID())})
 }
 
 // We note the time at the start of the merge in order to limit the set of

pkg/sql/create_index.go

@@ -913,6 +913,7 @@ func (p *planner) configureZoneConfigForNewIndexPartitioning(
 			ctx,
 			p.txn,
 			p.ExecCfg(),
+			p.Descriptors(),
 			regionConfig,
 			tableDesc,
 			applyZoneConfigForMultiRegionTableOptionNewIndexes(indexIDs...),

pkg/sql/create_table.go

@@ -477,6 +477,7 @@ func (n *createTableNode) startExec(params runParams) error {
 			params.ctx,
 			params.p.txn,
 			params.p.ExecCfg(),
+			params.p.Descriptors(),
 			regionConfig,
 			desc,
 			ApplyZoneConfigForMultiRegionTableOptionTableAndIndexes,

pkg/sql/create_view.go

@@ -333,6 +333,7 @@ func (n *createViewNode) startExec(params runParams) error {
 					params.ctx,
 					params.p.txn,
 					params.p.ExecCfg(),
+					params.p.Descriptors(),
 					regionConfig,
 					newDesc,
 					applyZoneConfigForMultiRegionTableOptionTableNewConfig(

pkg/sql/database_region_change_finalizer.go

@@ -134,7 +134,7 @@ func (r *databaseRegionChangeFinalizer) preDrop(ctx context.Context, txn *kv.Txn
 	}
 	for _, update := range zoneConfigUpdates {
 		if _, err := writeZoneConfigUpdate(
-			ctx, txn, r.localPlanner.ExecCfg(), update,
+			ctx, txn, r.localPlanner.ExecCfg(), r.localPlanner.Descriptors(), update,
 		); err != nil {
 			return err
 		}
@@ -210,6 +210,7 @@ func (r *databaseRegionChangeFinalizer) updateDatabaseZoneConfig(
 		regionConfig,
 		txn,
 		r.localPlanner.ExecCfg(),
+		r.localPlanner.Descriptors(),
 	)
 }
 

pkg/sql/gcjob/index_garbage_collection.go

@@ -79,7 +79,7 @@ func gcIndexes(
 				return err
 			}
 			return sql.RemoveIndexZoneConfigs(
-				ctx, txn, execCfg, freshParentTableDesc, []uint32{uint32(index.IndexID)},
+				ctx, txn, execCfg, descriptors, freshParentTableDesc, []uint32{uint32(index.IndexID)},
 			)
 		}
 		if err := sql.DescsTxn(ctx, execCfg, removeIndexZoneConfigs); err != nil {

pkg/sql/region_util.go

@@ -825,6 +825,7 @@ func ApplyZoneConfigForMultiRegionTable(
 	ctx context.Context,
 	txn *kv.Txn,
 	execCfg *ExecutorConfig,
+	descriptors *descs.Collection,
 	regionConfig multiregion.RegionConfig,
 	table catalog.TableDescriptor,
 	opts ...applyZoneConfigForMultiRegionTableOption,
@@ -833,7 +834,7 @@ func ApplyZoneConfigForMultiRegionTable(
 	if update == nil || err != nil {
 		return err
 	}
-	_, err = writeZoneConfigUpdate(ctx, txn, execCfg, update)
+	_, err = writeZoneConfigUpdate(ctx, txn, execCfg, descriptors, update)
 	return err
 }
 
@@ -845,6 +846,7 @@ func ApplyZoneConfigFromDatabaseRegionConfig(
 	regionConfig multiregion.RegionConfig,
 	txn *kv.Txn,
 	execConfig *ExecutorConfig,
+	descriptors *descs.Collection,
 ) error {
 	// Build a zone config based on the RegionConfig information.
 	dbZoneConfig, err := zoneConfigForMultiRegionDatabase(regionConfig)
@@ -857,13 +859,18 @@ func ApplyZoneConfigFromDatabaseRegionConfig(
 		dbZoneConfig,
 		txn,
 		execConfig,
+		descriptors,
 	)
 }
 
 // discardMultiRegionFieldsForDatabaseZoneConfig resets the multi-region zone
 // config fields for a multi-region database.
 func discardMultiRegionFieldsForDatabaseZoneConfig(
-	ctx context.Context, dbID descpb.ID, txn *kv.Txn, execConfig *ExecutorConfig,
+	ctx context.Context,
+	dbID descpb.ID,
+	txn *kv.Txn,
+	execConfig *ExecutorConfig,
+	descriptors *descs.Collection,
 ) error {
 	// Merge with an empty zone config.
 	return applyZoneConfigForMultiRegionDatabase(
@@ -872,6 +879,7 @@ func discardMultiRegionFieldsForDatabaseZoneConfig(
 		*zonepb.NewZoneConfig(),
 		txn,
 		execConfig,
+		descriptors,
 	)
 }
 
@@ -881,6 +889,7 @@ func applyZoneConfigForMultiRegionDatabase(
 	mergeZoneConfig zonepb.ZoneConfig,
 	txn *kv.Txn,
 	execConfig *ExecutorConfig,
+	descriptors *descs.Collection,
 ) error {
 	currentZoneConfig, err := getZoneConfigRaw(ctx, txn, execConfig.Codec, execConfig.Settings, dbID)
 	if err != nil {
@@ -912,6 +921,7 @@ func applyZoneConfigForMultiRegionDatabase(
 		nil, /* table */
 		&newZoneConfig,
 		execConfig,
+		descriptors,
 		false, /* hasNewSubzones */
 	); err != nil {
 		return err
@@ -951,6 +961,7 @@ func (p *planner) refreshZoneConfigsForTables(
 					ctx,
 					p.txn,
 					p.ExecCfg(),
+					p.Descriptors(),
 					regionConfig,
 					tbDesc,
 					ApplyZoneConfigForMultiRegionTableOptionTableAndIndexes,
@@ -1014,7 +1025,8 @@ func (p *planner) maybeInitializeMultiRegionDatabase(
 		desc.ID,
 		*regionConfig,
 		p.txn,
-		p.execCfg); err != nil {
+		p.execCfg,
+		p.Descriptors()); err != nil {
 		return err
 	}
 
@@ -1099,6 +1111,7 @@ func (p *planner) ResetMultiRegionZoneConfigsForTable(ctx context.Context, id in
 		ctx,
 		p.txn,
 		p.ExecCfg(),
+		p.Descriptors(),
 		regionConfig,
 		desc,
 		ApplyZoneConfigForMultiRegionTableOptionTableAndIndexes,
@@ -1138,6 +1151,7 @@ func (p *planner) ResetMultiRegionZoneConfigsForDatabase(ctx context.Context, id
 		regionConfig,
 		p.txn,
 		p.execCfg,
+		p.Descriptors(),
 	); err != nil {
 		return err
 	}

pkg/sql/schema_changer.go

@@ -1892,6 +1892,7 @@ func maybeUpdateZoneConfigsForPKChange(
 	ctx context.Context,
 	txn *kv.Txn,
 	execCfg *ExecutorConfig,
+	descriptors *descs.Collection,
 	table *tabledesc.Mutable,
 	swapInfo *descpb.PrimaryKeySwap,
 ) error {
@@ -1934,7 +1935,7 @@ func maybeUpdateZoneConfigsForPKChange(
 
 	// Write the zone back. This call regenerates the index spans that apply
 	// to each partition in the index.
-	_, err = writeZoneConfig(ctx, txn, table.ID, table, zone, execCfg, false)
+	_, err = writeZoneConfig(ctx, txn, table.ID, table, zone, execCfg, descriptors, false)
 	if err != nil && !sqlerrors.IsCCLRequiredError(err) {
 		return err
 	}
@@ -3030,6 +3031,7 @@ func (sc *SchemaChanger) applyZoneConfigChangeForMutation(
 				ctx,
 				txn,
 				sc.execCfg,
+				descsCol,
 				regionConfig,
 				tableDesc,
 				opts...,
@@ -3042,7 +3044,7 @@ func (sc *SchemaChanger) applyZoneConfigChangeForMutation(
 		// Note this is done even for isDone = true, though not strictly
 		// necessary.
 		return maybeUpdateZoneConfigsForPKChange(
-			ctx, txn, sc.execCfg, tableDesc, pkSwap.PrimaryKeySwapDesc(),
+			ctx, txn, sc.execCfg, descsCol, tableDesc, pkSwap.PrimaryKeySwapDesc(),
 		)
 	}
 	return nil

pkg/sql/set_zone_config.go

@@ -27,6 +27,8 @@ import (
 	"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/catalog/descs"
+	"github.com/cockroachdb/cockroach/pkg/sql/descmetadata"
 	"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgcode"
 	"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror"
 	"github.com/cockroachdb/cockroach/pkg/sql/privilege"
@@ -790,7 +792,7 @@ func (n *setZoneConfigNode) startExec(params runParams) error {
 		// TODO(ajwerner): This is extremely fragile because we accept a nil table
 		// all the way down here.
 		n.run.numAffected, err = writeZoneConfig(params.ctx, params.p.txn,
-			targetID, table, zoneToWrite, execConfig, hasNewSubzones)
+			targetID, table, zoneToWrite, execConfig, params.p.Descriptors(), hasNewSubzones)
 		if err != nil {
 			return err
 		}
@@ -1054,8 +1056,8 @@ func validateZoneLocalitiesForSecondaryTenants(
 const MultitenancyZoneCfgIssueNo = 49854
 
 type zoneConfigUpdate struct {
-	id    descpb.ID
-	value []byte
+	id         descpb.ID
+	zoneConfig *zonepb.ZoneConfig
 }
 
 func prepareZoneConfigWrites(
@@ -1080,11 +1082,7 @@ func prepareZoneConfigWrites(
 	if zone.IsSubzonePlaceholder() && len(zone.Subzones) == 0 {
 		return &zoneConfigUpdate{id: targetID}, nil
 	}
-	buf, err := protoutil.Marshal(zone)
-	if err != nil {
-		return nil, pgerror.Wrap(err, pgcode.CheckViolation, "could not marshal zone config")
-	}
-	return &zoneConfigUpdate{id: targetID, value: buf}, nil
+	return &zoneConfigUpdate{id: targetID, zoneConfig: zone}, nil
 }
 
 func writeZoneConfig(
@@ -1094,24 +1092,33 @@ func writeZoneConfig(
 	table catalog.TableDescriptor,
 	zone *zonepb.ZoneConfig,
 	execCfg *ExecutorConfig,
+	descriptors *descs.Collection,
 	hasNewSubzones bool,
 ) (numAffected int, err error) {
 	update, err := prepareZoneConfigWrites(ctx, execCfg, targetID, table, zone, hasNewSubzones)
 	if err != nil {
 		return 0, err
 	}
-	return writeZoneConfigUpdate(ctx, txn, execCfg, update)
+	return writeZoneConfigUpdate(ctx, txn, execCfg, descriptors, update)
 }
 
 func writeZoneConfigUpdate(
-	ctx context.Context, txn *kv.Txn, execCfg *ExecutorConfig, update *zoneConfigUpdate,
+	ctx context.Context,
+	txn *kv.Txn,
+	execCfg *ExecutorConfig,
+	descriptors *descs.Collection,
+	update *zoneConfigUpdate,
 ) (numAffected int, _ error) {
-	if update.value == nil {
-		return execCfg.InternalExecutor.Exec(ctx, "delete-zone", txn,
-			"DELETE FROM system.zones WHERE id = $1", update.id)
+	metaDataUpdater := descmetadata.NewMetadataUpdater(ctx,
+		execCfg.InternalExecutorFactory,
+		descriptors,
+		execCfg.SV(),
+		txn,
+		NewFakeSessionData(execCfg.SV()))
+	if update.zoneConfig == nil {
+		return metaDataUpdater.DeleteZoneConfig(ctx, update.id)
 	}
-	return execCfg.InternalExecutor.Exec(ctx, "update-zone", txn,
-		"UPSERT INTO system.zones (id, config) VALUES ($1, $2)", update.id, update.value)
+	return metaDataUpdater.UpsertZoneConfig(ctx, update.id, update.zoneConfig)
 }
 
 // getZoneConfigRaw looks up the zone config with the given ID. Unlike
@@ -1180,6 +1187,7 @@ func RemoveIndexZoneConfigs(
 	ctx context.Context,
 	txn *kv.Txn,
 	execCfg *ExecutorConfig,
+	descriptors *descs.Collection,
 	tableDesc catalog.TableDescriptor,
 	indexIDs []uint32,
 ) error {
@@ -1210,11 +1218,10 @@ func RemoveIndexZoneConfigs(
 
 	if zcRewriteNecessary {
 		// Ignore CCL required error to allow schema change to progress.
-		_, err = writeZoneConfig(ctx, txn, tableDesc.GetID(), tableDesc, zone, execCfg, false /* hasNewSubzones */)
+		_, err = writeZoneConfig(ctx, txn, tableDesc.GetID(), tableDesc, zone, execCfg, descriptors, false /* hasNewSubzones */)
 		if err != nil && !sqlerrors.IsCCLRequiredError(err) {
 			return err
 		}
 	}
-
 	return nil
 }