Merge #70581

70581: sql: disallow cross-database sequence references r=fqazi a=fqazi Fixes: #69992 Previously, we incorrectly allowed cross DB references for sequences. This was inadequate because we are going to drop support for cross DB references. To address this this patch will disable support for cross DB sequence references by default. Release note (sql change): Disallow cross DB references for sequences by default. This can be enabled with the cluster setting sql.cross_db_sequence_references.enabled. Co-authored-by: Faizan Qazi <[email protected]>
cockroachdb · Sep 28, 2021 · f1b2494 · f1b2494
2 parents 19e9716 + 3b8d7f7
commit f1b2494
Show file tree

Hide file tree

Showing 10 changed files with 164 additions and 63 deletions.
diff --git a/docs/generated/settings/settings-for-tenants.txt b/docs/generated/settings/settings-for-tenants.txt
@@ -74,6 +74,7 @@ server.web_session.purge.ttl	duration	1h0m0s	if nonzero, entries in system.web_s
 server.web_session_timeout	duration	168h0m0s	the duration that a newly created web session will be valid
 sql.cross_db_fks.enabled	boolean	false	if true, creating foreign key references across databases is allowed
 sql.cross_db_sequence_owners.enabled	boolean	false	if true, creating sequences owned by tables from other databases is allowed
+sql.cross_db_sequence_references.enabled	boolean	false	if true, sequences referenced by tables from other databases are allowed
 sql.cross_db_views.enabled	boolean	false	if true, creating views that refer to other databases is allowed
 sql.defaults.copy_partitioning_when_deinterleaving_table.enabled	boolean	false	default value for enable_copying_partitioning_when_deinterleaving_table session variable
 sql.defaults.datestyle	enumeration	iso, mdy	default value for DateStyle session setting [iso, mdy = 0, iso, dmy = 1, iso, ymd = 2]

diff --git a/docs/generated/settings/settings.html b/docs/generated/settings/settings.html
@@ -79,6 +79,7 @@
 <tr><td><code>server.web_session_timeout</code></td><td>duration</td><td><code>168h0m0s</code></td><td>the duration that a newly created web session will be valid</td></tr>
 <tr><td><code>sql.cross_db_fks.enabled</code></td><td>boolean</td><td><code>false</code></td><td>if true, creating foreign key references across databases is allowed</td></tr>
 <tr><td><code>sql.cross_db_sequence_owners.enabled</code></td><td>boolean</td><td><code>false</code></td><td>if true, creating sequences owned by tables from other databases is allowed</td></tr>
+<tr><td><code>sql.cross_db_sequence_references.enabled</code></td><td>boolean</td><td><code>false</code></td><td>if true, sequences referenced by tables from other databases are allowed</td></tr>
 <tr><td><code>sql.cross_db_views.enabled</code></td><td>boolean</td><td><code>false</code></td><td>if true, creating views that refer to other databases is allowed</td></tr>
 <tr><td><code>sql.defaults.copy_partitioning_when_deinterleaving_table.enabled</code></td><td>boolean</td><td><code>false</code></td><td>default value for enable_copying_partitioning_when_deinterleaving_table session variable</td></tr>
 <tr><td><code>sql.defaults.datestyle</code></td><td>enumeration</td><td><code>iso, mdy</code></td><td>default value for DateStyle session setting [iso, mdy = 0, iso, dmy = 1, iso, ymd = 2]</td></tr>

diff --git a/pkg/sql/exec_util.go b/pkg/sql/exec_util.go
@@ -168,6 +168,14 @@ var allowCrossDatabaseSeqOwner = settings.RegisterBoolSetting(
 	false,
 ).WithPublic()
 
+const allowCrossDatabaseSeqReferencesSetting = "sql.cross_db_sequence_references.enabled"
+
+var allowCrossDatabaseSeqReferences = settings.RegisterBoolSetting(
+	allowCrossDatabaseSeqReferencesSetting,
+	"if true, sequences referenced by tables from other databases are allowed",
+	false,
+).WithPublic()
+
 const secondaryTenantsZoneConfigsEnabledSettingName = "sql.zone_configs.experimental_allow_for_secondary_tenant.enabled"
 
 // secondaryTenantZoneConfigsEnabled controls if secondary tenants are allowed

diff --git a/pkg/sql/logictest/testdata/logic_test/drop_database b/pkg/sql/logictest/testdata/logic_test/drop_database
@@ -1,6 +1,9 @@
 statement ok
 SET CLUSTER SETTING sql.cross_db_views.enabled = TRUE
 
+statement ok
+SET CLUSTER SETTING sql.cross_db_sequence_references.enabled = TRUE
+
 statement ok
 CREATE DATABASE "foo-bar"
 

diff --git a/pkg/sql/logictest/testdata/logic_test/drop_sequence b/pkg/sql/logictest/testdata/logic_test/drop_sequence
@@ -3,6 +3,9 @@
 statement ok
 SET sql_safe_updates = true
 
+statement ok
+SET CLUSTER SETTING sql.cross_db_sequence_references.enabled = TRUE
+
 # Test dropping sequences with/without CASCADE
 subtest drop_sequence
 

diff --git a/pkg/sql/logictest/testdata/logic_test/rename_database b/pkg/sql/logictest/testdata/logic_test/rename_database
@@ -3,6 +3,9 @@
 statement ok
 SET CLUSTER SETTING sql.cross_db_views.enabled = TRUE
 
+statement ok
+SET CLUSTER SETTING sql.cross_db_sequence_references.enabled = TRUE
+
 query TTTTT
 SHOW DATABASES
 ----

diff --git a/pkg/sql/logictest/testdata/logic_test/sequences b/pkg/sql/logictest/testdata/logic_test/sequences
@@ -6,6 +6,10 @@
 statement ok
 SET CLUSTER SETTING sql.cross_db_sequence_owners.enabled = TRUE
 
+statement ok
+SET CLUSTER SETTING sql.cross_db_sequence_references.enabled = TRUE
+
+
 statement ok
 SET DATABASE = test
 
@@ -1820,3 +1824,16 @@ test  public  tdb3ref  db3  public  s  table column refers to sequence
 # Testing parsing empty string for currval issue #34527.
 statement error pq: currval\(\): invalid table name:
 SELECT currval('')
+
+statement ok
+SET CLUSTER SETTING sql.cross_db_sequence_references.enabled = FALSE
+
+# Validate that cross DB sequences are detected by internal tables
+statement ok
+CREATE DATABASE db4;
+
+statement ok
+CREATE SEQUENCE db4.s;
+
+statement error pq: sequence references cannot come from other databases; \(see the 'sql\.cross_db_sequence_references\.enabled' cluster setting\)
+CREATE TABLE tDb4Ref (i INT PRIMARY KEY DEFAULT (nextval('db4.s')));
diff --git a/pkg/sql/logictest/testdata/logic_test/sequences_regclass b/pkg/sql/logictest/testdata/logic_test/sequences_regclass
@@ -28,6 +28,12 @@ ALTER TABLE foo ADD COLUMN k SERIAL
 statement ok
 ALTER TABLE foo ADD COLUMN l INT NOT NULL
 
+statement error pq: sequence references cannot come from other databases; \(see the 'sql\.cross_db_sequence_references\.enabled' cluster setting\)
+ALTER TABLE FOO ALTER COLUMN l SET DEFAULT currval('diff_db.test_seq')
+
+statement ok
+SET CLUSTER SETTING sql.cross_db_sequence_references.enabled = TRUE
+
 statement ok
 ALTER TABLE FOO ALTER COLUMN l SET DEFAULT currval('diff_db.test_seq')
 

diff --git a/pkg/sql/rename_table.go b/pkg/sql/rename_table.go
@@ -303,6 +303,9 @@ func (n *renameTableNode) checkForCrossDbReferences(
 	// Checks inbound / outbound foreign key references for cross DB references.
 	// The refTableID flag determines if the reference or origin field are checked.
 	checkFkForCrossDbDep := func(fk *descpb.ForeignKeyConstraint, refTableID bool) error {
+		if allowCrossDatabaseFKs.Get(&p.execCfg.Settings.SV) {
+			return nil
+		}
 		tableID := fk.ReferencedTableID
 		if !refTableID {
 			tableID = fk.OriginTableID
@@ -322,6 +325,7 @@ func (n *renameTableNode) checkForCrossDbReferences(
 		if referencedTable.GetParentID() == targetDbDesc.GetID() {
 			return nil
 		}
+
 		return errors.WithHintf(
 			pgerror.Newf(pgcode.ObjectNotInPrerequisiteState,
 				"a foreign key constraint %q will exist between databases after rename "+
@@ -334,7 +338,9 @@ func (n *renameTableNode) checkForCrossDbReferences(
 	// Validates if a given dependency on a relation will
 	// lead to a cross DB reference, and an appropriate
 	// error is generated.
-	checkDepForCrossDbRef := func(depID descpb.ID) error {
+	type crossDBDepType int
+	const owner, reference crossDBDepType = 0, 1
+	checkDepForCrossDbRef := func(depID descpb.ID, depType crossDBDepType) error {
 		dependentObject, err := p.Descriptors().GetImmutableTableByID(ctx, p.txn, depID,
 			tree.ObjectLookupFlags{
 				CommonLookupFlags: tree.CommonLookupFlags{
@@ -348,53 +354,94 @@ func (n *renameTableNode) checkForCrossDbReferences(
 		if dependentObject.GetParentID() == targetDbDesc.GetID() {
 			return nil
 		}
-		// For tables return an error based on if we are depending
-		// on a view or sequence.
-		if tableDesc.IsTable() {
-			if dependentObject.IsView() {
+		// Based in the primary object.
+		switch {
+		case tableDesc.IsTable():
+			// Based on the dependent objects type, since
+			// for tables the type of the dependent object will
+			// determine the message.
+			switch {
+			case dependentObject.IsView():
+				if !allowCrossDatabaseViews.Get(&p.execCfg.Settings.SV) {
+					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(),
+					)
+				}
+			case dependentObject.IsSequence() && depType == owner:
+				if !allowCrossDatabaseSeqOwner.Get(&p.execCfg.Settings.SV) {
+					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(),
+					)
+				}
+			case dependentObject.IsSequence() && depType == reference:
+				if !allowCrossDatabaseSeqReferences.Get(&p.execCfg.Settings.SV) {
+					return errors.WithHintf(
+						pgerror.Newf(pgcode.ObjectNotInPrerequisiteState,
+							"a sequence %q will be referenced by a table in a different database after rename "+
+								"(see the '%s' cluster setting)",
+							dependentObject.GetName(),
+							allowCrossDatabaseSeqOwnerSetting),
+						crossDBReferenceDeprecationHint(),
+					)
+				}
+			}
+		case tableDesc.IsView():
+			if !allowCrossDatabaseViews.Get(&p.execCfg.Settings.SV) {
+				// For view's dependent objects can only be
+				// relations.
 				return errors.WithHintf(
 					pgerror.Newf(pgcode.ObjectNotInPrerequisiteState,
-						"a view %q reference to this table will refer to another databases after rename "+
+						"this view will reference a table %q in another databases after rename "+
 							"(see the '%s' cluster setting)",
 						dependentObject.GetName(),
 						allowCrossDatabaseViewsSetting),
 					crossDBReferenceDeprecationHint(),
 				)
-			} else if !allowCrossDatabaseSeqOwner.Get(&p.execCfg.Settings.SV) &&
-				dependentObject.IsSequence() {
+			}
+		case tableDesc.IsSequence() && depType == reference:
+			if !allowCrossDatabaseSeqReferences.Get(&p.execCfg.Settings.SV) {
+				// For sequences dependent references can only be
+				// a relations.
 				return errors.WithHintf(
 					pgerror.Newf(pgcode.ObjectNotInPrerequisiteState,
-						"a sequence %q will be OWNED BY a table in a different database after rename "+
+						"this sequence will be referenced by a table %q in a different database after rename "+
 							"(see the '%s' cluster setting)",
 						dependentObject.GetName(),
-						allowCrossDatabaseSeqOwnerSetting),
+						allowCrossDatabaseSeqReferencesSetting),
+					crossDBReferenceDeprecationHint(),
+				)
+			}
+		case tableDesc.IsSequence() && depType == owner:
+			if !allowCrossDatabaseSeqOwner.Get(&p.execCfg.Settings.SV) {
+				// For sequences dependent owners can only be
+				// a relations.
+				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)",
+						dependentObject.GetName(),
+						allowCrossDatabaseSeqReferencesSetting),
 					crossDBReferenceDeprecationHint(),
 				)
 			}
-		} else if tableDesc.IsView() {
-			// For views it can only be a relation.
-			return errors.WithHintf(
-				pgerror.Newf(pgcode.ObjectNotInPrerequisiteState,
-					"this view will reference a table %q in another databases after rename "+
-						"(see the '%s' cluster setting)",
-					dependentObject.GetName(),
-					allowCrossDatabaseViewsSetting),
-				crossDBReferenceDeprecationHint(),
-			)
-		} else if tableDesc.IsSequence() {
-			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)",
-					dependentObject.GetName(),
-					allowCrossDatabaseSeqOwnerSetting),
-				crossDBReferenceDeprecationHint(),
-			)
 		}
 		return nil
 	}
 
 	checkTypeDepForCrossDbRef := func(depID descpb.ID) error {
+		if allowCrossDatabaseViews.Get(&p.execCfg.Settings.SV) {
+			return nil
+		}
 		dependentObject, err := p.Descriptors().GetImmutableTypeByID(ctx, p.txn, depID,
 			tree.ObjectLookupFlags{
 				CommonLookupFlags: tree.CommonLookupFlags{
@@ -421,71 +468,72 @@ func (n *renameTableNode) checkForCrossDbReferences(
 	// For tables check if any outbound or inbound foreign key references would
 	// be impacted.
 	if tableDesc.IsTable() {
-		if !allowCrossDatabaseFKs.Get(&p.execCfg.Settings.SV) {
-			err := tableDesc.ForeachOutboundFK(func(fk *descpb.ForeignKeyConstraint) error {
-				return checkFkForCrossDbDep(fk, true)
-			})
-			if err != nil {
-				return err
-			}
-
-			err = tableDesc.ForeachInboundFK(func(fk *descpb.ForeignKeyConstraint) error {
-				return checkFkForCrossDbDep(fk, false)
-			})
-			if err != nil {
-				return err
-			}
+		err := tableDesc.ForeachOutboundFK(func(fk *descpb.ForeignKeyConstraint) error {
+			return checkFkForCrossDbDep(fk, true)
+		})
+		if err != nil {
+			return err
 		}
 
+		err = tableDesc.ForeachInboundFK(func(fk *descpb.ForeignKeyConstraint) error {
+			return checkFkForCrossDbDep(fk, false)
+		})
+		if err != nil {
+			return err
+		}
 		// If cross database sequence owners are not allowed, then
 		// check if any column owns a sequence.
-		if !allowCrossDatabaseSeqOwner.Get(&p.execCfg.Settings.SV) {
-			for _, columnDesc := range tableDesc.Columns {
-				for _, ownsSequenceID := range columnDesc.OwnsSequenceIds {
-					err := checkDepForCrossDbRef(ownsSequenceID)
-					if err != nil {
-						return err
-					}
+		for _, columnDesc := range tableDesc.Columns {
+			for _, ownsSequenceID := range columnDesc.OwnsSequenceIds {
+				if err := checkDepForCrossDbRef(ownsSequenceID, owner); err != nil {
+					return err
+				}
+			}
+			for _, seqID := range columnDesc.UsesSequenceIds {
+				if err := checkDepForCrossDbRef(seqID, reference); err != nil {
+					return err
 				}
 			}
 		}
-
 		// 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 {
-				return checkDepForCrossDbRef(dep.ID)
+				return checkDepForCrossDbRef(dep.ID, reference)
 			})
 			if err != nil {
 				return err
 			}
 		}
-	} else if tableDesc.IsView() &&
-		!allowCrossDatabaseViews.Get(&p.execCfg.Settings.SV) {
+	} else if tableDesc.IsView() {
 		// For views check if we depend on tables in a different database.
 		dependsOn := tableDesc.GetDependsOn()
 		for _, dependency := range dependsOn {
-			err := checkDepForCrossDbRef(dependency)
-			if err != nil {
+			if err := checkDepForCrossDbRef(dependency, reference); err != nil {
 				return err
 			}
 		}
 		// Check if we depend on types in a different database.
 		dependsOnTypes := tableDesc.GetDependsOnTypes()
 		for _, dependency := range dependsOnTypes {
-			err := checkTypeDepForCrossDbRef(dependency)
-			if err != nil {
+			if err := checkTypeDepForCrossDbRef(dependency); err != nil {
 				return err
 			}
 		}
-	} else if tableDesc.IsSequence() &&
-		!allowCrossDatabaseSeqOwner.Get(&p.execCfg.Settings.SV) {
-		// For sequences check if the sequence is owned by
-		// a different database.
+	} else if tableDesc.IsSequence() {
+		// Check if the sequence is owned by a different database.
 		sequenceOpts := tableDesc.GetSequenceOpts()
 		if sequenceOpts.SequenceOwner.OwnerTableID != descpb.InvalidID {
-			err := checkDepForCrossDbRef(sequenceOpts.SequenceOwner.OwnerTableID)
+			err := checkDepForCrossDbRef(sequenceOpts.SequenceOwner.OwnerTableID, owner)
+			if err != nil {
+				return err
+			}
+		}
+		// Check if a table in a different database depends on this
+		// sequence.
+		for _, sequenceReferences := range tableDesc.GetDependedOnBy() {
+			err := checkDepForCrossDbRef(sequenceReferences.ID, reference)
 			if err != nil {
 				return err
 			}

diff --git a/pkg/sql/sequence.go b/pkg/sql/sequence.go
@@ -611,6 +611,17 @@ func maybeAddSequenceDependencies(
 		if err != nil {
 			return nil, err
 		}
+		// Check if this reference is cross DB.
+		if seqDesc.GetParentID() != tableDesc.GetParentID() &&
+			!allowCrossDatabaseSeqReferences.Get(&st.SV) {
+			return nil, errors.WithHintf(
+				pgerror.Newf(pgcode.FeatureNotSupported,
+					"sequence references cannot come from other databases; (see the '%s' cluster setting)",
+					allowCrossDatabaseSeqReferencesSetting),
+				crossDBReferenceDeprecationHint(),
+			)
+
+		}
 		seqNameToID[seqIdentifier.SeqName] = int64(seqDesc.ID)
 
 		// If we had already modified this Sequence as part of this transaction,