sql: fix circular dependencies in views

This change fixes node crashes that could happen due to stack overflow if views were created with circular dependencies. It also fixes a few places in the schema changer where there were problems dropping views with circular dependencies. Fixes: #98999 Epic: none Co-authored-by: [email protected] Release note (bug fix): If views are created with circular dependencies, CRDB returns the error "cyclic view dependency for relation" instead of crashing the node. This bug is present since at least 21.1. Release justification: Fixes a bug that can cause the gateway node to crash if there are self-referencing views.
cockroachdb · Mar 30, 2023 · cfb9f42 · cfb9f42
1 parent b77cb3e
commit cfb9f42
Show file tree

Hide file tree

Showing 9 changed files with 97 additions and 9 deletions.
diff --git a/pkg/sql/BUILD.bazel b/pkg/sql/BUILD.bazel
@@ -375,6 +375,7 @@ go_library(
         "//pkg/sql/schemachanger/scrun",
         "//pkg/sql/scrub",
         "//pkg/sql/sem/builtins",
+        "//pkg/sql/sem/catid",
         "//pkg/sql/sem/transform",
         "//pkg/sql/sem/tree",
         "//pkg/sql/sem/tree/treebin",

diff --git a/pkg/sql/drop_database.go b/pkg/sql/drop_database.go
@@ -234,8 +234,9 @@ func (p *planner) accumulateAllObjectsToDelete(
 	ctx context.Context, objects []toDelete,
 ) ([]*tabledesc.Mutable, map[descpb.ID]*tabledesc.Mutable, error) {
 	implicitDeleteObjects := make(map[descpb.ID]*tabledesc.Mutable)
+	visited := make(map[descpb.ID]struct{})
 	for _, toDel := range objects {
-		err := p.accumulateCascadingViews(ctx, implicitDeleteObjects, toDel.desc)
+		err := p.accumulateCascadingViews(ctx, implicitDeleteObjects, visited, toDel.desc)
 		if err != nil {
 			return nil, nil, err
 		}
@@ -292,8 +293,12 @@ func (p *planner) accumulateOwnedSequences(
 // references, which means this list can't be constructed by simply scanning
 // the namespace table.
 func (p *planner) accumulateCascadingViews(
-	ctx context.Context, dependentObjects map[descpb.ID]*tabledesc.Mutable, desc *tabledesc.Mutable,
+	ctx context.Context,
+	dependentObjects map[descpb.ID]*tabledesc.Mutable,
+	visited map[descpb.ID]struct{},
+	desc *tabledesc.Mutable,
 ) error {
+	visited[desc.ID] = struct{}{}
 	for _, ref := range desc.DependedOnBy {
 		dependentDesc, err := p.Descriptors().GetMutableTableVersionByID(ctx, ref.ID, p.txn)
 		if err != nil {
@@ -302,8 +307,15 @@ func (p *planner) accumulateCascadingViews(
 		if !dependentDesc.IsView() {
 			continue
 		}
+
+		_, seen := visited[ref.ID]
+		if dependentObjects[ref.ID] == dependentDesc || seen {
+			// This view's dependencies are already added.
+			continue
+		}
 		dependentObjects[ref.ID] = dependentDesc
-		if err := p.accumulateCascadingViews(ctx, dependentObjects, dependentDesc); err != nil {
+
+		if err := p.accumulateCascadingViews(ctx, dependentObjects, visited, dependentDesc); err != nil {
 			return err
 		}
 	}

diff --git a/pkg/sql/drop_view.go b/pkg/sql/drop_view.go
@@ -18,6 +18,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/tabledesc"
 	"github.com/cockroachdb/cockroach/pkg/sql/privilege"
+	"github.com/cockroachdb/cockroach/pkg/sql/sem/catid"
 	"github.com/cockroachdb/cockroach/pkg/sql/sem/tree"
 	"github.com/cockroachdb/cockroach/pkg/sql/sqlerrors"
 	"github.com/cockroachdb/cockroach/pkg/util/log"
@@ -32,8 +33,9 @@ type dropViewNode struct {
 
 // DropView drops a view.
 // Privileges: DROP on view.
-//   Notes: postgres allows only the view owner to DROP a view.
-//          mysql requires the DROP privilege on the view.
+//
+//	Notes: postgres allows only the view owner to DROP a view.
+//	       mysql requires the DROP privilege on the view.
 func (p *planner) DropView(ctx context.Context, n *tree.DropView) (planNode, error) {
 	if err := checkSchemaChangeEnabled(
 		ctx,
@@ -138,6 +140,18 @@ func (p *planner) canRemoveDependentView(
 	ref descpb.TableDescriptor_Reference,
 	behavior tree.DropBehavior,
 ) error {
+	if p.trackDependency == nil {
+		p.trackDependency = make(map[catid.DescID]bool)
+	}
+	if p.trackDependency[ref.ID] {
+		// This table's dependencies are already tracked.
+		return nil
+	}
+	p.trackDependency[ref.ID] = true
+	defer func() {
+		p.trackDependency[ref.ID] = false
+	}()
+
 	return p.canRemoveDependentViewGeneric(ctx, string(from.DescriptorType()), from.Name, from.ParentID, ref, behavior)
 }
 

diff --git a/pkg/sql/logictest/testdata/logic_test/views b/pkg/sql/logictest/testdata/logic_test/views
@@ -19,12 +19,12 @@ CREATE VIEW v1 AS SELECT a, b FROM t
 statement error pgcode 42P07 relation \"test.public.t\" already exists
 CREATE VIEW t AS SELECT a, b FROM t
 
-# view statement ignored if other way around.
 statement ok
-CREATE VIEW IF NOT EXISTS v1 AS SELECT b, a FROM v1
+CREATE VIEW IF NOT EXISTS v2 (x, y) AS SELECT a, b FROM t
 
+# view statement ignored if other way around.
 statement ok
-CREATE VIEW IF NOT EXISTS v2 (x, y) AS SELECT a, b FROM t
+CREATE VIEW IF NOT EXISTS v2 AS SELECT b, a FROM v1
 
 statement error pgcode 42601 CREATE VIEW specifies 1 column name, but data source has 2 columns
 CREATE VIEW v3 (x) AS SELECT a, b FROM t
@@ -1260,3 +1260,37 @@ li  CREATE MATERIALIZED VIEW public.li (
       test_t1,
       rowid
     ) AS (SELECT 1 AS test_t1)
+
+subtest circular_dependency
+
+statement ok
+CREATE TABLE cd_t (a INT PRIMARY KEY, b INT);
+
+statement ok
+CREATE VIEW cd_v1 AS SELECT a, b FROM cd_t;
+
+statement ok
+CREATE VIEW cd_v2 AS SELECT a, b FROM cd_v1;
+
+# Note: Creating a circular dependency in views does not result in an error in
+# postgres. In postgres, we only encounter errors during queries on the views.
+statement error pgcode 42P17 pq: cyclic view dependency for relation db2.public.cd_v1
+CREATE OR REPLACE VIEW cd_v1 AS SELECT a, b FROM cd_v2;
+
+statement ok
+CREATE VIEW cd_v3 AS SELECT a, b FROM cd_v2;
+
+statement ok
+SELECT * FROM cd_v3;
+
+statement error pgcode 42P17 pq: cyclic view dependency for relation db2.public.cd_v1
+CREATE OR REPLACE VIEW cd_v1 AS SELECT a, b FROM cd_v3;
+
+statement ok
+SELECT * FROM cd_v3;
+
+statement error pq: cannot drop relation "cd_v1" because view "cd_v2" depends on it
+DROP VIEW cd_v1;
+
+statement ok
+DROP VIEW cd_v1 CASCADE;
diff --git a/pkg/sql/opt/optbuilder/builder.go b/pkg/sql/opt/optbuilder/builder.go
@@ -110,6 +110,10 @@ type Builder struct {
 	// are referenced multiple times in the same query.
 	views map[cat.View]*tree.Select
 
+	// sourceViews contains a map with all the views in the current data source
+	// chain. It is used to detect circular dependencies.
+	sourceViews map[string]struct{}
+
 	// subquery contains a pointer to the subquery which is currently being built
 	// (if any).
 	subquery *subquery

diff --git a/pkg/sql/opt/optbuilder/create_view.go b/pkg/sql/opt/optbuilder/create_view.go
@@ -31,12 +31,17 @@ func (b *Builder) buildCreateView(cv *tree.CreateView, inScope *scope) (outScope
 	b.insideViewDef = true
 	b.trackViewDeps = true
 	b.qualifyDataSourceNamesInAST = true
+	if b.sourceViews == nil {
+		b.sourceViews = make(map[string]struct{})
+	}
+	b.sourceViews[viewName.FQString()] = struct{}{}
 	defer func() {
 		b.insideViewDef = false
 		b.trackViewDeps = false
 		b.viewDeps = nil
 		b.viewTypeDeps = util.FastIntSet{}
 		b.qualifyDataSourceNamesInAST = false
+		delete(b.sourceViews, viewName.FQString())
 	}()
 
 	defScope := b.buildStmtAtRoot(cv.AsSource, nil /* desiredTypes */)

diff --git a/pkg/sql/opt/optbuilder/select.go b/pkg/sql/opt/optbuilder/select.go
@@ -244,6 +244,17 @@ func (b *Builder) buildDataSource(
 func (b *Builder) buildView(
 	view cat.View, viewName *tree.TableName, locking lockingSpec, inScope *scope,
 ) (outScope *scope) {
+	if b.sourceViews == nil {
+		b.sourceViews = make(map[string]struct{})
+	}
+	// Check whether there is a circular dependency between views.
+	if _, ok := b.sourceViews[viewName.FQString()]; ok {
+		panic(pgerror.Newf(pgcode.InvalidObjectDefinition, "cyclic view dependency for relation %s", viewName))
+	}
+	b.sourceViews[viewName.FQString()] = struct{}{}
+	defer func() {
+		delete(b.sourceViews, viewName.FQString())
+	}()
 	// Cache the AST so that multiple references won't need to reparse.
 	if b.views == nil {
 		b.views = make(map[cat.View]*tree.Select)

diff --git a/pkg/sql/planner.go b/pkg/sql/planner.go
@@ -31,6 +31,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/sql/opt/exec"
 	"github.com/cockroachdb/cockroach/pkg/sql/parser"
 	"github.com/cockroachdb/cockroach/pkg/sql/querycache"
+	"github.com/cockroachdb/cockroach/pkg/sql/sem/catid"
 	"github.com/cockroachdb/cockroach/pkg/sql/sem/transform"
 	"github.com/cockroachdb/cockroach/pkg/sql/sem/tree"
 	"github.com/cockroachdb/cockroach/pkg/sql/sessiondata"
@@ -248,6 +249,9 @@ type planner struct {
 	// the type resolution steps will disallow resolution of types that have a
 	// parentID != contextDatabaseID when it is set.
 	contextDatabaseID descpb.ID
+
+	// trackDependency is used to track circular dependencies when dropping views.
+	trackDependency map[catid.DescID]bool
 }
 
 func (evalCtx *extendedEvalContext) setSessionID(sessionID ClusterWideID) {

diff --git a/pkg/sql/scanner/BUILD.bazel b/pkg/sql/scanner/BUILD.bazel
@@ -10,7 +10,10 @@ go_library(
 
 go_test(
     name = "scanner_test",
-    srcs = ["scan_test.go"],
+    srcs = [
+        "scan_test.go",
+        "token_names_test.go",
+    ],
     embed = [":scanner"],
     deps = [
         "//pkg/sql/lexbase",