diff --git a/pkg/ccl/logictestccl/testdata/logic_test/multi_region_privileges b/pkg/ccl/logictestccl/testdata/logic_test/multi_region_privileges new file mode 100644 index 000000000000..6400a758bd7e --- /dev/null +++ b/pkg/ccl/logictestccl/testdata/logic_test/multi_region_privileges @@ -0,0 +1,60 @@ +# LogicTest: multiregion-9node-3region-3azs + +user root + +statement ok +CREATE DATABASE db; +CREATE TABLE db.t(); +GRANT CREATE ON DATABASE db TO testuser; +CREATE TABLE db.t2(); +ALTER USER testuser CREATEDB; + +user testuser + +statement error user testuser does not have CREATE or ZONECONFIG privilege on t +ALTER DATABASE db SET PRIMARY REGION "us-east-1" + +user root + +statement ok +GRANT ZONECONFIG ON TABLE db.t TO testuser + +user testuser + +statement ok +ALTER DATABASE db SET PRIMARY REGION "us-east-1" + +user root + +statement ok +REVOKE ZONECONFIG ON TABLE db.t FROM testuser + +user testuser + +statement error user testuser does not have CREATE or ZONECONFIG privilege on t +ALTER DATABASE db DROP REGION "us-east-1" + +user root + +statement ok +GRANT ZONECONFIG ON TABLE db.t TO testuser + +user testuser + +statement ok +ALTER DATABASE db DROP REGION "us-east-1" + +# Now the same thing, except this time we grant the CREATE privilege to testuser. +user root + +statement ok +REVOKE ZONECONFIG ON TABLE db.t FROM testuser; +GRANT CREATE ON TABLE db.t TO testuser + +user testuser + +statement ok +ALTER DATABASE db SET PRIMARY REGION "us-east-1" + +statement ok +ALTER DATABASE db DROP REGION "us-east-1" diff --git a/pkg/sql/alter_database.go b/pkg/sql/alter_database.go index 47e5d2e4f3bb..a3a18d584bac 100644 --- a/pkg/sql/alter_database.go +++ b/pkg/sql/alter_database.go @@ -21,9 +21,11 @@ import ( "github.com/cockroachdb/cockroach/pkg/sql/catalog/dbdesc" "github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb" "github.com/cockroachdb/cockroach/pkg/sql/catalog/multiregion" + "github.com/cockroachdb/cockroach/pkg/sql/catalog/tabledesc" "github.com/cockroachdb/cockroach/pkg/sql/catalog/typedesc" "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgcode" "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror" + "github.com/cockroachdb/cockroach/pkg/sql/privilege" "github.com/cockroachdb/cockroach/pkg/sql/roleoption" "github.com/cockroachdb/cockroach/pkg/sql/sem/tree" "github.com/cockroachdb/cockroach/pkg/sql/sqlerrors" @@ -358,6 +360,24 @@ func (p *planner) AlterDatabaseDropRegion( }, nil } +// ensureCorrectMultiRegionPrivileges ensures the current user has the required +// privileges to alter the locality configuration of the given table descriptor. +// Currently, this entails that the user must have either CREATE or ZONECONFIG +// privilege on the table descriptor. +// TODO(arul): This ought to be unified with `ALTER TABLE LOCALITY` as well. +func ensureCorrectMultiRegionPrivileges( + ctx context.Context, p *planner, tableDesc catalog.TableDescriptor, +) error { + zoneConfigPrivErr := p.CheckPrivilege(ctx, tableDesc, privilege.ZONECONFIG) + createPrivErr := p.CheckPrivilege(ctx, tableDesc, privilege.CREATE) + if zoneConfigPrivErr != nil && createPrivErr != nil { + return pgerror.Newf(pgcode.InsufficientPrivilege, + "user %s does not have %s or %s privilege on %s", + p.SessionData().User(), privilege.CREATE, privilege.ZONECONFIG, tableDesc.GetName()) + } + return nil +} + // removeLocalityConfigFromAllTablesInDB removes the locality config from all // tables under the supplied database. func removeLocalityConfigFromAllTablesInDB( @@ -369,19 +389,25 @@ func removeLocalityConfigFromAllTablesInDB( desc.GetID(), ) } + hasAdminRole, err := p.HasAdminRole(ctx) + if err != nil { + return err + } b := p.Txn().NewBatch() - if err := forEachTableDesc(ctx, p, desc, hideVirtual, - func(immutable *dbdesc.Immutable, _ string, desc catalog.TableDescriptor) error { - mutDesc, err := p.Descriptors().GetMutableTableByID(ctx, p.txn, desc.GetID(), tree.ObjectLookupFlags{}) - if err != nil { - return err - } - mutDesc.LocalityConfig = nil - if err := p.writeSchemaChangeToBatch(ctx, mutDesc, b); err != nil { + if err := p.forEachTableInMultiRegionDatabase(ctx, desc, func(ctx context.Context, tbDesc *tabledesc.Mutable) error { + // The user must either be an admin or have the requisite privileges. + if !hasAdminRole { + if err := ensureCorrectMultiRegionPrivileges(ctx, p, tbDesc); err != nil { return err } - return nil - }); err != nil { + } + + tbDesc.LocalityConfig = nil + if err := p.writeSchemaChangeToBatch(ctx, tbDesc, b); err != nil { + return err + } + return nil + }); err != nil { return err } return p.Txn().Run(ctx, b) @@ -574,39 +600,41 @@ func addDefaultLocalityConfigToAllTables( dbDesc.GetID(), ) } + hasAdminRole, err := p.HasAdminRole(ctx) + if err != nil { + return err + } b := p.Txn().NewBatch() - if err := forEachTableDesc(ctx, p, dbDesc, hideVirtual, - func(immutable *dbdesc.Immutable, _ string, desc catalog.TableDescriptor) error { - mutDesc, err := p.Descriptors().GetMutableTableByID( - ctx, p.txn, desc.GetID(), tree.ObjectLookupFlags{}, - ) - if err != nil { + if err := p.forEachTableInMultiRegionDatabase(ctx, dbDesc, func(ctx context.Context, tbDesc *tabledesc.Mutable) error { + // The user must either be an admin or have the requisite privileges. + if !hasAdminRole { + if err := ensureCorrectMultiRegionPrivileges(ctx, p, tbDesc); err != nil { return err } + } - if err := checkCanConvertTableToMultiRegion(dbDesc, mutDesc); err != nil { - return err - } + if err := checkCanConvertTableToMultiRegion(dbDesc, tbDesc); err != nil { + return err + } - if mutDesc.MaterializedView() { - if err := p.alterTableDescLocalityToGlobal( - ctx, mutDesc, regionEnumID, - ); err != nil { - return err - } - } else { - if err := p.alterTableDescLocalityToRegionalByTable( - ctx, tree.PrimaryRegionNotSpecifiedName, mutDesc, regionEnumID, - ); err != nil { - return err - } + if tbDesc.MaterializedView() { + if err := p.alterTableDescLocalityToGlobal( + ctx, tbDesc, regionEnumID, + ); err != nil { + return err } - - if err := p.writeSchemaChangeToBatch(ctx, mutDesc, b); err != nil { + } else { + if err := p.alterTableDescLocalityToRegionalByTable( + ctx, tree.PrimaryRegionNotSpecifiedName, tbDesc, regionEnumID, + ); err != nil { return err } - return nil - }); err != nil { + } + if err := p.writeSchemaChangeToBatch(ctx, tbDesc, b); err != nil { + return err + } + return nil + }); err != nil { return err } return p.Txn().Run(ctx, b) diff --git a/pkg/sql/region_util.go b/pkg/sql/region_util.go index c53199e7d28b..47ea374c9a9a 100644 --- a/pkg/sql/region_util.go +++ b/pkg/sql/region_util.go @@ -19,6 +19,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/config/zonepb" "github.com/cockroachdb/cockroach/pkg/kv" + "github.com/cockroachdb/cockroach/pkg/security" "github.com/cockroachdb/cockroach/pkg/sql/catalog" "github.com/cockroachdb/cockroach/pkg/sql/catalog/dbdesc" "github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb" @@ -62,7 +63,7 @@ func (p *planner) getLiveClusterRegions(ctx context.Context) (LiveClusterRegions "get_live_cluster_regions", p.txn, sessiondata.InternalExecutorOverride{ - User: p.SessionData().User(), + User: security.RootUserName(), }, "SELECT region FROM [SHOW REGIONS FROM CLUSTER]", ) @@ -700,64 +701,31 @@ func applyZoneConfigForMultiRegionDatabase( return nil } -// forEachTableWithLocalityConfigInDatabase loops through each schema and table -// for a table with a LocalityConfig configured. -// NOTE: this function uses cached table and schema descriptors. As a result, it may -// not be safe to run within a schema change. -// TODO(arul): This looks like a remnant of when we could have tables inside an -// MR database without a locality config. Investigate if this can be cleaned up. -func (p *planner) forEachTableWithLocalityConfigInDatabase( +// forEachTableInMultiRegionDatabase calls the given function on every table +// descriptor inside the given multi-region database. Tables that have been +// dropped are skipped. +func (p *planner) forEachTableInMultiRegionDatabase( ctx context.Context, - desc *dbdesc.Mutable, - f func(ctx context.Context, schema string, tbName tree.TableName, tbDesc *tabledesc.Mutable) error, + dbDesc *dbdesc.Immutable, + fn func(ctx context.Context, tbDesc *tabledesc.Mutable) error, ) error { - // No work to be done if the database isn't a multi-region database. - if !desc.IsMultiRegion() { - return nil + if !dbDesc.IsMultiRegion() { + return errors.AssertionFailedf("db %q is not multi-region", dbDesc.Name) } - lookupFlags := p.CommonLookupFlags(true /*required*/) - lookupFlags.AvoidCached = false - schemas, err := p.Descriptors().GetSchemasForDatabase(ctx, p.txn, desc.GetID()) + allDescs, err := p.Descriptors().GetAllDescriptors(ctx, p.txn) if err != nil { return err } - tblLookupFlags := p.CommonLookupFlags(true /*required*/) - tblLookupFlags.AvoidCached = false - tblLookupFlags.Required = false - - // Loop over all schemas, then loop over all tables. - for _, schema := range schemas { - tbNames, err := p.Descriptors().GetObjectNames( - ctx, - p.txn, - desc, - schema, - tree.DatabaseListFlags{ - CommonLookupFlags: lookupFlags, - ExplicitPrefix: true, - }, - ) - if err != nil { - return err + lCtx := newInternalLookupCtx(ctx, allDescs, dbDesc, nil /* fallback */) + for _, tbID := range lCtx.tbIDs { + desc := lCtx.tbDescs[tbID] + if desc.Dropped() { + continue } - for i := range tbNames { - found, tbDesc, err := p.Descriptors().GetMutableTableByName( - ctx, p.txn, &tbNames[i], tree.ObjectLookupFlags{CommonLookupFlags: tblLookupFlags}, - ) - if err != nil { - return err - } - - // If we couldn't find the table, or it has no LocalityConfig, there's nothing - // to do here. - if !found || tbDesc.LocalityConfig == nil { - continue - } - - if err := f(ctx, schema, tbNames[i], tbDesc); err != nil { - return err - } + mutable := tabledesc.NewBuilder(desc.TableDesc()).BuildExistingMutableTable() + if err := fn(ctx, mutable); err != nil { + return err } } return nil @@ -766,10 +734,10 @@ func (p *planner) forEachTableWithLocalityConfigInDatabase( // updateZoneConfigsForAllTables loops through all of the tables in the // specified database and refreshes the zone configs for all tables. func (p *planner) updateZoneConfigsForAllTables(ctx context.Context, desc *dbdesc.Mutable) error { - return p.forEachTableWithLocalityConfigInDatabase( + return p.forEachTableInMultiRegionDatabase( ctx, - desc, - func(ctx context.Context, schema string, tbName tree.TableName, tbDesc *tabledesc.Mutable) error { + &desc.Immutable, + func(ctx context.Context, tbDesc *tabledesc.Mutable) error { regionConfig, err := SynthesizeRegionConfig(ctx, p.txn, desc.ID, p.Descriptors()) if err != nil { return err diff --git a/pkg/sql/type_change.go b/pkg/sql/type_change.go index db043dee2579..8da2358bdce3 100644 --- a/pkg/sql/type_change.go +++ b/pkg/sql/type_change.go @@ -486,10 +486,6 @@ func repartitionRegionalByRowTables( defer cleanup() localPlanner := p.(*planner) - allDescs, err := localPlanner.Descriptors().GetAllDescriptors(ctx, txn) - if err != nil { - return nil, err - } _, dbDesc, err := descsCol.GetImmutableDatabaseByID( ctx, txn, typeDesc.ParentID, tree.DatabaseLookupFlags{ Required: true, @@ -497,112 +493,105 @@ func repartitionRegionalByRowTables( if err != nil { return nil, err } - lCtx := newInternalLookupCtx(ctx, allDescs, dbDesc, nil /* fallback */) b := txn.NewBatch() - for _, tbID := range lCtx.tbIDs { - tableDesc, err := localPlanner.Descriptors().GetMutableTableByID( - ctx, txn, tbID, tree.ObjectLookupFlags{ - CommonLookupFlags: tree.CommonLookupFlags{ - AvoidCached: true, - Required: true, - IncludeDropped: true, - }, - }) - if err != nil { - return nil, err - } + err = localPlanner.forEachTableInMultiRegionDatabase(ctx, dbDesc, + func(ctx context.Context, tableDesc *tabledesc.Mutable) error { + if !tableDesc.IsLocalityRegionalByRow() || tableDesc.Dropped() { + // We only need to re-partition REGIONAL BY ROW tables. Even then, we + // don't need to (can't) repartition a REGIONAL BY ROW table if it has + // been dropped. + return nil + } - if !tableDesc.IsLocalityRegionalByRow() || tableDesc.Dropped() { - // We only need to re-partition REGIONAL BY ROW tables. Even then, we - // don't need to (can't) repartition a REGIONAL BY ROW table if it has - // been dropped. - continue - } + colName, err := tableDesc.GetRegionalByRowTableRegionColumnName() + if err != nil { + return err + } + partitionAllBy := partitionByForRegionalByRow(regionConfig, colName) - colName, err := tableDesc.GetRegionalByRowTableRegionColumnName() - if err != nil { - return nil, err - } - partitionAllBy := partitionByForRegionalByRow(regionConfig, colName) + // oldPartitioningDescs saves the old partitioning descriptors for each + // index that is repartitioned. This is later used to remove zone + // configurations from any partitions that are removed. + oldPartitioningDescs := make(map[descpb.IndexID]descpb.PartitioningDescriptor) - // oldPartitioningDescs saves the old partitioning descriptors for each - // index that is repartitioned. This is later used to remove zone - // configurations from any partitions that are removed. - oldPartitioningDescs := make(map[descpb.IndexID]descpb.PartitioningDescriptor) + // Update the partitioning on all indexes of the table that aren't being + // dropped. + for _, index := range tableDesc.NonDropIndexes() { + newIdx, err := CreatePartitioning( + ctx, + localPlanner.extendedEvalCtx.Settings, + localPlanner.EvalContext(), + tableDesc, + *index.IndexDesc(), + partitionAllBy, + nil, /* allowedNewColumnName*/ + true, /* allowImplicitPartitioning */ + ) + if err != nil { + return err + } - // Update the partitioning on all indexes of the table that aren't being - // dropped. - for _, index := range tableDesc.NonDropIndexes() { - newIdx, err := CreatePartitioning( - ctx, - localPlanner.extendedEvalCtx.Settings, - localPlanner.EvalContext(), - tableDesc, - *index.IndexDesc(), - partitionAllBy, - nil, /* allowedNewColumnName*/ - true, /* allowImplicitPartitioning */ - ) - if err != nil { - return nil, err - } + oldPartitioningDescs[index.GetID()] = index.IndexDesc().Partitioning - oldPartitioningDescs[index.GetID()] = index.IndexDesc().Partitioning + // Update the index descriptor proto's partitioning. + index.IndexDesc().Partitioning = newIdx.Partitioning + } - // Update the index descriptor proto's partitioning. - index.IndexDesc().Partitioning = newIdx.Partitioning - } + // Remove zone configurations that applied to partitions that were removed + // in the previous step. This requires all indexes to have been + // repartitioned such that there is no partitioning on the removed enum + // value. This is because `deleteRemovedPartitionZoneConfigs` generates + // subzone spans for the entire table (all indexes) downstream for each + // index. Spans can only be generated if partitioning values are present on + // the type descriptor (removed enum values obviously aren't), so we must + // remove the partition from all indexes before trying to delete zone + // configurations. + for _, index := range tableDesc.NonDropIndexes() { + oldPartitioning := oldPartitioningDescs[index.GetID()] + + // Remove zone configurations that reference partition values we removed + // in the previous step. + if err = deleteRemovedPartitionZoneConfigs( + ctx, + txn, + tableDesc, + index.IndexDesc(), + &oldPartitioning, + &index.IndexDesc().Partitioning, + execCfg, + ); err != nil { + return err + } + } - // Remove zone configurations that applied to partitions that were removed - // in the previous step. This requires all indexes to have been - // repartitioned such that there is no partitioning on the removed enum - // value. This is because `deleteRemovedPartitionZoneConfigs` generates - // subzone spans for the entire table (all indexes) downstream for each - // index. Spans can only be generated if partitioning values are present on - // the type descriptor (removed enum values obviously aren't), so we must - // remove the partition from all indexes before trying to delete zone - // configurations. - for _, index := range tableDesc.NonDropIndexes() { - oldPartitioning := oldPartitioningDescs[index.GetID()] - - // Remove zone configurations that reference partition values we removed - // in the previous step. - if err = deleteRemovedPartitionZoneConfigs( + // Update the zone configurations now that the partition's been added. + regionConfig, err := SynthesizeRegionConfig(ctx, txn, typeDesc.ParentID, descsCol) + if err != nil { + return err + } + if err := ApplyZoneConfigForMultiRegionTable( ctx, txn, + localPlanner.ExecCfg(), + regionConfig, tableDesc, - index.IndexDesc(), - &oldPartitioning, - &index.IndexDesc().Partitioning, - execCfg, + ApplyZoneConfigForMultiRegionTableOptionTableAndIndexes, ); err != nil { - return nil, err + return err } - } - // Update the zone configurations now that the partition's been added. - regionConfig, err := SynthesizeRegionConfig(ctx, txn, typeDesc.ParentID, descsCol) - if err != nil { - return nil, err - } - if err := ApplyZoneConfigForMultiRegionTable( - ctx, - txn, - localPlanner.ExecCfg(), - regionConfig, - tableDesc, - ApplyZoneConfigForMultiRegionTableOptionTableAndIndexes, - ); err != nil { - return nil, err - } - - if err := localPlanner.Descriptors().WriteDescToBatch(ctx, false /* kvTrace */, tableDesc, b); err != nil { - return nil, err - } + if err := localPlanner.Descriptors().WriteDescToBatch(ctx, false /* kvTrace */, tableDesc, b); err != nil { + return err + } - repartitionedTableIDs = append(repartitionedTableIDs, tbID) + repartitionedTableIDs = append(repartitionedTableIDs, tableDesc.GetID()) + return nil + }) + if err != nil { + return nil, err } + if err := txn.Run(ctx, b); err != nil { return nil, err }