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

sqlsmith: add initial support for PL/pgSQL #119037

Merged
merged 2 commits into from Mar 11, 2024

Conversation

DrewKimball
Copy link
Collaborator

@DrewKimball DrewKimball commented Feb 9, 2024

plpgsql: add support for NULL statements

This commit implements the PL/pgSQL NULL statement, which is a no-op.
This will be convenient for the following commit, which adds initial
support for PL/pgSQL to sqlsmith.

Informs #117744
Informs #106368

Release note (sql change): Added support for the PL/pgSQL NULL statement.

sqlsmith: add initial support for PL/pgSQL

This commit adds initial support for PL/pgSQL routines. The following
statements are supported:

  • Block
  • If/Elsif/Else
  • Return
  • Null
  • Assign
  • SQL statements

Informs #106368

Release note: None

@DrewKimball DrewKimball requested a review from a team as a code owner February 9, 2024 23:01
@DrewKimball DrewKimball requested review from rytaft and removed request for a team February 9, 2024 23:01
Copy link

blathers-crl bot commented Feb 9, 2024

Your pull request contains more than 1000 changes. It is strongly encouraged to split big PRs into smaller chunks.

🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.

@DrewKimball
Copy link
Collaborator Author

The first three commits are #119034.

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@DrewKimball
Copy link
Collaborator Author

Example generated statement:

CREATE FUNCTION func_29()
	RETURNS RECORD
	LANGUAGE plpgsql
	AS $funcbody$
DECLARE
decl_205 NAME := e'C\x14':::STRING:::NAME;
"D
ecl_206" INET := '218.182.213.88/3':::INET;
"😃Decl_207" VOID;
decl_208 TIMETZ;
"de
   cl_209" REFCURSOR := e'q2Jm\x046j':::REFCURSOR;
"d e%pcl_210" REGTYPE := 1023113024:::OID;
BEGIN
"D
ecl_206" := "D
ecl_206";
RETURN (1461997381:::OID, '23:03:00.602946':::TIME, 783147277:::OID);
NULL;
SELECT e'w24G9,\x06%':::REFCURSOR AS col_3019, "t{ab_1855"."c.ol0__2" AS col_3020, '115.161.46.85/21':::INET AS col_3021 FROM defaultdb.public.tab_1110@"tab_1110_c.ol0__2_key" AS "t{ab_1855" LIMIT 89:::INT8;
IF false THEN
	DELETE FROM defaultdb.public.tab_1110 AS tab_1856 USING defaultdb.public.tab_1110 AS "tab\g_1857", defaultdb.public.tab_1110 AS tab_1858, defaultdb.public.tab_1110 AS t😰ab_1859, defaultdb.public.tab_1110 AS "ta\\u3F42b͌_1860" WHERE false LIMIT 25:::INT8;
ELSIF true THEN
	"d e%pcl_210" := 3429146290:::OID;
END IF;
END;
$funcbody$;

Copy link
Member

@yuzefovich yuzefovich left a comment

Choose a reason for hiding this comment

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

Nice! I have a few nits.

Reviewed 3 of 3 files at r4, 5 of 5 files at r5, all commit messages.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @DrewKimball and @rytaft)


pkg/internal/sqlsmith/plpgsql.go line 90 at r5 (raw file):

	for i := 0; i < numStmts; i++ {
		for {
			// No need for a retry counter, because eventually a NULL statement will

I'm a bit confused by this comment - can you expand on it? Is it effectively saying that we don't need to count the number of times Next is called when it returns ok = false because at some point NULL stmt is returned which will have ok = true, so infinite loop is impossible?


pkg/internal/sqlsmith/plpgsql.go line 109 at r5 (raw file):

	}
	if s.coin() {
		numElsIfs := s.rnd.Intn(3) + 1

nit: perhaps s/numElsIfs/numElseIfs/.


pkg/internal/sqlsmith/plpgsql.go line 136 at r5 (raw file):

}

// TODO(#106368): implement generation for the remaining statements.

nit: this issue is being closed by this PR right now - should this refer to a different issue? Or should this PR not fully close it?

Copy link
Collaborator Author

@DrewKimball DrewKimball 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 @rytaft and @yuzefovich)


pkg/internal/sqlsmith/plpgsql.go line 90 at r5 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

I'm a bit confused by this comment - can you expand on it? Is it effectively saying that we don't need to count the number of times Next is called when it returns ok = false because at some point NULL stmt is returned which will have ok = true, so infinite loop is impossible?

Yes, that's exactly it. I modified the comment to explain that better.


pkg/internal/sqlsmith/plpgsql.go line 109 at r5 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

nit: perhaps s/numElsIfs/numElseIfs/.

Done.


pkg/internal/sqlsmith/plpgsql.go line 136 at r5 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

nit: this issue is being closed by this PR right now - should this refer to a different issue? Or should this PR not fully close it?

Good point. I'll leave the issue open to track support for other statements.

This commit implements the PL/pgSQL NULL statement, which is a no-op.
This will be convenient for the following commit, which adds initial
support for PL/pgSQL to sqlsmith.

Informs cockroachdb#117744
Informs cockroachdb#106368

Release note (sql change): Added support for the PL/pgSQL NULL statement.
This commit adds initial support for PL/pgSQL routines. The following
statements are supported:
* Block
* If/Elsif/Else
* Return
* Null
* Assign
* SQL statements

Informs cockroachdb#106368

Release note: None
Copy link
Member

@yuzefovich yuzefovich 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 8 of 9 files at r1, 1 of 3 files at r7, 2 of 2 files at r8, 3 of 3 files at r9, 5 of 5 files at r10, 17 of 17 files at r11, 16 of 16 files at r12, all commit messages.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @rytaft)


pkg/internal/sqlsmith/plpgsql.go line 109 at r5 (raw file):

Previously, DrewKimball (Drew Kimball) wrote…

Done.

Sorry, I didn't realize that ELSIF is the keyword - up to you whether you want to revert the change in response to my comment.

Copy link
Collaborator Author

@DrewKimball DrewKimball 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! 1 of 0 LGTMs obtained (waiting on @rytaft)


pkg/internal/sqlsmith/plpgsql.go line 109 at r5 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

Sorry, I didn't realize that ELSIF is the keyword - up to you whether you want to revert the change in response to my comment.

Eh, I think numElseIfs is good. TFYR!

@DrewKimball
Copy link
Collaborator Author

bors r+

@craig
Copy link
Contributor

craig bot commented Mar 11, 2024

Build succeeded:

@craig craig bot merged commit 0801dd3 into cockroachdb:master Mar 11, 2024
17 of 18 checks passed
@DrewKimball DrewKimball deleted the sqlsmith-plpgsql branch March 11, 2024 23:15
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.

None yet

3 participants