From cfb9f42e19f6d0b3db5410db2c673304f730ba98 Mon Sep 17 00:00:00 2001 From: rharding6373 Date: Tue, 21 Mar 2023 09:17:54 -0700 Subject: [PATCH] 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: chengxiong@cockroachlabs.com 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. --- pkg/sql/BUILD.bazel | 1 + pkg/sql/drop_database.go | 18 ++++++++-- pkg/sql/drop_view.go | 18 ++++++++-- pkg/sql/logictest/testdata/logic_test/views | 40 +++++++++++++++++++-- pkg/sql/opt/optbuilder/builder.go | 4 +++ pkg/sql/opt/optbuilder/create_view.go | 5 +++ pkg/sql/opt/optbuilder/select.go | 11 ++++++ pkg/sql/planner.go | 4 +++ pkg/sql/scanner/BUILD.bazel | 5 ++- 9 files changed, 97 insertions(+), 9 deletions(-) diff --git a/pkg/sql/BUILD.bazel b/pkg/sql/BUILD.bazel index 17624fffb6fd..2d4e88bd6061 100644 --- 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 index 9d2104d145d6..b7e0cfe756d6 100644 --- 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 index 45d49d713c66..243e2ec6a183 100644 --- 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 index 964907adb9e7..efce7b70f31f 100644 --- 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 index 870eeea8b71b..834d35546446 100644 --- 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 index a190f886977f..9eb2cc10bebb 100644 --- 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 index 58280314bcda..7e868ca52d07 100644 --- 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 index 4ae0c93a7e47..296e29f3d7c4 100644 --- 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 index c74c89a8952c..b05d4ffe26e8 100644 --- 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",