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

delegate: simplify / speed up SHOW FUNCTIONS; redefine several pg builtins as UDFs; fix pg_proc virtual index #94771

Merged
merged 3 commits into from
Jan 15, 2023

Conversation

jordanlewis
Copy link
Member

@jordanlewis jordanlewis commented Jan 5, 2023

Epic: CRDB-23454
Closes #94746
Closes #94953

Remove a couple of unnecessary builtin function calls in SHOW FUNCTIONS.
The query used to take 50 seconds, now it takes less than a second.

Also, rewrite several pg compatibility builtins as UDFs for improved performance.

Also, fix the pg_proc oid virtual index which didn't work for user-defined functions.

Epic: None

@jordanlewis jordanlewis requested a review from a team January 5, 2023 18:50
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@jordanlewis jordanlewis changed the title delegate: simplify / speed up SHOW FUNCTIONS delegate: simplify / speed up SHOW FUNCTIONS; redefine several pg builtins as UDFs Jan 5, 2023
@jordanlewis jordanlewis force-pushed the show-functions-and-udf-builtins branch 2 times, most recently from cf5484b to 13cf0f9 Compare January 5, 2023 19:45
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.

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


pkg/sql/delegate/show_functions.go line 75 at r1 (raw file):

WHERE true
%[2]s
GROUP BY n.nspname, p.proname, p.prorettype, p.prokind, p.provolatile

why do we need the GROUP BY? also, could we just do group by 1, 2, 3, 4


pkg/sql/sem/builtins/pg_builtins.go line 274 at r2 (raw file):

		FROM pg_catalog.pg_matviews v
		JOIN pg_catalog.pg_class c ON c.relname=v.matviewname
		WHERE c.oid=$1`,

looks like this should return NULL when there's no view with that OID. does this new version do that, or return 0 rows? (and is it already tested?)


pkg/sql/sem/builtins/pg_builtins.go line 287 at r2 (raw file):

		IsUDF:      true,
		Body: `SELECT COALESCE((SELECT condef FROM pg_catalog.pg_constraint WHERE oid=$1),
													  'unknown constraint (OID=' || $1 || ')')`,

is this right? instead of returning an error, this will return a string row.

also, it seems like the PG behavior is that null is returned in this case, so i'm not sure why the old code returned a pgerror


pkg/sql/sem/builtins/pg_builtins.go line 653 at r2 (raw file):

			ReturnType: tree.FixedReturnType(types.String),
			IsUDF:      true,
			Body: `SELECT trim('{}' FROM

nit: could you a comment saying how it's getting reformatted (like you added below)


pkg/sql/sem/builtins/pg_builtins.go line 873 at r2 (raw file):

		tree.Overload{
			Types:      tree.ParamTypes{{Name: "sequence_oid", Typ: types.Oid}},
			ReturnType: tree.FixedReturnType(types.String),

can the return type change to a record now?


pkg/sql/sem/builtins/pg_builtins.go line 877 at r2 (raw file):

			Body: `SELECT COALESCE (((seqstart, seqmin, seqmax, seqincrement, seqcycle, seqcache, seqtypeid)
             FROM pg_catalog.pg_sequence WHERE seqrelid=$1 LIMIT 1),
             'unknown sequence (OID=' || $1 || ')')`,

hm in PG this one actually returns an error, but now it will just return this message in the results

crazy how the pg_* builtins are inconsistent about how they behave with not found entities

@jordanlewis jordanlewis force-pushed the show-functions-and-udf-builtins branch from 13cf0f9 to 4d140e4 Compare January 5, 2023 23:01
@jordanlewis jordanlewis requested a review from rafiss January 5, 2023 23:01
Copy link
Member Author

@jordanlewis jordanlewis 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/sql/delegate/show_functions.go line 75 at r1 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

why do we need the GROUP BY? also, could we just do group by 1, 2, 3, 4

The presence of the array_agg requires the group by. Okay, that's a better pattern.


pkg/sql/sem/builtins/pg_builtins.go line 274 at r2 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

looks like this should return NULL when there's no view with that OID. does this new version do that, or return 0 rows? (and is it already tested?)

Returning 0 rows here is the same as returning NULL - all of the builtin functions like this one behave like that


pkg/sql/sem/builtins/pg_builtins.go line 287 at r2 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

is this right? instead of returning an error, this will return a string row.

also, it seems like the PG behavior is that null is returned in this case, so i'm not sure why the old code returned a pgerror

Done.


pkg/sql/sem/builtins/pg_builtins.go line 873 at r2 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

can the return type change to a record now?

Done.


pkg/sql/sem/builtins/pg_builtins.go line 877 at r2 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

hm in PG this one actually returns an error, but now it will just return this message in the results

crazy how the pg_* builtins are inconsistent about how they behave with not found entities

Done.

@jordanlewis jordanlewis force-pushed the show-functions-and-udf-builtins branch from 4d140e4 to 22a6bca Compare January 6, 2023 04:47
@jordanlewis jordanlewis changed the title delegate: simplify / speed up SHOW FUNCTIONS; redefine several pg builtins as UDFs delegate: simplify / speed up SHOW FUNCTIONS; redefine several pg builtins as UDFs; fix pg_proc virtual index Jan 6, 2023
@jordanlewis jordanlewis force-pushed the show-functions-and-udf-builtins branch 3 times, most recently from 21e24ed to e4027a0 Compare January 6, 2023 17:18
@jordanlewis jordanlewis requested a review from a team as a code owner January 6, 2023 17:18
@jordanlewis
Copy link
Member Author

Lots of little things to fix in there - anyway it's RFAL.

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.

cool!

Reviewed 1 of 2 files at r3, 2 of 3 files at r4, 1 of 1 files at r5.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @jordanlewis and @rharding6373)


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

	),

	"obj_description": makeBuiltin(defProps(),

obj_description and col_description are also great candidates for this rewrite. we've seen both come up with performance issues in the past

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.

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


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

Previously, rafiss (Rafi Shamim) wrote…

obj_description and col_description are also great candidates for this rewrite. we've seen both come up with performance issues in the past

#87947

Copy link
Collaborator

@rharding6373 rharding6373 left a comment

Choose a reason for hiding this comment

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

:lgtm: but @mgartner is the UDF expert so I've added him to take a look.

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @jordanlewis, @mgartner, and @rafiss)


pkg/sql/sem/builtins/pg_builtins.go line 887 at r6 (raw file):

		// The real implementation returns a record; we fake it by returning a
		// comma-delimited string enclosed by parentheses.
		// TODO(jordan): convert this to return a record type once we support that.

nit: is this TODO obsolete now?


pkg/sql/sem/builtins/pg_builtins.go line 1003 at r6 (raw file):

			FROM pg_catalog.pg_description
			WHERE objoid=$1
		  AND objsubid = 0

nit: this body and the next look like they have odd whitespace formatting

rafiss
rafiss previously requested changes Jan 9, 2023
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.

assuming this fixes #94953 could you add a test in pkg/sql/logictest/testdata/logic_test/pg_builtins for pg_function_is_visible? see the analagous tests for pg_type_is_visible

@rafiss
Copy link
Collaborator

rafiss commented Jan 9, 2023

You can copy test I added in #94959

Copy link
Collaborator

@mgartner mgartner left a comment

Choose a reason for hiding this comment

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

Cool! LGTM

# Ensure that the pg_proc virtual index works properly.

query TT
SELECT oid, proname FROM pg_proc WHERE oid = 'sc.proc_f_2'::regproc
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: force the index with pg_proc@pg_proc_oid_idx

Copy link
Member Author

Choose a reason for hiding this comment

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

You can't force virtual indexes - the optimizer gives an error.

@jordanlewis jordanlewis force-pushed the show-functions-and-udf-builtins branch 2 times, most recently from 0d1ae6c to 1c28200 Compare January 13, 2023 20:20
Previously, the pg_proc virtual oid index was broken for user-defined
functions.

Release note: None (no release with this bug)
Remove a couple of unnecessary builtin function calls in SHOW FUNCTIONS.
The query used to take 50 seconds, now it takes less than a second.

Release note (performance improvement): improve the performance of SHOW
FUNCTIONS
Release note (performance improvement): several pg compatibility
builtins have improved performance
@jordanlewis jordanlewis force-pushed the show-functions-and-udf-builtins branch from 1c28200 to ebe2686 Compare January 14, 2023 20:14
@jordanlewis jordanlewis dismissed rafiss’s stale review January 15, 2023 15:43

Responded to review comment

@jordanlewis
Copy link
Member Author

bors r+

@craig
Copy link
Contributor

craig bot commented Jan 15, 2023

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.

sql: pg_function_is_visible fails for UDFs sql: SHOW FUNCTIONS FROM pg_catalog is slow
5 participants