release-23.1: sql: fix circular dependencies in views

pkg/sql/drop_database.go

@@ -253,8 +253,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
 		}
@@ -311,8 +312,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 {
 		desc, err := p.Descriptors().MutableByID(p.txn).Desc(ctx, ref.ID)
 		if err != nil {
@@ -327,8 +332,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
 		}
 	}

pkg/sql/drop_view.go

@@ -21,6 +21,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/tabledesc"
 	"github.com/cockroachdb/cockroach/pkg/sql/privilege"
 	"github.com/cockroachdb/cockroach/pkg/sql/schemachanger/scerrors"
+	"github.com/cockroachdb/cockroach/pkg/sql/sem/catid"
 	"github.com/cockroachdb/cockroach/pkg/sql/sem/tree"
 	"github.com/cockroachdb/cockroach/pkg/sql/sqltelemetry"
 	"github.com/cockroachdb/cockroach/pkg/util/log"
@@ -170,6 +171,18 @@ func (p *planner) canRemoveDependentFromTable(
 	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.canRemoveDependent(ctx, string(from.DescriptorType()), from.Name, from.ParentID, ref, behavior)
 }
 

pkg/sql/logictest/testdata/logic_test/views

@@ -22,12 +22,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
@@ -1813,3 +1813,37 @@ SELECT * FROM v;
 
 statement ok
 SET DATABASE = test;
+
+subtest circular_dependency
+
+statement ok
+CREATE TABLE t (a INT PRIMARY KEY, b INT);
+
+statement ok
+CREATE VIEW cd_v1 AS SELECT a, b FROM 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 test.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 test.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;

pkg/sql/opt/optbuilder/builder.go

@@ -113,6 +113,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

pkg/sql/opt/optbuilder/create_view.go

@@ -36,12 +36,17 @@ func (b *Builder) buildCreateView(cv *tree.CreateView, inScope *scope) (outScope
 	b.insideViewDef = true
 	b.trackSchemaDeps = 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.trackSchemaDeps = false
 		b.schemaDeps = nil
 		b.schemaTypeDeps = intsets.Fast{}
 		b.qualifyDataSourceNamesInAST = false
+		delete(b.sourceViews, viewName.FQString())
 
 		b.semaCtx.FunctionResolver = preFuncResolver
 		switch recErr := recover().(type) {

pkg/sql/opt/optbuilder/select.go

@@ -280,6 +280,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)

pkg/sql/planner.go

@@ -40,6 +40,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/sql/parser"
 	"github.com/cockroachdb/cockroach/pkg/sql/privilege"
 	"github.com/cockroachdb/cockroach/pkg/sql/querycache"
+	"github.com/cockroachdb/cockroach/pkg/sql/sem/catid"
 	"github.com/cockroachdb/cockroach/pkg/sql/sem/eval"
 	"github.com/cockroachdb/cockroach/pkg/sql/sem/transform"
 	"github.com/cockroachdb/cockroach/pkg/sql/sem/tree"
@@ -262,6 +263,9 @@ type planner struct {
 
 	// evalCatalogBuiltins is used as part of the eval.Context.
 	evalCatalogBuiltins evalcatalog.Builtins
+
+	// trackDependency is used to track circular dependencies when dropping views.
+	trackDependency map[catid.DescID]bool
 }
 
 // hasFlowForPausablePortal returns true if the planner is for re-executing a