From 3b66cdb65efab3589c186a09391d585357939573 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. There's an increase in number of round trips, but it's a constant factor. 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. --- .../testdata/benchmark_expectations | 6 +-- 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 | 50 ------------------- pkg/sql/sem/builtins/pg_builtins.go | 18 +++++-- pkg/sql/sem/eval/deps.go | 6 --- 7 files changed, 22 insertions(+), 87 deletions(-) diff --git a/pkg/bench/rttanalysis/testdata/benchmark_expectations b/pkg/bench/rttanalysis/testdata/benchmark_expectations index 82a1c76e23e8..8f6a1e46e4eb 100644 --- a/pkg/bench/rttanalysis/testdata/benchmark_expectations +++ b/pkg/bench/rttanalysis/testdata/benchmark_expectations @@ -53,9 +53,9 @@ exp,benchmark 11,GrantRole/grant_1_role 15,GrantRole/grant_2_roles 3,ORMQueries/activerecord_type_introspection_query -3,ORMQueries/django_table_introspection_1_table -3,ORMQueries/django_table_introspection_4_tables -3,ORMQueries/django_table_introspection_8_tables +6,ORMQueries/django_table_introspection_1_table +6,ORMQueries/django_table_introspection_4_tables +6,ORMQueries/django_table_introspection_8_tables 2,ORMQueries/has_column_privilege_using_attnum 2,ORMQueries/has_column_privilege_using_column_name 1,ORMQueries/has_schema_privilege_1 diff --git a/pkg/sql/faketreeeval/evalctx.go b/pkg/sql/faketreeeval/evalctx.go index 2b5988afc7d1..6036a582b6d3 100644 --- a/pkg/sql/faketreeeval/evalctx.go +++ b/pkg/sql/faketreeeval/evalctx.go @@ -68,13 +68,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, @@ -372,13 +365,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 be7b2d9b0e4b..54b63352d5c7 100644 --- a/pkg/sql/importer/import_table_creation.go +++ b/pkg/sql/importer/import_table_creation.go @@ -303,13 +303,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 c6d141722050..1608bac5b152 100644 --- a/pkg/sql/resolver.go +++ b/pkg/sql/resolver.go @@ -138,56 +138,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) { - dbDesc, err := p.Descriptors().GetImmutableDatabaseByName(ctx, p.Txn(), curDB, - tree.DatabaseLookupFlags{ - Required: true, - AvoidLeased: p.skipDescriptorCache, - }) - if err != nil { - return false, false, err - } - // It is critical that we set ParentID on the flags in order to ensure that - // we do not do a very expensive, and ultimately fruitless lookup for an - // OID which definitely does not exist. Only OIDs corresponding to relations - // in the current database are relevant for this function. If we have already - // fetched all the tables in the current database, then we can use that - // fact to avoid a KV lookup. The descs layer relies on our setting this - // field in the flags to avoid that lookup. - flags := p.ObjectLookupFlags(true /* required */, false /* requireMutable */) - flags.ParentID = dbDesc.GetID() - tableDesc, err := p.Descriptors().GetImmutableTableByID(ctx, p.Txn(), descpb.ID(tableID), flags) - 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 - } - 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 96065d1077c1..3e0aad341e19 100644 --- a/pkg/sql/sem/builtins/pg_builtins.go +++ b/pkg/sql/sem/builtins/pg_builtins.go @@ -1217,16 +1217,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 INNER LOOKUP 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 2bafce989471..cb3ae873830f 100644 --- a/pkg/sql/sem/eval/deps.go +++ b/pkg/sql/sem/eval/deps.go @@ -65,12 +65,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(