Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

sql: implement pg_my_temp_schema and pg_is_other_temp_schema #69909

Merged

Conversation

nvanbenschoten
Copy link
Member

Needed for #69010.

This PR adds a real implementation for the pg_my_temp_schema builtin
function. pg_my_temp_schema returns the OID of session's temporary schema, or 0
if the session has not yet created a temporary schema.

The PR then implements the pg_is_other_temp_schema builtin function.
pg_is_other_temp_schema returns true if the given OID is the OID of another
session's temporary schema. This can be useful, for example, to exclude other
sessions' temporary tables from a catalog display.

Release note (sql change): The pg_my_temp_schema builtin function now properly
returns the OID of the active session's temporary schema, if one exists.

Release note (sql change): The pg_is_other_temp_schema builtin function is now
supported, which returns whether the given OID is the OID of another session's
temporary schema.

Release justification: None, waiting for v22.1.

@nvanbenschoten nvanbenschoten requested a review from a team September 8, 2021 02:30
@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Collaborator

@rafiss rafiss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for implementing these!

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @nvanbenschoten)


pkg/sql/sem/builtins/pg_builtins.go, line 847 at r3 (raw file):

					return tree.NewDOid(0), nil
				}
				oid, err := ctx.Planner.ResolveOIDFromString(

would be nice if we could use planner.Descriptors().GetSchemaByName() (and avoid going through the internal executor). but i believe it's not possible to add that to the EvalPlanner interface because of the dependencies it has.

how would you feel about adding a test that uses this builtin to rttanalysis.BenchmarkORMQueries? then if we resolve #69495 or improve this in some other way, we'd be able to measure it


pkg/sql/sem/builtins/pg_builtins.go, line 875 at r4 (raw file):

			Fn: func(ctx *tree.EvalContext, args tree.Datums) (tree.Datum, error) {
				schemaArg := tree.UnwrapDatum(ctx, args[0])
				schema, err := getNameForArg(ctx, schemaArg, "pg_namespace", "nspname")

similarly, could you add a benchmark for this?

@nvanbenschoten nvanbenschoten force-pushed the nvanbenschoten/pg_is_other_temp_schema branch from 227ef71 to b8e200a Compare September 9, 2021 17:45
Copy link
Member Author

@nvanbenschoten nvanbenschoten left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TFTR!

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @rafiss)


pkg/sql/sem/builtins/pg_builtins.go, line 847 at r3 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

would be nice if we could use planner.Descriptors().GetSchemaByName() (and avoid going through the internal executor). but i believe it's not possible to add that to the EvalPlanner interface because of the dependencies it has.

how would you feel about adding a test that uses this builtin to rttanalysis.BenchmarkORMQueries? then if we resolve #69495 or improve this in some other way, we'd be able to measure it

Done.

BenchmarkORMQueries/pg_my_temp_schema-16         	     177	   6485745 ns/op	         2.000 roundtrips

pkg/sql/sem/builtins/pg_builtins.go, line 875 at r4 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

similarly, could you add a benchmark for this?

Done.

BenchmarkORMQueries/pg_is_other_temp_schema-16         	     100	  12407798 ns/op	         3.000 roundtrips

@jordanlewis jordanlewis requested a review from a team September 9, 2021 17:57
Copy link
Collaborator

@rafiss rafiss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm! would be good to get a look from sql-schema, specifically for awareness about the ResolveOIDFromString and getNameForArg calls, just in case there's an immediately obvious improvement to be had.

Copy link
Contributor

@ajwerner ajwerner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @nvanbenschoten and @rafiss)


pkg/bench/rttanalysis/orm_queries_bench_test.go, line 187 at r8 (raw file):

Quoted 12 lines of code…
		{
			Name: "pg_my_temp_schema",
			Setup: `SET experimental_enable_temp_tables = true;
              CREATE TEMP TABLE t(a int primary key, b int)`,
			Stmt: `SELECT pg_my_temp_schema()`,
		},
		{
			Name: "pg_is_other_temp_schema",
			Setup: `SET experimental_enable_temp_tables = true;
              CREATE TEMP TABLE t(a int primary key, b int)`,
			Stmt: `SELECT pg_is_other_temp_schema('t'::regclass)`,
		},

thoughts on adding another one that does it twice in the same statement and shows that that is currently worse than doing it once?

@nvanbenschoten nvanbenschoten force-pushed the nvanbenschoten/pg_is_other_temp_schema branch from b8e200a to cc41508 Compare September 13, 2021 23:05
Copy link
Member Author

@nvanbenschoten nvanbenschoten left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @rafiss)


pkg/bench/rttanalysis/orm_queries_bench_test.go, line 187 at r8 (raw file):

Previously, ajwerner wrote…
		{
			Name: "pg_my_temp_schema",
			Setup: `SET experimental_enable_temp_tables = true;
              CREATE TEMP TABLE t(a int primary key, b int)`,
			Stmt: `SELECT pg_my_temp_schema()`,
		},
		{
			Name: "pg_is_other_temp_schema",
			Setup: `SET experimental_enable_temp_tables = true;
              CREATE TEMP TABLE t(a int primary key, b int)`,
			Stmt: `SELECT pg_is_other_temp_schema('t'::regclass)`,
		},

thoughts on adding another one that does it twice in the same statement and shows that that is currently worse than doing it once?

Done for both.

name                                                  time/op
ORMQueries/pg_my_temp_schema-16                       6.77ms ± 0%
ORMQueries/pg_my_temp_schema_multiple_times-16        6.54ms ± 0%
ORMQueries/pg_is_other_temp_schema-16                 7.41ms ± 0%
ORMQueries/pg_is_other_temp_schema_multiple_times-16  11.3ms ± 0%

name                                                  roundtrips
ORMQueries/pg_my_temp_schema-16                         2.00 ± 0%
ORMQueries/pg_my_temp_schema_multiple_times-16          2.00 ± 0%
ORMQueries/pg_is_other_temp_schema-16                   6.99 ± 0%
ORMQueries/pg_is_other_temp_schema_multiple_times-16    11.0 ± 0%

craig bot pushed a commit that referenced this pull request Sep 14, 2021
69913: sql: implement information_schema.{_pg_truetypid, _pg_truetypmod, _pg_char_max_length} r=nvanbenschoten a=nvanbenschoten

Needed for #69010.

This PR adds implementations for the following three builtin functions
`information_schema._pg_truetypid`
`information_schema._pg_truetypmod`
`information_schema._pg_char_max_length`

The first two functions return the "true" type ID and modifier, disregarding indirection introduced by domain types. The third returns the maximum character length of a type with the provided ID and modifier.

The builtins are implemented as user-defined functions in Postgres here: https://github.com/postgres/postgres/blob/master/src/backend/catalog/information_schema.sql

Combined with #69909 and #69911, this PR unlocks these two gnarly introspection queries in PostgREST:
- https://github.com/PostgREST/postgrest/blob/b05898d17f8e33c8c82fc1d05a30eb3044999668/src/PostgREST/DbStructure.hs#L538
- https://github.com/PostgREST/postgrest/blob/b05898d17f8e33c8c82fc1d05a30eb3044999668/src/PostgREST/DbStructure.hs#L709

Release justification: None, waiting for v22.1.

Co-authored-by: Nathan VanBenschoten <[email protected]>
This commit updated the pg_class virtual table population routing to
set the `relistemp` column properly. We almost addressed this in b1d1547,
but it was missed.

Release justification: None, waiting for v22.1.
This commit moves the `pg_get_serial_sequence` and `pg_collation_for` builtin
functions from `builtins.go` to `pg_builtins.go`, where they belong. Strictly
code movement.

Release justification: None, waiting for v22.1.
This commit adds a real implementation for the `pg_my_temp_schema` builtin
function. `pg_my_temp_schema` returns the OID of session's temporary schema,
or 0 if the session has not yet created a temporary schema.

Release note (sql change): The pg_my_temp_schema builtin function now properly
returns the OID of the active session's temporary schema, if one exists.

Release justification: None, waiting for v22.1.
Needed for cockroachdb#69010.

This commit implements the `pg_is_other_temp_schema` builtin function.
`pg_is_other_temp_schema` returns true if the given OID is the OID of another
session's temporary schema. This can be useful, for example, to exclude other
sessions' temporary tables from a catalog display.

Release note (sql change): The pg_is_other_temp_schema builtin function is now
supported, which returns whether the given OID is the OID of another session's
temporary schema.

Release justification: None, waiting for v22.1.
@nvanbenschoten nvanbenschoten force-pushed the nvanbenschoten/pg_is_other_temp_schema branch from cc41508 to f9a8de9 Compare September 14, 2021 01:15
@nvanbenschoten
Copy link
Member Author

bors r+

@craig
Copy link
Contributor

craig bot commented Sep 14, 2021

Build succeeded:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants