Skip to content

Commit

Permalink
sql: disallow table/view/sequence rename from making cross DB references
Browse files Browse the repository at this point in the history
Fixes: cockroachdb#55709

Previously, disallowed cross DB references could be created
using a ALTER TABLE/VIEW/SEQUENCE rename operations. This
was inadequate because users could accidentally start using
deprecated functionality. To address this, this patch detects
such scenarios are returns an appropriate error.

Release note (bug fix): ALTER TABLE/VIEW/SEQUENCE can no longer
be used to incorrectly create cross DB references.

Release justification: Low risk fix to avoid users from accidentally,
using deprecated functionality.
  • Loading branch information
fqazi committed Mar 10, 2021
1 parent 8b8c6b0 commit 5ed5961
Show file tree
Hide file tree
Showing 2 changed files with 267 additions and 0 deletions.
83 changes: 83 additions & 0 deletions pkg/sql/logictest/testdata/logic_test/rename_table
Original file line number Diff line number Diff line change
Expand Up @@ -381,3 +381,86 @@ DROP DATABASE olddb CASCADE

statement ok
DROP DATABASE newdb CASCADE

# Sanity check for #55709 renaming tables and cross DB references

statement ok
CREATE DATABASE NEWDB;

statement ok
use NEWDB;

# First validate renames are blocked

statement ok
create table t1Ref(i int PRIMARY KEY);

statement ok
create table t2Ref(j int, constraint t1_reference foreign key(j) references t1Ref(i));

statement error a foreign key constraint "t1_reference" will exist between databases after rename
alter table t2Ref rename to test2.public.t2;

statement error a foreign key constraint "t1_reference" will exist between databases after rename
alter table t1Ref rename to test2.public.t1;


statement ok
create table t1VRef(i int PRIMARY KEY);

statement ok
create view t1View as (select i from t1VRef);

statement error cannot rename relation "t1vref" because view "t1view" depends on it
alter table t1VRef rename to test2.public.t1Vref;

statement error this view will reference a table "t1vref" in another databases after rename
alter view t1View rename to test2.public.t1View;

statement ok
create table t1S1Ref(i int PRIMARY KEY);

statement ok
create sequence s1 owned by t1S1Ref.i;

statement error a sequence "s1" will be OWNED BY a table in a different database after rename
alter table t1S1Ref rename to test2.public.t1S1Ref;

statement error this sequence will be OWNED BY a table "t1s1ref" in a different database after rename
alter sequence s1 rename to test2.public.s1;

# Enable cluster setting to allow foreign keys across DB's
statement ok
SET CLUSTER SETTING sql.cross_db_fks.enabled = TRUE

statement ok
alter table t2Ref rename to test2.public.t2Ref;

statement ok
alter table test2.public.t2Ref rename to t2Ref;

statement ok
alter table t1Ref rename to test2.public.t1;

#Enable cluster setting for views across DB's
statement ok
SET CLUSTER SETTING sql.cross_db_views.enabled = TRUE

statement error cannot rename relation "t1vref" because view "t1view" depends on it
alter table t1VRef rename to test2.public.t1Vref;

statement ok
alter view t1View rename to test2.public.t1View;

#Enable cluster setting for sequence owned by across DB's
statement ok
SET CLUSTER SETTING sql.cross_db_sequence_owners.enabled = TRUE

statement ok
alter table t1S1Ref rename to test2.public.t1S1Ref;

statement ok
alter table test2.public.t1S1Ref rename to t1S1Ref;

statement ok
alter sequence s1 rename to test2.public.s1;
184 changes: 184 additions & 0 deletions pkg/sql/rename_table.go
Original file line number Diff line number Diff line change
Expand Up @@ -188,6 +188,190 @@ func (n *renameTableNode) startExec(params runParams) error {
}
}

// Special checks for tables, view and sequences to determine if cross
// DB references would occur.
if oldTn.Catalog() != newTn.Catalog() {
if tableDesc.IsTable() {
// For tables check if any outbound or inbound foreign key references would
// be impacted.
if !allowCrossDatabaseFKs.Get(&p.execCfg.Settings.SV) {
err := tableDesc.ForeachOutboundFK(func(fk *descpb.ForeignKeyConstraint) error {
referencedTable, err := p.Descriptors().GetImmutableTableByID(ctx, p.txn, fk.ReferencedTableID,
tree.ObjectLookupFlags{
CommonLookupFlags: tree.CommonLookupFlags{
Required: true,
},
})
if err != nil {
return err
}
if referencedTable.GetParentID() != targetDbDesc.GetID() {
return errors.WithHintf(
pgerror.Newf(pgcode.ObjectNotInPrerequisiteState,
"a foreign key constraint %q will exist between databases after rename "+
"(see the '%s' cluster setting)",
fk.Name,
allowCrossDatabaseFKsSetting),
crossDBReferenceDeprecationHint(),
)
}
return nil
})
if err != nil {
return err
}

err = tableDesc.ForeachInboundFK(func(fk *descpb.ForeignKeyConstraint) error {
referencedTable, err := p.Descriptors().GetImmutableTableByID(ctx, p.txn, fk.OriginTableID,
tree.ObjectLookupFlags{
CommonLookupFlags: tree.CommonLookupFlags{
Required: true,
}})
if err != nil {
return err
}
if referencedTable.GetParentID() != targetDbDesc.GetID() {
return errors.WithHintf(
pgerror.Newf(pgcode.ObjectNotInPrerequisiteState,
"a foreign key constraint %q will exist between databases after rename "+
"(see the '%s' cluster setting)",
fk.Name,
allowCrossDatabaseFKsSetting),
crossDBReferenceDeprecationHint(),
)
}
return nil
})
if err != nil {
return err
}
}

// If cross database sequence owners are not allowed, we need to
// get all descriptors to validate, since DependsOnBy is only
// populate if this sequence is a default column.
if !allowCrossDatabaseSeqOwner.Get(&p.execCfg.Settings.SV) {
allDescriptors, err := p.Descriptors().GetAllDescriptors(ctx, p.txn)
if err != nil {
return err
}
for _, descriptor := range allDescriptors {
if seqDesc, ok := descriptor.(catalog.TableDescriptor); ok {
if seqDesc.IsSequence() {
// For sequences check if the sequence is owned by
// a different database.
sequenceOpts := seqDesc.GetSequenceOpts()
if sequenceOpts != nil &&
sequenceOpts.SequenceOwner.OwnerTableID == tableDesc.GetID() &&
seqDesc.GetParentID() != targetDbDesc.GetID() {
return errors.WithHintf(
pgerror.Newf(pgcode.ObjectNotInPrerequisiteState,
"a sequence %q will be OWNED BY a table in a different database after rename "+
"(see the '%s' cluster setting)",
seqDesc.GetName(),
allowCrossDatabaseSeqOwnerSetting),
crossDBReferenceDeprecationHint(),
)
}
}
}
}
}

// Check if any views depend on this table, while
// DependsOnBy contains sequences these are only
// once that are in use.
if !allowCrossDatabaseViews.Get(&p.execCfg.Settings.SV) {

err := tableDesc.ForeachDependedOnBy(func(dep *descpb.TableDescriptor_Reference) error {
dependentObject, err := p.Descriptors().GetImmutableTableByID(ctx, p.txn, dep.ID,
tree.ObjectLookupFlags{
CommonLookupFlags: tree.CommonLookupFlags{
Required: true,
}})
if err != nil {
return err
}
if dependentObject.GetParentID() != targetDbDesc.GetID() {
if dependentObject.IsView() {
return errors.WithHintf(
pgerror.Newf(pgcode.ObjectNotInPrerequisiteState,
"a view %q reference to this table will refer to another databases after rename "+
"(see the '%s' cluster setting)",
dependentObject.GetName(),
allowCrossDatabaseViewsSetting),
crossDBReferenceDeprecationHint(),
)
} else if !allowCrossDatabaseSeqOwner.Get(&p.execCfg.Settings.SV) &&
dependentObject.IsSequence() {
return errors.WithHintf(
pgerror.Newf(pgcode.ObjectNotInPrerequisiteState,
"a sequence %q will be OWNED BY a table in a different database after rename "+
"(see the '%s' cluster setting)",
dependentObject.GetName(),
allowCrossDatabaseSeqOwnerSetting),
crossDBReferenceDeprecationHint(),
)
}
}
return nil
})
if err != nil {
return err
}
}
} else if tableDesc.IsView() &&
!allowCrossDatabaseViews.Get(&p.execCfg.Settings.SV) {
// For views check if we depend on tables in a different database.
dependsOn := tableDesc.GetDependsOn()
for _, dependency := range dependsOn {
dependentTable, err := p.Descriptors().GetImmutableTableByID(ctx, p.txn, dependency,
tree.ObjectLookupFlags{
CommonLookupFlags: tree.CommonLookupFlags{
Required: true,
}})
if err != nil {
return err
}
if dependentTable.GetParentID() != targetDbDesc.GetID() {
return errors.WithHintf(
pgerror.Newf(pgcode.ObjectNotInPrerequisiteState,
"this view will reference a table %q in another databases after rename "+
"(see the '%s' cluster setting)",
dependentTable.GetName(),
allowCrossDatabaseViewsSetting),
crossDBReferenceDeprecationHint(),
)
}
}
} else if tableDesc.IsSequence() &&
!allowCrossDatabaseSeqOwner.Get(&p.execCfg.Settings.SV) {
// For sequences check if the sequence is owned by
// a different database.
sequenceOpts := tableDesc.GetSequenceOpts()
if sequenceOpts.SequenceOwner.OwnerTableID != descpb.InvalidID {
ownerTable, err := p.Descriptors().GetImmutableTableByID(ctx, p.txn, sequenceOpts.SequenceOwner.OwnerTableID,
tree.ObjectLookupFlags{
CommonLookupFlags: tree.CommonLookupFlags{
Required: true,
}})
if err != nil {
return err
}
if ownerTable.GetParentID() != targetDbDesc.GetID() {
return errors.WithHintf(
pgerror.Newf(pgcode.ObjectNotInPrerequisiteState,
"this sequence will be OWNED BY a table %q in a different database after rename "+
"(see the '%s' cluster setting)",
ownerTable.GetName(),
allowCrossDatabaseSeqOwnerSetting),
crossDBReferenceDeprecationHint(),
)
}
}
}
}

// oldTn and newTn are already normalized, so we can compare directly here.
if oldTn.Catalog() == newTn.Catalog() &&
oldTn.Schema() == newTn.Schema() &&
Expand Down

0 comments on commit 5ed5961

Please sign in to comment.