Merge pull request cockroachdb#54520 from RaduBerinde/backport20.2-54…

…407-54475

release-20.2: sql: disallow cross-database foreign keys and views

docs/generated/settings/settings.html

@@ -46,6 +46,8 @@
 <tr><td><code>server.time_until_store_dead</code></td><td>duration</td><td><code>5m0s</code></td><td>the time after which if there is no new gossiped information about a store, it is considered dead</td></tr>
 <tr><td><code>server.user_login.timeout</code></td><td>duration</td><td><code>10s</code></td><td>timeout after which client authentication times out if some system range is unavailable (0 = no timeout)</td></tr>
 <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_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.default_int_size</code></td><td>integer</td><td><code>8</code></td><td>the size, in bytes, of an INT type</td></tr>
 <tr><td><code>sql.defaults.disallow_full_table_scans.enabled</code></td><td>boolean</td><td><code>false</code></td><td>setting to true rejects queries that have planned a full table scan</td></tr>
 <tr><td><code>sql.defaults.results_buffer.size</code></td><td>byte size</td><td><code>16 KiB</code></td><td>default size of the buffer that accumulates results for a statement or a batch of statements before they are sent to the client. This can be overridden on an individual connection with the 'results_buffer_size' parameter. Note that auto-retries generally only happen while no results have been delivered to the client, so reducing this size can increase the number of retriable errors a client receives. On the other hand, increasing the buffer size can increase the delay until the client receives the first result row. Updating the setting only affects new connections. Setting to 0 disables any buffering.</td></tr>

pkg/ccl/backupccl/backup_test.go

@@ -2757,6 +2757,7 @@ func TestBackupRestoreCrossTableReferences(t *testing.T) {
 
 	// Generate some testdata and back it up.
 	{
+		origDB.Exec(t, "SET CLUSTER SETTING sql.cross_db_views.enabled = TRUE")
 		origDB.Exec(t, createStore)
 		origDB.Exec(t, createStoreStats)
 

pkg/ccl/backupccl/show_test.go

@@ -39,6 +39,7 @@ func TestShowBackup(t *testing.T) {
 	defer cleanupFn()
 	defer cleanupEmptyCluster()
 	sqlDB.Exec(t, `
+SET CLUSTER SETTING sql.cross_db_fks.enabled = TRUE;
 CREATE TYPE data.welcome AS ENUM ('hello', 'hi');
 USE data; CREATE SCHEMA sc;
 CREATE TABLE data.sc.t1 (a INT);

pkg/cli/dump_test.go

@@ -708,6 +708,8 @@ INSERT INTO public.t2 (id, pkey) VALUES
 			name:     "dump_cross_references",
 			recreate: true,
 			create: `
+SET CLUSTER SETTING sql.cross_db_fks.enabled = TRUE;
+SET CLUSTER SETTING sql.cross_db_views.enabled = TRUE;
 CREATE DATABASE dbB;
 USE dbB;
 

pkg/sql/alter_table.go

@@ -265,12 +265,11 @@ func (n *alterTableNode) startExec(params runParams) error {
 				// If there are any FKs, we will need to update the table descriptor of the
 				// depended-on table (to register this table against its DependedOnBy field).
 				// This descriptor must be looked up uncached, and we'll allow FK dependencies
-				// on tables that were just added. See the comment at the start of
-				// the global-scope resolveFK().
+				// on tables that were just added. See the comment at the start of ResolveFK().
 				// TODO(vivek): check if the cache can be used.
 				var err error
 				params.p.runWithOptions(resolveFlags{skipCache: true}, func() {
-					// Check whether the table is empty, and pass the result to resolveFK(). If
+					// Check whether the table is empty, and pass the result to ResolveFK(). If
 					// the table is empty, then resolveFK will automatically add the necessary
 					// index for a fk constraint if the index does not exist.
 					span := n.tableDesc.PrimaryIndexSpan(params.ExecCfg().Codec)
@@ -285,7 +284,17 @@ func (n *alterTableNode) startExec(params runParams) error {
 					} else {
 						tableState = NonEmptyTable
 					}
-					err = params.p.resolveFK(params.ctx, n.tableDesc, d, affected, tableState, t.ValidationBehavior)
+					err = ResolveFK(
+						params.ctx,
+						params.p.txn,
+						params.p,
+						n.tableDesc,
+						d,
+						affected,
+						tableState,
+						t.ValidationBehavior,
+						params.p.EvalContext(),
+					)
 				})
 				if err != nil {
 					return err

pkg/sql/create_table.go

@@ -513,21 +513,6 @@ func (n *createTableNode) Close(ctx context.Context) {
 	}
 }
 
-// resolveFK on the planner calls resolveFK() on the current txn.
-//
-// The caller must make sure the planner is configured to look up
-// descriptors without caching. See the comment on resolveFK().
-func (p *planner) resolveFK(
-	ctx context.Context,
-	tbl *tabledesc.Mutable,
-	d *tree.ForeignKeyConstraintTableDef,
-	backrefs map[descpb.ID]*tabledesc.Mutable,
-	ts FKTableState,
-	validationBehavior tree.ValidationBehavior,
-) error {
-	return ResolveFK(ctx, p.txn, p, tbl, d, backrefs, ts, validationBehavior, p.EvalContext())
-}
-
 func qualifyFKColErrorWithDB(
 	ctx context.Context, txn *kv.Txn, codec keys.SQLCodec, tbl *tabledesc.Mutable, col string,
 ) string {
@@ -547,7 +532,7 @@ func qualifyFKColErrorWithDB(
 	return tree.ErrString(tree.NewUnresolvedName(db.GetName(), schema, tbl.Name, col))
 }
 
-// FKTableState is the state of the referencing table resolveFK() is called on.
+// FKTableState is the state of the referencing table ResolveFK() is called on.
 type FKTableState int
 
 const (
@@ -662,6 +647,14 @@ func ResolveFK(
 	if err != nil {
 		return err
 	}
+	if target.ParentID != tbl.ParentID {
+		if !allowCrossDatabaseFKs.Get(&evalCtx.Settings.SV) {
+			return pgerror.Newf(pgcode.InvalidForeignKey,
+				"foreign references between databases are not allowed (see the '%s' cluster setting)",
+				allowCrossDatabaseFKsSetting,
+			)
+		}
+	}
 	if tbl.Temporary != target.Temporary {
 		persistenceType := "permanent"
 		if tbl.Temporary {
@@ -1205,7 +1198,7 @@ func newTableDescIfAs(
 // The caller must also ensure that the SchemaResolver is configured
 // to bypass caching and enable visibility of just-added descriptors.
 // This is used to resolve sequence and FK dependencies. Also see the
-// comment at the start of the global scope resolveFK().
+// comment at the start of ResolveFK().
 //
 // If the table definition *may* use the SERIAL type, the caller is
 // also responsible for processing serial types using
@@ -1823,7 +1816,7 @@ func newTableDesc(
 	// We need to run NewTableDesc with caching disabled, because
 	// it needs to pull in descriptors from FK depended-on tables
 	// and interleaved parents using their current state in KV.
-	// See the comment at the start of NewTableDesc() and resolveFK().
+	// See the comment at the start of NewTableDesc() and ResolveFK().
 	params.p.runWithOptions(resolveFlags{skipCache: true, contextDatabaseID: parentID}, func() {
 		ret, err = NewTableDesc(
 			params.ctx,

pkg/sql/create_view.go

@@ -15,6 +15,7 @@ import (
 	"fmt"
 
 	"github.com/cockroachdb/cockroach/pkg/clusterversion"
+	"github.com/cockroachdb/cockroach/pkg/keys"
 	"github.com/cockroachdb/cockroach/pkg/server/telemetry"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/catalogkv"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/colinfo"
@@ -64,6 +65,18 @@ func (n *createViewNode) startExec(params runParams) error {
 	persistence := n.persistence
 	log.VEventf(params.ctx, 2, "dependencies for view %s:\n%s", viewName, n.planDeps.String())
 
+	// Check that the view does not contain references to other databases.
+	if !allowCrossDatabaseViews.Get(&params.p.execCfg.Settings.SV) {
+		for _, dep := range n.planDeps {
+			if dbID := dep.desc.ParentID; dbID != n.dbDesc.ID && dbID != keys.SystemDatabaseID {
+				return pgerror.Newf(pgcode.FeatureNotSupported,
+					"the view cannot refer to other databases; (see the '%s' cluster setting)",
+					allowCrossDatabaseViewsSetting,
+				)
+			}
+		}
+	}
+
 	// First check the backrefs and see if any of them are temporary.
 	// If so, promote this view to temporary.
 	backRefMutables := make(map[descpb.ID]*tabledesc.Mutable, len(n.planDeps))

pkg/sql/exec_util.go

@@ -121,6 +121,22 @@ var defaultIntSize = func() *settings.IntSetting {
 	return s
 }()
 
+const allowCrossDatabaseFKsSetting = "sql.cross_db_fks.enabled"
+
+var allowCrossDatabaseFKs = settings.RegisterPublicBoolSetting(
+	allowCrossDatabaseFKsSetting,
+	"if true, creating foreign key references across databases is allowed",
+	false,
+)
+
+const allowCrossDatabaseViewsSetting = "sql.cross_db_views.enabled"
+
+var allowCrossDatabaseViews = settings.RegisterPublicBoolSetting(
+	allowCrossDatabaseViewsSetting,
+	"if true, creating views that refer to other databases is allowed",
+	false,
+)
+
 // traceTxnThreshold can be used to log SQL transactions that take
 // longer than duration to complete. For example, traceTxnThreshold=1s
 // will log the trace for any transaction that takes 1s or longer. To

pkg/sql/logictest/testdata/logic_test/drop_database

@@ -2,6 +2,9 @@
 # The mixed configuration is enabled to test backward compatibility for database
 # schema changes.
 
+statement ok
+SET CLUSTER SETTING sql.cross_db_views.enabled = TRUE
+
 statement ok
 CREATE DATABASE "foo-bar"
 

pkg/sql/logictest/testdata/logic_test/fk

@@ -1,5 +1,8 @@
 # LogicTest: !3node-tenant(49854)
 
+statement ok
+SET CLUSTER SETTING sql.cross_db_fks.enabled = TRUE
+
 # Randomize the use of insert fast path.
 # The let statement will also log the value.
 let $enable_insert_fast_path
@@ -3524,3 +3527,75 @@ SELECT
     *
 FROM
     a
+
+# Test that we disallow cross-database foreign key references, depending on the
+# cluster setting.
+
+subtest cross_db_fks
+
+statement ok
+SET CLUSTER SETTING sql.cross_db_fks.enabled = FALSE
+
+statement ok
+CREATE DATABASE db1
+
+statement ok
+CREATE DATABASE db2
+
+statement ok
+USE db1
+
+statement ok
+CREATE TABLE parent (p INT PRIMARY KEY);
+
+statement ok
+CREATE TABLE child1 (c INT PRIMARY KEY, p INT REFERENCES parent(p))
+
+statement ok
+CREATE TABLE child2 (c INT PRIMARY KEY, p INT REFERENCES db1.public.parent(p))
+
+statement ok
+CREATE TABLE db1.public.child3 (c INT PRIMARY KEY, p INT REFERENCES db1.public.parent(p))
+
+statement error foreign references between databases are not allowed
+CREATE TABLE db2.public.child (c INT PRIMARY KEY, p INT REFERENCES parent(p))
+
+statement ok
+CREATE SCHEMA sc1
+
+# Cross-schema references should always be allowed.
+statement ok
+CREATE TABLE db1.sc1.child (c INT PRIMARY KEY, p INT REFERENCES db1.public.parent(p))
+
+statement ok
+USE db2
+
+statement error foreign references between databases are not allowed
+CREATE TABLE child (c INT PRIMARY KEY, p INT REFERENCES db1.public.parent(p))
+
+statement error foreign references between databases are not allowed
+CREATE TABLE db2.public.child (c INT PRIMARY KEY, p INT REFERENCES db1.public.parent(p))
+
+statement ok
+CREATE TABLE child (c INT PRIMARY KEY, p INT)
+
+statement error foreign references between databases are not allowed
+ALTER TABLE child ADD CONSTRAINT fk FOREIGN KEY (p) REFERENCES db1.public.parent(p)
+
+statement ok
+USE db1
+
+statement error foreign references between databases are not allowed
+ALTER TABLE db2.public.child ADD CONSTRAINT fk FOREIGN KEY (p) REFERENCES parent(p)
+
+statement ok
+SET CLUSTER SETTING sql.cross_db_fks.enabled = TRUE
+
+statement ok
+ALTER TABLE db2.public.child ADD CONSTRAINT fk FOREIGN KEY (p) REFERENCES parent(p)
+
+statement ok
+USE db2
+
+statement ok
+CREATE TABLE child2 (c INT PRIMARY KEY, p INT REFERENCES db1.public.parent(p))

pkg/sql/logictest/testdata/logic_test/rename_database

@@ -1,3 +1,6 @@
+statement ok
+SET CLUSTER SETTING sql.cross_db_views.enabled = TRUE
+
 query T
 SHOW DATABASES
 ----

pkg/sql/logictest/testdata/logic_test/rename_database-mixed-20.1-20.2

@@ -1,5 +1,8 @@
 # TestLogic: local-mixed-20.1-20.2
 
+statement ok
+SET CLUSTER SETTING sql.cross_db_views.enabled = TRUE
+
 query T
 SHOW DATABASES
 ----

pkg/sql/logictest/testdata/logic_test/rename_view

@@ -1,3 +1,6 @@
+statement ok
+SET CLUSTER SETTING sql.cross_db_views.enabled = TRUE
+
 statement error pgcode 42P01 relation "foo" does not exist
 ALTER VIEW foo RENAME TO bar
 

pkg/sql/logictest/testdata/logic_test/reparent_database

@@ -1,4 +1,5 @@
 statement ok
+SET CLUSTER SETTING sql.cross_db_fks.enabled = TRUE;
 CREATE DATABASE pgdatabase;
 CREATE TABLE pgdatabase.t1 (x INT PRIMARY KEY);
 CREATE DATABASE pgschema;

pkg/sql/logictest/testdata/logic_test/show_source

@@ -331,7 +331,6 @@ system.public.descriptor  CREATE TABLE public.descriptor (
                           FAMILY fam_2_descriptor (descriptor)
 )
 
-
 query TT colnames
 CREATE VIEW v AS SELECT id FROM system.descriptor; SELECT * FROM [SHOW CREATE VIEW v]
 ----

pkg/sql/logictest/testdata/logic_test/temp_table

@@ -1,3 +1,6 @@
+statement ok
+SET CLUSTER SETTING sql.cross_db_views.enabled = TRUE
+
 statement error temporary tables are only supported experimentally\nHINT:.*46260.*\n.*\n.*SET experimental_enable_temp_tables = 'on'
 CREATE TEMP TABLE a_temp(a INT PRIMARY KEY)
 

pkg/sql/logictest/testdata/logic_test/views

@@ -1,3 +1,6 @@
+statement ok
+SET CLUSTER SETTING sql.cross_db_views.enabled = TRUE
+
 # NOTE: Keep this table at the beginning of the file to ensure that its numeric
 #       reference is 53 (the numeric reference of the first table). If the
 #       numbering scheme in cockroach changes, this test will break.
@@ -762,3 +765,70 @@ CREATE VIEW vregclass AS SELECT 1 FROM (SELECT 1) AS foo WHERE 'tregclass'::regc
 
 statement error pq: cannot drop relation "tregclass" because view "vregclass" depends on it
 DROP TABLE tregclass
+
+# Test that we disallow cross-database references from views, depending on the
+# cluster setting.
+subtest cross_db_views
+
+statement ok
+SET CLUSTER SETTING sql.cross_db_views.enabled = FALSE
+
+statement ok
+CREATE DATABASE db1
+
+statement ok
+CREATE DATABASE db2
+
+statement ok
+USE db1
+
+statement ok
+CREATE TABLE ab (a INT, b INT)
+
+statement ok
+CREATE VIEW v1 AS SELECT a+b FROM ab
+
+statement ok
+CREATE VIEW db1.public.v2 AS SELECT a+b FROM db1.public.ab
+
+statement error the view cannot refer to other databases
+CREATE VIEW db2.v AS SELECT a+b FROM db1.public.ab
+
+# Verify that cross-schema views are allowed.
+statement ok
+CREATE SCHEMA sc2
+
+statement ok
+CREATE VIEW db1.sc2.v AS SELECT a+b FROM db1.public.ab
+
+statement ok
+CREATE TABLE db1.sc2.cd (c INT, d INT)
+
+statement ok
+CREATE VIEW db1.public.v3 AS SELECT a+b+c+d FROM db1.public.ab, db1.sc2.cd
+
+# Verify that views involving system tables are allowed.
+
+statement ok
+CREATE VIEW sys AS SELECT id FROM system.descriptor
+
+statement ok
+CREATE VIEW sys2 AS SELECT id, a+b FROM system.descriptor, ab
+
+statement ok
+USE db2
+
+statement ok
+CREATE TABLE cd (c INT, d INT)
+
+statement error the view cannot refer to other databases
+CREATE VIEW v AS SELECT a+b+c+d FROM cd, db1.public.ab
+
+statement ok
+SET CLUSTER SETTING sql.cross_db_views.enabled = TRUE
+
+statement ok
+CREATE VIEW db2.v1 AS SELECT a+b FROM db1.public.ab
+
+statement ok
+CREATE VIEW db2.v2 AS SELECT a+b+c+d FROM cd, db1.public.ab