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.

Sign up for GitHub

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

Jump to bottom

sql: use crdb_internal for pg_proc.pronamespace when needed #95029

Merged
merged 2 commits into from
Jan 13, 2023
Merged

sql: use crdb_internal for pg_proc.pronamespace when needed #95029

merged 2 commits into from
Jan 13, 2023

Conversation

rafiss
Copy link
Collaborator

@rafiss rafiss commented Jan 10, 2023 •
edited by knz
Loading

Epic: CRDB-23454
fixes #94952

Release note (bug fix): The pronamespace column of the pg_proc table now correctly reports the crdb_internal schema for built-in functions that have the "crdb_internal" prefix.

comprules: support completion for qualified functions

The syntax completion engine now generates suggestion for user-defined
functions. It restricts the suggestions to only functions in the schema
name if one was provided, or else to the schemas in the search_path.

No release note since this functionality is new in v23.1.

@rafiss rafiss requested review from knz, e-mbrown and a team January 10, 2023 23:01
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@rafiss rafiss force-pushed the fix-pg-proc-crdb-internal branch from 140c0dc to 8113484 Compare January 10, 2023 23:08
e-mbrown
e-mbrown approved these changes Jan 11, 2023
View reviewed changes
Copy link
Contributor

@e-mbrown e-mbrown left a comment

Choose a reason for hiding this comment

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

LGTM!

Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @knz)

@rafiss
Copy link
Collaborator Author

rafiss commented Jan 11, 2023

@knz could you PTAL at the changes in the show completions test? CI fails with

        /home/roach/.cache/bazel/_bazel_roach/c5a4e7d36696d9cd970af2045211a7df/sandbox/processwrapper-sandbox/9461/execroot/com_github_cockroachdb_cockroach/bazel-out/k8-fastbuild/bin/pkg/sql/logictest/tests/local-vec-off/local-vec-off_test_/local-vec-off_test.runfiles/com_github_cockroachdb_cockroach/pkg/sql/logictest/testdata/logic_test/show_completions:40: SELECT completion FROM [SHOW COMPLETIONS AT OFFSET 10 FOR 'SHOW CREAT']
        expected:
            CREATE
            CREATEDB
            CREATELOGIN
            CREATEROLE
        but found (query options: "") :
            CREATE
            CREATEDB
            CREATELOGIN
            CREATEROLE
            create_join_token(
            create_regclass(
            create_regnamespace(
            create_regproc(
            create_regprocedure(
            create_regrole(
            create_regtype(
            create_session_revival_token(
            create_sql_schema_telemetry_job(

So it looks like it's suggesting crdb_internal builtin functions now. But the suggestion won't work, since crdb_internal is not on the search_path by default. We want to restrict the suggested completions to only include items resolvable in the search_path. Would you agree, and if so, where should I make the change?

knz
knz approved these changes Jan 11, 2023
View reviewed changes
Copy link
Contributor

@knz knz left a comment

Choose a reason for hiding this comment

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

where should I make the change?
Very good question!

You'd need to modify the completion rule for functions, at pkg/sql/comprules/rules.go line 160.
We want to use a query that enumerate the functions visible from the current search_path.

So I believe this willb e something like replacing

SELECT DISTINCT proname FROM pg_catalog.pg_proc

to

SELECT DISTINCT proname FROM pg_catalog.pg_proc p,
 JOIN  pg_catalog.pg_namespace n ON p.pronamespace = n.oid
WHERE n.nspname = ANY pg_catalog.current_schemas(TRUE)

You'll also see that function says something about built-in function comments not being available through pg_objdescription yet. Should we file an issue about that?

Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @rafiss)

knz
knz reviewed Jan 11, 2023
View reviewed changes
Copy link
Contributor

@knz knz left a comment

Choose a reason for hiding this comment

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

Ooh wait my previous explanation is wrong. That completion rule is a bit more intelligent, and is able to recognize a namespace prefix earlier in the SQL syntax. This should be used instead of current_schemas if available.

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

@knz
Copy link
Contributor

knz commented Jan 11, 2023

If you'd like we can do some pair programming later this week to iterate on this!

@rafiss
Copy link
Collaborator Author

rafiss commented Jan 11, 2023

You'll also see that function says something about built-in function comments not being available through pg_objdescription yet. Should we file an issue about that?

since we don't have comment on function support yet (#44135) i think we can just track built-in function comments using that issue as well.

@rafiss rafiss requested a review from a team as a code owner January 12, 2023 18:02
@rafiss rafiss force-pushed the fix-pg-proc-crdb-internal branch from 1cdbb29 to 9941ce1 Compare January 12, 2023 19:00
@rafiss rafiss requested a review from knz January 12, 2023 20:28
@rafiss
Copy link
Collaborator Author

rafiss commented Jan 12, 2023

@knz thanks for the pointer. I added a new commit to fix the completion rules. Can you review it when you have time?

knz
knz approved these changes Jan 12, 2023
View reviewed changes
Copy link
Contributor

@knz knz left a comment

Choose a reason for hiding this comment

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

You got this! I'm so glad you found the simplification. This architecture works.

There's a few nits we need to take care of: #95029

LGTM modulo something like that + necessary test adjustments.

Reviewed 1 of 5 files at r2, all commit messages.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @e-mbrown and @rafiss)

pkg/sql/comprules/rules.go line 150 at r2 (raw file):

	// built-in functions through pg_catalog.
	const query = `
WITH p AS (SELECT DISTINCT proname, nspname FROM pg_catalog.pg_proc JOIN pg_catalog.pg_namespace n ON n.oid = pronamespace)

👏 👏 👏

knz
knz reviewed Jan 12, 2023
View reviewed changes
Copy link
Contributor

@knz knz left a comment

Choose a reason for hiding this comment

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

Sorry I meant to link to this: rafiss#1

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

@rafiss rafiss force-pushed the fix-pg-proc-crdb-internal branch from 9941ce1 to e6303fc Compare January 12, 2023 22:55
knz
knz approved these changes Jan 12, 2023
View reviewed changes
Copy link
Contributor

@knz knz left a comment

Choose a reason for hiding this comment

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

Reviewed 3 of 5 files at r2, 4 of 4 files at r3, all commit messages.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @rafiss)

@rafiss rafiss force-pushed the fix-pg-proc-crdb-internal branch 3 times, most recently from f46e8b6 to 78feb8e Compare January 13, 2023 05:54
rafiss added 2 commits January 13, 2023 06:48
@rafiss
sql: use crdb_internal for pg_proc.pronamespace when needed
992fd5d 
Release note (bug fix): The pronamespace column of the pg_proc table
now correctly reports the crdb_internal schema for built-in functions
that have the "crdb_internal" prefix.
@rafiss
comprules: support completion for qualified functions
83fca32 
The syntax completion engine now generates suggestion for user-defined
functions. It restricts the suggestions to only functions in the schema
name if one was provided, or else to the schemas in the search_path.

No release note since this functionality is new in v23.1.

Release note: None
@rafiss rafiss force-pushed the fix-pg-proc-crdb-internal branch from 78feb8e to 83fca32 Compare January 13, 2023 11:48
@knz
Copy link
Contributor

knz commented Jan 13, 2023

The remaining flake in CI here is not yours. Let's file an issue for it (and perhaps skip it), you can merge your PR.

@knz
Copy link
Contributor

knz commented Jan 13, 2023

i'll file it for you

@knz
Copy link
Contributor

knz commented Jan 13, 2023

#95201

@rafiss
Copy link
Collaborator Author

rafiss commented Jan 13, 2023

thank you for the help!

bors r=knz

@craig
Copy link
Contributor

craig bot commented Jan 13, 2023

Build failed:

@rafiss
Copy link
Collaborator Author

rafiss commented Jan 13, 2023

flake was #94956 which is also on me to fix (unrelated to this PR)

bors r=knz

@craig
Copy link
Contributor

craig bot commented Jan 13, 2023

Build failed (retrying...):

@craig
Copy link
Contributor

craig bot commented Jan 13, 2023

Build succeeded:

@craig craig bot merged commit 6f5d89e into cockroachdb:master Jan 13, 2023
@rafiss rafiss deleted the fix-pg-proc-crdb-internal branch January 18, 2023 08:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@e-mbrown e-mbrown e-mbrown approved these changes

@knz knz knz approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

sql: pg_proc.pronamespace is not populated for built-in functions
4 participants
@rafiss @cockroach-teamcity @knz @e-mbrown