From a07c766c54ede6ed16b762af1775282245ea13a4 Mon Sep 17 00:00:00 2001 From: Rafi Shamim Date: Tue, 25 Oct 2022 15:54:35 -0400 Subject: [PATCH] sql: fix pg_table_is_visible to handle indexes This uses the internal executor now to do the introspection using pg_class where everything can be looked up all at once. This should be faster compared to previous releases, since there have been improvements in the internal executor. Release note (bug fix): Fixed the pg_table_is_visible builtin function so it correctly reports visibility of indexes based on the current search_path. --- pkg/sql/faketreeeval/evalctx.go | 14 ------ pkg/sql/importer/import_table_creation.go | 7 --- .../logictest/testdata/logic_test/pg_builtins | 8 ++-- pkg/sql/resolver.go | 48 ------------------- pkg/sql/sem/builtins/pg_builtins.go | 18 +++++-- pkg/sql/sem/eval/deps.go | 6 --- 6 files changed, 19 insertions(+), 82 deletions(-) diff --git a/pkg/sql/faketreeeval/evalctx.go b/pkg/sql/faketreeeval/evalctx.go index 716e98e03212..5cb752344b2a 100644 --- a/pkg/sql/faketreeeval/evalctx.go +++ b/pkg/sql/faketreeeval/evalctx.go @@ -65,13 +65,6 @@ func (so *DummySequenceOperators) SchemaExists( return false, errors.WithStack(errSequenceOperators) } -// IsTableVisible is part of the eval.DatabaseCatalog interface. -func (so *DummySequenceOperators) IsTableVisible( - ctx context.Context, curDB string, searchPath sessiondata.SearchPath, tableID oid.Oid, -) (bool, bool, error) { - return false, false, errors.WithStack(errSequenceOperators) -} - // IsTypeVisible is part of the eval.DatabaseCatalog interface. func (so *DummySequenceOperators) IsTypeVisible( ctx context.Context, curDB string, searchPath sessiondata.SearchPath, typeID oid.Oid, @@ -345,13 +338,6 @@ func (ep *DummyEvalPlanner) SchemaExists(ctx context.Context, dbName, scName str return false, errors.WithStack(errEvalPlanner) } -// IsTableVisible is part of the eval.DatabaseCatalog interface. -func (ep *DummyEvalPlanner) IsTableVisible( - ctx context.Context, curDB string, searchPath sessiondata.SearchPath, tableID oid.Oid, -) (bool, bool, error) { - return false, false, errors.WithStack(errEvalPlanner) -} - // IsTypeVisible is part of the eval.DatabaseCatalog interface. func (ep *DummyEvalPlanner) IsTypeVisible( ctx context.Context, curDB string, searchPath sessiondata.SearchPath, typeID oid.Oid, diff --git a/pkg/sql/importer/import_table_creation.go b/pkg/sql/importer/import_table_creation.go index fa6e561093b2..40da0b8244cf 100644 --- a/pkg/sql/importer/import_table_creation.go +++ b/pkg/sql/importer/import_table_creation.go @@ -298,13 +298,6 @@ func (so *importSequenceOperators) SchemaExists( return false, errSequenceOperators } -// IsTableVisible is part of the eval.DatabaseCatalog interface. -func (so *importSequenceOperators) IsTableVisible( - ctx context.Context, curDB string, searchPath sessiondata.SearchPath, tableID oid.Oid, -) (bool, bool, error) { - return false, false, errors.WithStack(errSequenceOperators) -} - // IsTypeVisible is part of the eval.DatabaseCatalog interface. func (so *importSequenceOperators) IsTypeVisible( ctx context.Context, curDB string, searchPath sessiondata.SearchPath, typeID oid.Oid, diff --git a/pkg/sql/logictest/testdata/logic_test/pg_builtins b/pkg/sql/logictest/testdata/logic_test/pg_builtins index 3702fbc10dd7..0aca9e1cf64a 100644 --- a/pkg/sql/logictest/testdata/logic_test/pg_builtins +++ b/pkg/sql/logictest/testdata/logic_test/pg_builtins @@ -99,10 +99,12 @@ SET DATABASE = test; query TB rowsort SELECT c.relname, pg_table_is_visible(c.oid) FROM pg_class c -WHERE c.relname IN ('is_visible', 'not_visible') +WHERE c.relname IN ('is_visible', 'not_visible', 'is_visible_pkey', 'not_visible_pkey') ---- -is_visible true -not_visible false +is_visible true +is_visible_pkey true +not_visible false +not_visible_pkey false # Looking up a table in a different database should return NULL. query B diff --git a/pkg/sql/resolver.go b/pkg/sql/resolver.go index df4077619406..7671c00f5613 100644 --- a/pkg/sql/resolver.go +++ b/pkg/sql/resolver.go @@ -135,54 +135,6 @@ func (p *planner) SchemaExists(ctx context.Context, dbName, scName string) (foun return found, err } -// IsTableVisible is part of the eval.DatabaseCatalog interface. -func (p *planner) IsTableVisible( - ctx context.Context, curDB string, searchPath sessiondata.SearchPath, tableID oid.Oid, -) (isVisible, exists bool, err error) { - tableDesc, err := p.LookupTableByID(ctx, descpb.ID(tableID)) - if err != nil { - // If a "not found" error happened here, we return "not exists" rather than - // the error. - if errors.Is(err, catalog.ErrDescriptorNotFound) || - errors.Is(err, catalog.ErrDescriptorDropped) || - pgerror.GetPGCode(err) == pgcode.UndefinedTable || - pgerror.GetPGCode(err) == pgcode.UndefinedObject { - return false, false, nil //nolint:returnerrcheck - } - return false, false, err - } - schemaID := tableDesc.GetParentSchemaID() - schemaDesc, err := p.Descriptors().GetImmutableSchemaByID(ctx, p.Txn(), schemaID, - tree.SchemaLookupFlags{ - Required: true, - AvoidLeased: p.skipDescriptorCache}) - if err != nil { - return false, false, err - } - if schemaDesc.SchemaKind() != catalog.SchemaVirtual { - dbID := tableDesc.GetParentID() - _, dbDesc, err := p.Descriptors().GetImmutableDatabaseByID(ctx, p.Txn(), dbID, - tree.DatabaseLookupFlags{ - Required: true, - AvoidLeased: p.skipDescriptorCache}) - if err != nil { - return false, false, err - } - if dbDesc.GetName() != curDB { - // If the table is in a different database, then it's considered to be - // "not existing" instead of just "not visible"; this matches PostgreSQL. - return false, false, nil - } - } - iter := searchPath.Iter() - for scName, ok := iter.Next(); ok; scName, ok = iter.Next() { - if schemaDesc.GetName() == scName { - return true, true, nil - } - } - return false, true, nil -} - // IsTypeVisible is part of the eval.DatabaseCatalog interface. func (p *planner) IsTypeVisible( ctx context.Context, curDB string, searchPath sessiondata.SearchPath, typeID oid.Oid, diff --git a/pkg/sql/sem/builtins/pg_builtins.go b/pkg/sql/sem/builtins/pg_builtins.go index 398129c84dd4..3653fe303f31 100644 --- a/pkg/sql/sem/builtins/pg_builtins.go +++ b/pkg/sql/sem/builtins/pg_builtins.go @@ -1216,16 +1216,26 @@ SELECT description ReturnType: tree.FixedReturnType(types.Bool), Fn: func(ctx context.Context, evalCtx *eval.Context, args tree.Datums) (tree.Datum, error) { oidArg := tree.MustBeDOid(args[0]) - isVisible, exists, err := evalCtx.Planner.IsTableVisible( - ctx, evalCtx.SessionData().Database, evalCtx.SessionData().SearchPath, oidArg.Oid, + row, err := evalCtx.Planner.QueryRowEx( + ctx, "pg_table_is_visible", + sessiondata.NoSessionDataOverride, + "SELECT n.nspname from pg_class c JOIN pg_namespace n ON c.relnamespace = n.oid WHERE c.oid=$1 LIMIT 1", + oidArg.Oid, ) if err != nil { return nil, err } - if !exists { + if row == nil { return tree.DNull, nil } - return tree.MakeDBool(tree.DBool(isVisible)), nil + foundSchemaName := string(tree.MustBeDString(row[0])) + iter := evalCtx.SessionData().SearchPath.Iter() + for scName, ok := iter.Next(); ok; scName, ok = iter.Next() { + if foundSchemaName == scName { + return tree.DBoolTrue, nil + } + } + return tree.DBoolFalse, nil }, Info: "Returns whether the table with the given OID belongs to one of the schemas on the search path.", Volatility: volatility.Stable, diff --git a/pkg/sql/sem/eval/deps.go b/pkg/sql/sem/eval/deps.go index 136a1003c7cd..232f585d8351 100644 --- a/pkg/sql/sem/eval/deps.go +++ b/pkg/sql/sem/eval/deps.go @@ -66,12 +66,6 @@ type DatabaseCatalog interface { // whether it exists. SchemaExists(ctx context.Context, dbName, scName string) (found bool, err error) - // IsTableVisible checks if the table with the given ID belongs to a schema - // on the given sessiondata.SearchPath. - IsTableVisible( - ctx context.Context, curDB string, searchPath sessiondata.SearchPath, tableID oid.Oid, - ) (isVisible bool, exists bool, err error) - // IsTypeVisible checks if the type with the given ID belongs to a schema // on the given sessiondata.SearchPath. IsTypeVisible(