From f4e3749656a6c156abfc6644dc891c68c496fe1e Mon Sep 17 00:00:00 2001 From: Jordan Lewis Date: Thu, 29 Dec 2022 12:44:29 -0500 Subject: [PATCH 1/3] sql: hide x-db objs in type,func virtual indexes Previously, the pg_proc and pg_type virtual indexes would show cross-db objects, which is not supposed to happen. This is now corrected. Release note (bug fix): the pg_proc and pg_type virtual oid indexes no longer incorrectly show cross-db objects. This is unlikely to affect real-world use cases. --- .../typedesc/table_implicit_record_type.go | 3 +-- pkg/sql/pg_catalog.go | 18 ++++++++++++++++++ 2 files changed, 19 insertions(+), 2 deletions(-) diff --git a/pkg/sql/catalog/typedesc/table_implicit_record_type.go b/pkg/sql/catalog/typedesc/table_implicit_record_type.go index 1ceb12905567..a90379dfd8e3 100644 --- a/pkg/sql/catalog/typedesc/table_implicit_record_type.go +++ b/pkg/sql/catalog/typedesc/table_implicit_record_type.go @@ -118,8 +118,7 @@ func (v TableImplicitRecordType) Adding() bool { // Dropped implements the Descriptor interface. func (v TableImplicitRecordType) Dropped() bool { - v.panicNotSupported("Dropped") - return false + return v.desc.Dropped() } // Offline implements the Descriptor interface. diff --git a/pkg/sql/pg_catalog.go b/pkg/sql/pg_catalog.go index adf608b9848b..fed2279a868b 100644 --- a/pkg/sql/pg_catalog.go +++ b/pkg/sql/pg_catalog.go @@ -2618,6 +2618,9 @@ https://www.postgresql.org/docs/9.5/catalog-pg-proc.html`, if err != nil { return false, err } + if fnDesc.Dropped() || fnDesc.GetParentID() != dbContext.GetID() { + return false, nil + } err = addPgProcUDFRow(h, scDesc, fnDesc, addRow) if err != nil { @@ -3321,6 +3324,10 @@ https://www.postgresql.org/docs/9.5/catalog-pg-type.html`, return false, err } + if typDesc.Dropped() { + return false, nil + } + // It's an entry for the implicit record type created on behalf of each // table. We have special logic for this case. if typDesc.GetKind() == descpb.TypeDescriptor_TABLE_IMPLICIT_RECORD_TYPE { @@ -3333,6 +3340,12 @@ https://www.postgresql.org/docs/9.5/catalog-pg-type.html`, } return false, err } + if !table.IsVirtualTable() && table.GetParentID() != db.GetID() { + // If we're looking an implicit record type for a virtual table, we + // always return the row regardless of the parent DB. But for real + // tables, we only return a row if we're in the same DB as the table. + return false, nil + } if err := addPGTypeRowForTable( ctx, p, @@ -3347,6 +3360,11 @@ https://www.postgresql.org/docs/9.5/catalog-pg-type.html`, return true, nil } + if typDesc.GetParentID() != db.GetID() { + // Don't return types that aren't in our database. + return false, nil + } + nspOid = schemaOid(sc.GetID()) typ, err = typedesc.HydratedTFromDesc(ctx, tree.NewUnqualifiedTypeName(typDesc.GetName()), typDesc, p) if err != nil { From acf5006af145e338178d833f361f20a9ced78a5e Mon Sep 17 00:00:00 2001 From: Jordan Lewis Date: Tue, 27 Dec 2022 15:34:27 -0500 Subject: [PATCH 2/3] builtins: implement pg_*_is_visible as UDF Now that we support UDFs, we can implement builtin functions as "virtual UDFs", that are defined in the builtins map. The builtin appears to be about twice as fast with this method, which is nice because it gets used a lot in ORM introspection queries. Epic: None Release note (performance improvement): improve the performance of pg_{function,table,type}_is_visible Co-authored-by: rafiss --- pkg/sql/pg_catalog.go | 3 +- pkg/sql/sem/builtins/pg_builtins.go | 72 ++++++++--------------------- 2 files changed, 20 insertions(+), 55 deletions(-) diff --git a/pkg/sql/pg_catalog.go b/pkg/sql/pg_catalog.go index fed2279a868b..aaf7339b1025 100644 --- a/pkg/sql/pg_catalog.go +++ b/pkg/sql/pg_catalog.go @@ -31,6 +31,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/sql/catalog/catprivilege" "github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb" "github.com/cockroachdb/cockroach/pkg/sql/catalog/descs" + "github.com/cockroachdb/cockroach/pkg/sql/catalog/funcdesc" "github.com/cockroachdb/cockroach/pkg/sql/catalog/schemadesc" "github.com/cockroachdb/cockroach/pkg/sql/catalog/schemaexpr" "github.com/cockroachdb/cockroach/pkg/sql/catalog/tabledesc" @@ -2601,7 +2602,7 @@ https://www.postgresql.org/docs/9.5/catalog-pg-proc.html`, return false, err } - if overload.IsUDF { + if funcdesc.IsOIDUserDefinedFunc(ooid) { fnDesc, err := p.Descriptors().GetImmutableFunctionByID( ctx, p.Txn(), descpb.ID(overload.Oid), tree.ObjectLookupFlags{ diff --git a/pkg/sql/sem/builtins/pg_builtins.go b/pkg/sql/sem/builtins/pg_builtins.go index 2e3ee5325d43..895ec4a62a40 100644 --- a/pkg/sql/sem/builtins/pg_builtins.go +++ b/pkg/sql/sem/builtins/pg_builtins.go @@ -1226,26 +1226,14 @@ SELECT description // https://www.postgresql.org/docs/9.6/static/functions-info.html "pg_function_is_visible": makeBuiltin(defProps(), tree.Overload{ + IsUDF: true, Types: tree.ParamTypes{{Name: "oid", Typ: types.Oid}}, ReturnType: tree.FixedReturnType(types.Bool), - Fn: func(ctx context.Context, evalCtx *eval.Context, args tree.Datums) (tree.Datum, error) { - oidArg := tree.MustBeDOid(args[0]) - row, err := evalCtx.Planner.QueryRowEx( - ctx, "pg_function_is_visible", - sessiondata.NoSessionDataOverride, - "SELECT n.nspname from pg_proc p JOIN pg_namespace n ON p.pronamespace = n.oid WHERE p.oid=$1 LIMIT 1", - oidArg, - ) - if err != nil { - return nil, err - } - if row == nil { - return tree.DNull, nil - } - foundSchemaName := string(tree.MustBeDString(row[0])) - isVisible := evalCtx.SessionData().SearchPath.Contains(foundSchemaName, true /* includeImplicit */) - return tree.MakeDBool(tree.DBool(isVisible)), nil - }, + Body: `SELECT n.nspname = any current_schemas(true) + FROM pg_proc p + INNER LOOKUP JOIN pg_namespace n + ON p.pronamespace = n.oid + WHERE p.oid=$1 LIMIT 1`, Info: "Returns whether the function with the given OID belongs to one of the schemas on the search path.", Volatility: volatility.Stable, }, @@ -1255,26 +1243,14 @@ SELECT description // https://www.postgresql.org/docs/9.6/static/functions-info.html "pg_table_is_visible": makeBuiltin(defProps(), tree.Overload{ + IsUDF: true, Types: tree.ParamTypes{{Name: "oid", Typ: types.Oid}}, ReturnType: tree.FixedReturnType(types.Bool), - Fn: func(ctx context.Context, evalCtx *eval.Context, args tree.Datums) (tree.Datum, error) { - oidArg := tree.MustBeDOid(args[0]) - 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, - ) - if err != nil { - return nil, err - } - if row == nil { - return tree.DNull, nil - } - foundSchemaName := string(tree.MustBeDString(row[0])) - isVisible := evalCtx.SessionData().SearchPath.Contains(foundSchemaName, true /* includeImplicit */) - return tree.MakeDBool(tree.DBool(isVisible)), nil - }, + Body: `SELECT n.nspname = any current_schemas(true) + FROM pg_class c + INNER LOOKUP JOIN pg_namespace n + ON c.relnamespace = n.oid + WHERE c.oid=$1 LIMIT 1`, Info: "Returns whether the table with the given OID belongs to one of the schemas on the search path.", Volatility: volatility.Stable, }, @@ -1288,26 +1264,14 @@ SELECT description // https://www.postgresql.org/docs/9.6/static/functions-info.html "pg_type_is_visible": makeBuiltin(defProps(), tree.Overload{ + IsUDF: true, Types: tree.ParamTypes{{Name: "oid", Typ: types.Oid}}, ReturnType: tree.FixedReturnType(types.Bool), - Fn: func(ctx context.Context, evalCtx *eval.Context, args tree.Datums) (tree.Datum, error) { - oidArg := tree.MustBeDOid(args[0]) - row, err := evalCtx.Planner.QueryRowEx( - ctx, "pg_type_is_visible", - sessiondata.NoSessionDataOverride, - "SELECT n.nspname from pg_type t JOIN pg_namespace n ON t.typnamespace = n.oid WHERE t.oid=$1 LIMIT 1", - oidArg, - ) - if err != nil { - return nil, err - } - if row == nil { - return tree.DNull, nil - } - foundSchemaName := string(tree.MustBeDString(row[0])) - isVisible := evalCtx.SessionData().SearchPath.Contains(foundSchemaName, true /* includeImplicit */) - return tree.MakeDBool(tree.DBool(isVisible)), nil - }, + Body: `SELECT n.nspname = any current_schemas(true) + FROM pg_type t + INNER LOOKUP JOIN pg_namespace n + ON t.typnamespace = n.oid + WHERE t.oid=$1 LIMIT 1`, Info: "Returns whether the type with the given OID belongs to one of the schemas on the search path.", Volatility: volatility.Stable, }, From d363b40ffdce40be25efb12c3e1771b9addc1f86 Mon Sep 17 00:00:00 2001 From: Jordan Lewis Date: Wed, 28 Dec 2022 16:23:44 -0500 Subject: [PATCH 3/3] builtins: implement _pg_index_position as UDF Release note: None --- pkg/sql/sem/builtins/pg_builtins.go | 35 +++++------------------------ 1 file changed, 6 insertions(+), 29 deletions(-) diff --git a/pkg/sql/sem/builtins/pg_builtins.go b/pkg/sql/sem/builtins/pg_builtins.go index 895ec4a62a40..0370a95d350f 100644 --- a/pkg/sql/sem/builtins/pg_builtins.go +++ b/pkg/sql/sem/builtins/pg_builtins.go @@ -2056,43 +2056,20 @@ SELECT description // _pg_index_position return the column's position in the index // (or NULL if not there). // - // NOTE: this could be defined as a user-defined function, like - // it is in Postgres: + // NOTE: this is defined as a UDF, same as in Postgres: // https://github.com/postgres/postgres/blob/master/src/backend/catalog/information_schema.sql - // - // CREATE FUNCTION _pg_index_position(oid, smallint) RETURNS int - // LANGUAGE sql STRICT STABLE - // BEGIN ATOMIC - // SELECT (ss.a).n FROM - // (SELECT information_schema._pg_expandarray(indkey) AS a - // FROM pg_catalog.pg_index WHERE indexrelid = $1) ss - // WHERE (ss.a).x = $2; - // END; - // "information_schema._pg_index_position": makeBuiltin(defProps(), tree.Overload{ + IsUDF: true, Types: tree.ParamTypes{ {Name: "oid", Typ: types.Oid}, {Name: "col", Typ: types.Int2}, }, ReturnType: tree.FixedReturnType(types.Int), - Fn: func(ctx context.Context, evalCtx *eval.Context, args tree.Datums) (tree.Datum, error) { - r, err := evalCtx.Planner.QueryRowEx( - ctx, "information_schema._pg_index_position", - sessiondata.NoSessionDataOverride, - `SELECT (ss.a).n FROM - (SELECT information_schema._pg_expandarray(indkey) AS a - FROM pg_catalog.pg_index WHERE indexrelid = $1) ss - WHERE (ss.a).x = $2`, - args[0], args[1]) - if err != nil { - return nil, err - } - if len(r) == 0 { - return tree.DNull, nil - } - return r[0], nil - }, + Body: `SELECT (ss.a).n FROM + (SELECT information_schema._pg_expandarray(indkey) AS a + FROM pg_catalog.pg_index WHERE indexrelid = $1) ss + WHERE (ss.a).x = $2`, Info: notUsableInfo, Volatility: volatility.Stable, },