-
Notifications
You must be signed in to change notification settings - Fork 3.9k
sql: portable SHOW CREATE FUNCTION
output
#148746
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
base: master
Are you sure you want to change the base?
Conversation
It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR? 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
7397e84
to
a8e8d36
Compare
1c6d3ac
to
4a43efb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great. I had some additional test suggestions.
Reviewed 11 of 11 files at r1, all commit messages.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @jeffswenson)
pkg/sql/show_create_clauses.go
line 282 at r1 (raw file):
// walking the table names using the reformat option. the buffer is simply discarded f := tree.NewFmtCtx(tree.FmtSimple, tree.FmtReformatTableNames(func(ctx *tree.FmtCtx, tn *tree.TableName) {
nit: we have two formatters in this function f
and fmtCtx
. It's subtle stuff to understand why two are needed. I suggest using better variable names. For example, f
could be renamed to dbStripCtx
and fmtCtx
could be renamed to prettyPrintCtx
pkg/sql/show_create_clauses.go
line 283 at r1 (raw file):
// walking the table names using the reformat option. the buffer is simply discarded f := tree.NewFmtCtx(tree.FmtSimple, tree.FmtReformatTableNames(func(ctx *tree.FmtCtx, tn *tree.TableName) { if string(tn.CatalogName) == databaseName {
nit: it's probably fine, but can we have a test where a database name has a unicode character and a case-sensitive name (e.g. "myDB")?
pkg/sql/show_create_clauses.go
line 294 at r1 (raw file):
switch lang { case catpb.Function_SQL: parsedStmts, err := parser.Parse(queries)
The function bodies in triggers are lazily evaluated. So, you can have reference to non-existent objects. I think this should still parse fine, but could you ensure we have some SHOW CREATE FUNCTION
tests for these?
For example, in both cases the objects referenced in the select don't exist. These are allowed to be created. And you will get a non-existent object when you try to use the function in a trigger:
create function t1() returns trigger as $$ BEGIN select not valid(); end; $$ LANGUAGE PLpgSQL;
create function t2() returns trigger as $$ BEGIN select 1 from a.b.c; end; $$ LANGUAGE PLpgSQL;
pkg/sql/show_create_clauses.go
line 327 at r1 (raw file):
fmtCtx.FormatNode(stmts) default: return queries, nil
I think we only support SQL and PLpgSQL. Can we turn this into an assertion failure instead? That way if we add a new one in the future, we will know to update this code.
4a43efb
to
c8fa698
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @jeffswenson and @spilchen)
pkg/sql/show_create_clauses.go
line 294 at r1 (raw file):
Previously, spilchen wrote…
The function bodies in triggers are lazily evaluated. So, you can have reference to non-existent objects. I think this should still parse fine, but could you ensure we have some
SHOW CREATE FUNCTION
tests for these?For example, in both cases the objects referenced in the select don't exist. These are allowed to be created. And you will get a non-existent object when you try to use the function in a trigger:
create function t1() returns trigger as $$ BEGIN select not valid(); end; $$ LANGUAGE PLpgSQL; create function t2() returns trigger as $$ BEGIN select 1 from a.b.c; end; $$ LANGUAGE PLpgSQL;
Done.
pkg/sql/show_create_clauses.go
line 327 at r1 (raw file):
Previously, spilchen wrote…
I think we only support SQL and PLpgSQL. Can we turn this into an assertion failure instead? That way if we add a new one in the future, we will know to update this code.
Changed it to an error return or what do you mean by an assertion failure here?
The other format helpers just return empty strings without error (1, 2.
c8fa698
to
13d341e
Compare
Previously, the `SHOW CREATE <FUNCTION | PROCEDURE>` statements would include the fully qualified table names. The database component of the table name made it unportable. This change uses the unqualified table name to make the output portable. Fixes: cockroachdb#125936 Epic: CRDB-39674 Release note (sql change): The `SHOW CREATE FUNCTION` and `SHOW CREATE PROCEDURE` statements produce the 2-part table names rather than qualifying with the current database.
13d341e
to
6ca142c
Compare
Previously, the
SHOW CREATE FUNCTION
statement would include the fullyqualified table names.
The database component of the table name made it unportable.
This change uses the unqualified table name to make the output portable.
Fixes: #125936
Epic: CRDB-39674
Release note (sql change): The
SHOW CREATE FUNCTION
statement producesthe 2-part table names rather than qualifying with the current database.