Skip to content

Commit

Permalink
sql: validate primary / secondary region localities at end of txn
Browse files Browse the repository at this point in the history
Previously, if a database was restored with skip_localities,
there was no way to modify this database to set the primary
region since validation would kick in too early during the
statement. This meant fixing the regions in a restored database
was impossible if the primary region was no longer valid. To
address this, this patch, delays locality validation till the
end of the transaction.

Fixes: #103290

Release note (bug fix): SET PRIMARY REGION and SET SECONDARY REGION
did not validate transactionally, which could prevent cleaning up
removed regions after a restore.
  • Loading branch information
fqazi committed May 16, 2023
1 parent a65bca2 commit 3db9d6b
Show file tree
Hide file tree
Showing 6 changed files with 80 additions and 9 deletions.
14 changes: 14 additions & 0 deletions pkg/ccl/backupccl/testdata/backup-restore/multiregion
Original file line number Diff line number Diff line change
Expand Up @@ -69,10 +69,24 @@ exec-sql
RESTORE FROM LATEST IN 'nodelocal://1/full_cluster_backup/' WITH skip_localities_check;
----

exec-sql
ALTER DATABASE d SET PRIMARY REGION 'eu-central-1';
ALTER DATABASE d DROP REGION 'us-east-1';
ALTER DATABASE d DROP REGION 'us-west-1';
ALTER DATABASE d ADD REGION 'eu-north-1';
----

exec-sql
RESTORE DATABASE d FROM LATEST IN 'nodelocal://1/database_backup/' WITH skip_localities_check, new_db_name='d_new';
----

exec-sql
ALTER DATABASE d_new SET PRIMARY REGION 'eu-central-1';
ALTER DATABASE d_new DROP REGION 'us-east-1';
ALTER DATABASE d_new DROP REGION 'us-west-1';
ALTER DATABASE d_new ADD REGION 'eu-north-1';
----

exec-sql
DROP DATABASE d_new;
----
Expand Down
8 changes: 7 additions & 1 deletion pkg/sql/alter_database.go
Original file line number Diff line number Diff line change
Expand Up @@ -897,14 +897,18 @@ func (n *alterDatabasePrimaryRegionNode) switchPrimaryRegion(params runParams) e
return err
}

// Validate the final zone config at the end of the transaction, since
// we will not be validating localities right now.
*params.extendedEvalCtx.validateDbZoneConfig = true

// Update the database's zone configuration.
if err := ApplyZoneConfigFromDatabaseRegionConfig(
params.ctx,
n.desc.ID,
updatedRegionConfig,
params.p.InternalSQLTxn(),
params.p.execCfg,
true, /* validateLocalities */
false, /*validateLocalities*/
params.extendedEvalCtx.Tracing.KVTracingEnabled(),
); err != nil {
return err
Expand Down Expand Up @@ -2021,6 +2025,8 @@ func (n *alterDatabaseSecondaryRegion) startExec(params runParams) error {
return err
}

*params.extendedEvalCtx.validateDbZoneConfig = true

// Update the database's zone configuration.
if err := ApplyZoneConfigFromDatabaseRegionConfig(
params.ctx,
Expand Down
12 changes: 12 additions & 0 deletions pkg/sql/catalog/descs/collection.go
Original file line number Diff line number Diff line change
Expand Up @@ -702,6 +702,18 @@ func (tc *Collection) GetUncommittedTables() (tables []catalog.TableDescriptor)
return tables
}

// GetUncommittedDatabases returns all the databases updated or created in the
// transaction.
func (tc *Collection) GetUncommittedDatabases() (databases []catalog.DatabaseDescriptor) {
_ = tc.uncommitted.iterateUncommittedByID(func(desc catalog.Descriptor) error {
if database, ok := desc.(catalog.DatabaseDescriptor); ok {
databases = append(databases, database)
}
return nil
})
return databases
}

func newMutableSyntheticDescriptorAssertionError(id descpb.ID) error {
return errors.AssertionFailedf("attempted mutable access of synthetic descriptor %d", id)
}
Expand Down
21 changes: 13 additions & 8 deletions pkg/sql/conn_executor.go
Original file line number Diff line number Diff line change
Expand Up @@ -1357,6 +1357,9 @@ type connExecutor struct {
// comprising statements.
numRows int

// validateDbZoneConfig should the DB zone config on commit.
validateDbZoneConfig bool

// txnCounter keeps track of how many SQL txns have been open since
// the start of the session. This is used for logging, to
// distinguish statements that belong to separate SQL transactions.
Expand Down Expand Up @@ -1914,6 +1917,7 @@ func (ex *connExecutor) resetExtraTxnState(ctx context.Context, ev txnEvent) {
} else {
ex.extraTxnState.descCollection.ReleaseAll(ctx)
ex.extraTxnState.jobs.reset()
ex.extraTxnState.validateDbZoneConfig = false
ex.extraTxnState.schemaChangerState.memAcc.Clear(ctx)
ex.extraTxnState.schemaChangerState = &SchemaChangerState{
mode: ex.sessionData().NewSchemaChangerMode,
Expand Down Expand Up @@ -3403,14 +3407,15 @@ func (ex *connExecutor) initEvalCtx(ctx context.Context, evalCtx *extendedEvalCo
RangeStatsFetcher: p.execCfg.RangeStatsFetcher,
JobsProfiler: p,
},
Tracing: &ex.sessionTracing,
MemMetrics: &ex.memMetrics,
Descs: ex.extraTxnState.descCollection,
TxnModesSetter: ex,
jobs: ex.extraTxnState.jobs,
statsProvider: ex.server.sqlStats,
indexUsageStats: ex.indexUsageStats,
statementPreparer: ex,
Tracing: &ex.sessionTracing,
MemMetrics: &ex.memMetrics,
Descs: ex.extraTxnState.descCollection,
TxnModesSetter: ex,
jobs: ex.extraTxnState.jobs,
validateDbZoneConfig: &ex.extraTxnState.validateDbZoneConfig,
statsProvider: ex.server.sqlStats,
indexUsageStats: ex.indexUsageStats,
statementPreparer: ex,
}
evalCtx.copyFromExecCfg(ex.server.cfg)
}
Expand Down
31 changes: 31 additions & 0 deletions pkg/sql/conn_executor_exec.go
Original file line number Diff line number Diff line change
Expand Up @@ -1311,6 +1311,10 @@ func (ex *connExecutor) commitSQLTransactionInternal(ctx context.Context) error
ex.state.mu.txn.ConfigureStepping(ctx, prevSteppingMode)
}

if err := ex.validateDbZoneConfigs(ctx); err != nil {
return err
}

if err := ex.createJobs(ctx); err != nil {
return err
}
Expand Down Expand Up @@ -1356,6 +1360,33 @@ func (ex *connExecutor) commitSQLTransactionInternal(ctx context.Context) error
return nil
}

// validateDbZoneConfigs validates zone configs for any modified databases.
func (ex *connExecutor) validateDbZoneConfigs(ctx context.Context) error {
// If no DDL was executed requiring pre-commit validation,
// then nothing to do.
if !ex.extraTxnState.validateDbZoneConfig {
return nil
}
for _, db := range ex.extraTxnState.descCollection.GetUncommittedDatabases() {
regionConfig, err := SynthesizeRegionConfig(
ctx, ex.planner.txn, db.GetID(), ex.planner.Descriptors(),
)
if err != nil {
return err
}
_, err = generateAndValidateZoneConfigForMultiRegionDatabase(ctx,
ex.planner.regionsProvider(),
ex.planner.execCfg,
regionConfig,
true, /*validateLocalities*/
)
if err != nil {
return err
}
}
return nil
}

// createJobs creates jobs for the records cached in schemaChangeJobRecords
// during this transaction.
func (ex *connExecutor) createJobs(ctx context.Context) error {
Expand Down
3 changes: 3 additions & 0 deletions pkg/sql/planner.go
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,9 @@ type extendedEvalContext struct {
SchemaChangerState *SchemaChangerState

statementPreparer statementPreparer

// validateDbZoneConfig should the DB zone config on commit.
validateDbZoneConfig *bool
}

// copyFromExecCfg copies relevant fields from an ExecutorConfig.
Expand Down

0 comments on commit 3db9d6b

Please sign in to comment.