sql: add unimplemented errors and telemetry for plpgsql #99517

e-mbrown · 2023-03-24T19:59:19Z

Part of: #98147
Closes: #91569

This PR expands grammar rules and adds unimplemented support for pl/pgsql statements.

sql: Add telemetry for parsable plpgsql statements Is the first commit unique to this PR.

Release Notes: None

cockroach-teamcity · 2023-03-24T20:00:01Z

This change is

knz

The test suite is very good.

I wonder though - is there a way to do this without copy-pasting all the lexing code into a new directory? The lexing rules for plpgsql are the same as the regular SQL. And given their complexity, we certainly do not wish to have two copies to maintain side-by-side.

Error handling using panic is also unacceptable. There should be more uses of errors.AssertionFailedf in there.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @chengxiong-ruan, @e-mbrown, and @ZhouXing19)

pkg/sql/plpgsql/parser/plpgsql.y line 135 at r18 (raw file):

 */
// TODO (Chengxiong) figure out why these 3 tokens are needed
%token <plWord>		T_WORD		/* unrecognized simple identifier */

throughout: spaces, not tabs

chengxiong-ruan · 2023-03-27T15:13:19Z

I wonder though - is there a way to do this without copy-pasting all the lexing code into a new directory? The lexing rules for plpgsql are the same as the regular SQL. And given their complexity, we certainly do not wish to have two copies to maintain side-by-side.

Thanks for the callout! The lexer code for plpgsql actually has a bunch of customized logic which relies on ReadSqlConstruct which scans through remaining parts of a statement until a ; after seeing some expected plpgsql keywords. This is actually the most important part the plpgsql lexer working with non-plpgsql syntaxes. I think that merging it with the sql lexer would be very tricky. But this brought up a good question on the scanner which definitely contains a lot of complex logic which should be merged with the sql scanner, for example how it recognize numbers, string constants and etc... @e-mbrown could you compare the tow scanners? I think most part of them are the same.

e-mbrown

Sure thing!

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @chengxiong-ruan and @ZhouXing19)

knz · 2023-03-27T15:45:51Z

The lexer code for plpgsql actually has a bunch of customized logic which relies on ReadSqlConstruct which scans through remaining parts of a statement until a ; after seeing some expected plpgsql keywords. This is actually the most important part the plpgsql lexer working with non-plpgsql syntaxes.

Yes I agree.

I think that merging it with the sql lexer would be very tricky

Here I disagree. There's a thing we can do with dependency injection. I'd really like us to look at this seriously because the organizational overhead of a code split like you propose here is really going to hurt us in the long run.

knz · 2023-03-27T15:46:10Z

(I'll volunteer to sit down and discuss to get something workable)

chengxiong-ruan · 2023-03-27T16:10:53Z

(I'll volunteer to sit down and discuss to get something workable)

Thanks! Love to discuss about this.

knz · 2023-03-28T13:48:12Z

In preparation of our chat, I went to the pg source code and saw a few things:

the plpgsql scanner is indeed shared with the main sql scanner in pg. This is done by making the list of keywords a parameter of the scanner (if my reading is correct). The remainder - identifier vs literal scans etc is just the same code.
the plpgsql grammar uses separate API functions read_sql_stmt(), read_sql_construct(), read_sql_expression(), read_datatype() which makes the code much more succinct and readable. The work here should probably benefit from that refinement too.

ZhouXing19 · 2023-03-28T15:25:14Z

I agree that we can reuse the sql parser's scanner and lexer part. We can use the shared part in a cleaner way, and for parts that need to be customized for plpgsql, just override them or add new methods.

Just wanted to add some details to what Raphael has mentioned, postgres's plpg scanner share with their sql scanner. So I think we may want to have something similar, maybe like this:

type PLScanner struct {
	scanner  sqlscanner.Scanner
}

and override methods if needed.

My original concern was that importing sql/scanner or sql/parser into the plpgsql package would bring dependency loop, as we may want to hook the plpgsql parser in sql.y. But seems that is avoidable if we delay the function body parsing till the execution layer, like this.

knz · 2023-03-28T15:28:37Z

Importing sql/scanner or sql/parser into the plpgsql package would bring dependency loop, as we may want to hook the plpgsql parser in sql.y.

let's fix this using dependency injection.

chengxiong-ruan · 2023-03-28T15:42:03Z

Importing sql/scanner or sql/parser into the plpgsql package would bring dependency loop, as we may want to hook the plpgsql parser in sql.y.

Maybe we don't really need worry about hooking plpgsql parser in sql.y. I think when you call CREATE FUNCTION, function body is treated as pure string, statements are then parsed in optbuilder to analyze dependencies. One minor concern is that there are different hard coded scanning logic in the scanners, for example. But I think we cant sort it out somehow.

My biggest concern is indeed how we merge the two lexer when one of them has these ReadSqlConstruct thing (we actually copied the idea from postgres :) ). Another concern would be the lookahead logics which I think can be handled with dep injection.

Look forward to the chat~

knz

In terms of code sharing, in decreasing order of priority:

the code generators (e.g. the .awk file, the main.go in 'allkeywords', maybe the .sh file with an extra argument) can be defined in just 1 place and then referred to in the BUILD files from multiple go packages.
make a base impl of the code in scanner.go, with the Scan method provided via the constructor, then specialize 2 different Scan functions in the 2 different packages. This way we effectively reuse the ident/str /etc parsing code.
try and make all the complex error handling (construction of error obj + populating err details + telemetry) inside helper functions used in both places.
(stretch goal) explore more code sharing from lexer.go but perhaps at a lower priority.

Separately another thing that would make my day slightly brighter is to polish the .y file to make it look more like the other wrt whitespace, alignment, comments etc. Your call.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @chengxiong-ruan, @e-mbrown, and @ZhouXing19)

chengxiong-ruan

Looks good in general. But I have a few major comments.

chengxiong-ruan · 2023-04-20T18:37:19Z

pkg/sql/conn_executor_exec.go

@@ -835,6 +836,16 @@ func (ex *connExecutor) execStmtInOpenState(
 			return makeErrEvent(err)
 		}
 		return nil, nil, nil
+
+	case *tree.CreateFunction:


I don't think we need to make this into the conn executor. Do you want to give it a shot in optbuilder/create_function.go?

chengxiong-ruan · 2023-04-20T18:44:29Z

pkg/sql/plpgsql/parser/parse.go

+	if err != nil {
+		return nil, err
+	}
+	for _, s := range stmt.AST.Body {


I don't think this quite works because statement blocks can be nested. Do you want to give it a shot writing a visitor pattern for it so that we'd be able to walk through the whole AST. For example:

cockroach/pkg/sql/sem/tree/walk.go

Line 1975 in ac03a55

func walkStmt(v Visitor, stmt Statement) (newStmt Statement, changed bool) {

chengxiong-ruan · 2023-04-20T18:46:55Z

pkg/sql/testdata/telemetry/function

+$$
+----
+error: pq: unimplemented: plpgsql not supported for udf
+errorcodes.0A000                      1


I think we should revisit this list of output to show metrics that are interested. Namely, only show those plpgsql related? Let me know if you think why the other metrics are needed here.

Unimplemented errors and parser test were added for `stmt_perform`. Release note: None

Unimplemented errors and parser test were added for `stmt_raise`. Release note: None

chengxiong-ruan · 2023-04-26T18:21:43Z

pkg/sql/opt/optbuilder/create_function.go

@@ -99,6 +100,12 @@ func (b *Builder) buildCreateFunction(cf *tree.CreateFunction, inScope *scope) (
 			if _, err := funcinfo.FunctionLangToProto(opt); err != nil {
 				panic(err)
 			}
+
+			if opt == tree.FunctionLangPLpgSQL {


comment not added yet?

chengxiong-ruan · 2023-04-26T18:24:05Z

pkg/sql/plpgsql/parser/parse.go

+		return nil, err
+	}
+
+	for _, s := range stmt.AST.Body {


Heh, PLpgSQLStmtBlock should be also a statement, No? Namely, looks like you don't need to do the loop here, instead do it in the walkStmt of PLpgSQLStmtBlock?

chengxiong-ruan · 2023-04-26T18:27:04Z

pkg/sql/sem/plpgsqltree/plpg_visitor.go

+func Walk(v plpgsqlVisitor, stmt PLpgSQLStatement) error {
+	wStmt, isWalkable := stmt.(walkableStmt)
+
+	if !isWalkable {


I don't think we'd need the isWalkable concept anymore? It means that everything statement should do wStmt.walkStmt(v) and do v.visit in it.

chengxiong-ruan · 2023-04-26T18:27:44Z

pkg/sql/sem/plpgsqltree/plpg_visitor.go

+var _ walkableStmt = &PLpgSQLStmtIfElseIfArm{}
+
+// Walk walks plpgsql statements to ensure that telemetry is captured for nested statements.
+func Walk(v plpgsqlVisitor, stmt PLpgSQLStatement) error {


We should use the visitor interface type in function signature.

chengxiong-ruan · 2023-04-26T18:28:46Z

pkg/sql/sem/plpgsqltree/statements.go

+
+	for _, thenStmt := range s.ThenBody {
+
+		visitor.Err = Walk(visitor, thenStmt)


we probably don't need to call Walk() here, just thenStmt.walkStmt(visitor). Same comment for others.

chengxiong-ruan · 2023-04-26T18:29:00Z

pkg/sql/sem/plpgsqltree/statements.go

@@ -115,6 +127,30 @@ func (s *PLpgSQLStmtIf) Format(ctx *tree.FmtCtx) {
 	ctx.WriteString("<NOT DONE YET>")
 }

+func (s *PLpgSQLStmtIf) walkStmt(visitor plpgsqlVisitor) {


Same, we should use interface type in the signature. same comment on others as well.

chengxiong-ruan

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @e-mbrown, @knz, @rharding6373, and @ZhouXing19)

pkg/sql/plpgsql/parser/testdata/excep_sect line 10 at r44 (raw file):

Previously, e-mbrown (Ebony Brown) wrote…

I believe this is expected. The feature name creates a tag for telemetry

feature-usage
ALTER DOMAIN foo
----
error: pq: at or near "foo": syntax error: unimplemented: this syntax
errorcodes.0A000
unimplemented.alter domain
unimplemented.syntax.alter domain

oh ok.

pkg/sql/testdata/telemetry/function line 12 at r44 (raw file):

Previously, e-mbrown (Ebony Brown) wrote…

I figured it out, turns out the error I got was unrelated lol. Done.

nice

e-mbrown

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @chengxiong-ruan, @knz, @rharding6373, and @ZhouXing19)

pkg/sql/plpgsql/parser/parse.go line 140 at r52 (raw file):

Previously, chengxiong-ruan (Chengxiong Ruan) wrote…

Heh, PLpgSQLStmtBlock should be also a statement, No? Namely, looks like you don't need to do the loop here, instead do it in the walkStmt of PLpgSQLStmtBlock?

Makes sense. Done!

pkg/sql/sem/plpgsqltree/plpg_visitor.go line 72 at r44 (raw file):

Previously, chengxiong-ruan (Chengxiong Ruan) wrote…

Even though I think it should work, I don't think this is quite right... It feels weird that you do the counting explicitly here while also have the visitor do the counting. But in general I think you're moving toward the right direction.

Ideally, there should be a StmtVisitor interface which has Visit(stmt walkableStmt) method which will be implemented to have the counting logic. Then, in this WalkStmt function, you just call stmt.walkStmt(visitor), and in stmt.walkStmt(visitor you do visitor.Visit(stmt). Then call WalkStmt on each sub statements if there is any.

I know that in the SQL world the visitor actually only visit expressions, while the statement walking walks on the statements and sub-clauses. For telemetry counting purpose, our visitor can be simple and just visit the statement without considering the expressions.

Done.

pkg/sql/sem/plpgsqltree/plpg_visitor.go line 69 at r52 (raw file):

Previously, chengxiong-ruan (Chengxiong Ruan) wrote…

We should use the visitor interface type in function signature.

Done.

pkg/sql/sem/plpgsqltree/plpg_visitor.go line 72 at r52 (raw file):

Previously, chengxiong-ruan (Chengxiong Ruan) wrote…

I don't think we'd need the isWalkable concept anymore? It means that everything statement should do wStmt.walkStmt(v) and do v.visit in it.

Done.

pkg/sql/sem/plpgsqltree/statements.go line 130 at r52 (raw file):

Previously, chengxiong-ruan (Chengxiong Ruan) wrote…

Same, we should use interface type in the signature. same comment on others as well.

Done.

pkg/sql/sem/plpgsqltree/statements.go line 135 at r52 (raw file):

Previously, chengxiong-ruan (Chengxiong Ruan) wrote…

we probably don't need to call Walk() here, just thenStmt.walkStmt(visitor). Same comment for others.

Done.

chengxiong-ruan

very nice! thanks for your patience!

knz

just a few nits and this is good to go.

Reviewed 2 of 22 files at r11, 2 of 19 files at r12, 29 of 52 files at r22, 1 of 1 files at r29, 1 of 8 files at r37, 3 of 7 files at r47, 2 of 3 files at r48, 1 of 2 files at r49, 1 of 2 files at r50, 1 of 3 files at r51, 19 of 29 files at r52, 5 of 8 files at r53, 6 of 6 files at r54, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @chengxiong-ruan, @e-mbrown, @rharding6373, and @ZhouXing19)

pkg/sql/plpgsql/parser/parse.go line 151 at r54 (raw file):

}

// DealWithPLpgSQLFunc takes a plpgsql function and parses and collects

lol, the verb "deal with" is not a thing we should expect in source code.
Can you find a better verb?

pkg/sql/plpgsql/parser/parse.go line 152 at r54 (raw file):

// DealWithPLpgSQLFunc takes a plpgsql function and parses and collects
// telemetry on the parsable statements

nit: here and below, comments should be full sentences with a capital letter at the beginning and a period at the end.

pkg/sql/plpgsql/parser/parse.go line 164 at r54 (raw file):

	if _, err := CountPLpgSQLStmt(funcBodyStr); err != nil {
		return errors.Wrap(err, "plpgsql not supported for udf")

nit: "UDF" is an acronym. Probably "not supported in user-defined functions"

pkg/sql/plpgsql/parser/plpgsql.y line 1 at r54 (raw file):

%{

nit: through this file: replace tabs with spaces.

pkg/sql/plpgsql/parser/plpgsql.y line 604 at r54 (raw file):

;

stmt_perform	: PERFORM expr_until_semi ';'

nit: remove space between rule name and :.

pkg/sql/plpgsql/parser/plpgsql.y line 895 at r54 (raw file):

  //    we can just read until a ';', then do the sql expression validation during compile time.

stmt_return : RETURN return_variable ';'

nit: space before :

e-mbrown

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @chengxiong-ruan, @knz, @rharding6373, and @ZhouXing19)

a discussion (no related file):

Previously, chengxiong-ruan (Chengxiong Ruan) wrote…

comment not added yet?

Done.

pkg/sql/plpgsql/parser/parse.go line 151 at r54 (raw file):

Previously, knz (Raphael 'kena' Poss) wrote…

lol, the verb "deal with" is not a thing we should expect in source code.
Can you find a better verb?

Done.

knz

Reviewed 10 of 10 files at r55.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @chengxiong-ruan, @e-mbrown, @knz, @rharding6373, and @ZhouXing19)

a discussion (no related file):

Previously, e-mbrown (Ebony Brown) wrote…

Done.

nice

knz

Reviewable status: complete! 1 of 0 LGTMs obtained (and 1 stale) (waiting on @chengxiong-ruan, @e-mbrown, @rharding6373, and @ZhouXing19)

pkg/sql/sem/plpgsqltree/utils/plpg_visitor.go

e-mbrown

Reviewable status: complete! 0 of 0 LGTMs obtained (and 2 stale) (waiting on @chengxiong-ruan, @knz, @rharding6373, and @ZhouXing19)

pkg/sql/sem/plpgsqltree/utils/plpg_visitor.go line 87 at r56 (raw file):

Previously, chengxiong-ruan (Chengxiong Ruan) wrote…

Walk function should stay in plpgsqltree package.

Done.

Unimplemented errors and parser test were added for `stmt_move` and `stmt_from`. Release note: None

e-mbrown · 2023-04-27T19:27:57Z

Thanks for the reviews!

bors r+

craig · 2023-04-27T20:01:16Z

Build succeeded:

Bazel Essential CI (Cockroach)

e-mbrown requested review from chengxiong-ruan, ZhouXing19 and a team March 24, 2023 19:59

e-mbrown requested a review from a team as a code owner March 24, 2023 19:59

e-mbrown requested a review from a team March 24, 2023 19:59

e-mbrown requested a review from a team as a code owner March 24, 2023 19:59

knz suggested changes Mar 27, 2023

View reviewed changes

e-mbrown commented Mar 27, 2023

View reviewed changes

e-mbrown force-pushed the eb/unimp branch from d192997 to 5278064 Compare March 28, 2023 13:39

knz reviewed Mar 28, 2023

View reviewed changes

e-mbrown force-pushed the eb/unimp branch from 5278064 to 51e9838 Compare March 30, 2023 18:42

e-mbrown requested a review from a team as a code owner March 30, 2023 18:42

e-mbrown force-pushed the eb/unimp branch from 51e9838 to 9d85c48 Compare April 4, 2023 22:32

e-mbrown force-pushed the eb/unimp branch 6 times, most recently from 9ac3500 to 9a1cb2a Compare April 20, 2023 17:27

chengxiong-ruan reviewed Apr 20, 2023

View reviewed changes

e-mbrown force-pushed the eb/unimp branch from 9a1cb2a to 7f193d0 Compare April 24, 2023 16:59

e-mbrown added 2 commits April 26, 2023 12:26

sql: Add unimplemented errors for stmt_perform

83337a5

Unimplemented errors and parser test were added for `stmt_perform`. Release note: None

sql: add unimplemented error for stmt_raise

31c38b6

Unimplemented errors and parser test were added for `stmt_raise`. Release note: None

e-mbrown force-pushed the eb/unimp branch 3 times, most recently from 4d2f992 to f323d8f Compare April 26, 2023 18:03

chengxiong-ruan reviewed Apr 26, 2023

View reviewed changes

chengxiong-ruan suggested changes Apr 26, 2023

View reviewed changes

e-mbrown force-pushed the eb/unimp branch from f323d8f to 906b3ae Compare April 26, 2023 21:00

e-mbrown commented Apr 26, 2023

View reviewed changes

e-mbrown force-pushed the eb/unimp branch from 906b3ae to 77150f0 Compare April 27, 2023 15:04

e-mbrown requested a review from chengxiong-ruan April 27, 2023 15:05

chengxiong-ruan approved these changes Apr 27, 2023

View reviewed changes

knz reviewed Apr 27, 2023

View reviewed changes

e-mbrown force-pushed the eb/unimp branch from 77150f0 to 4b405ba Compare April 27, 2023 17:45

e-mbrown commented Apr 27, 2023

View reviewed changes

e-mbrown requested a review from knz April 27, 2023 17:46

e-mbrown force-pushed the eb/unimp branch from 4b405ba to 8464603 Compare April 27, 2023 17:56

knz reviewed Apr 27, 2023

View reviewed changes

knz approved these changes Apr 27, 2023

View reviewed changes

chengxiong-ruan reviewed Apr 27, 2023

View reviewed changes

pkg/sql/sem/plpgsqltree/utils/plpg_visitor.go Outdated Show resolved Hide resolved

e-mbrown force-pushed the eb/unimp branch 2 times, most recently from 57bfe45 to fbd957e Compare April 27, 2023 18:14

e-mbrown commented Apr 27, 2023

View reviewed changes

sql: add unimplemented error for stmt_move and stmt_from

e980cb9

Unimplemented errors and parser test were added for `stmt_move` and `stmt_from`. Release note: None

e-mbrown force-pushed the eb/unimp branch from fbd957e to e980cb9 Compare April 27, 2023 18:58

craig bot merged commit c2a51cc into cockroachdb:master Apr 27, 2023

e-mbrown mentioned this pull request Apr 28, 2023

release-22.2: sql: add unimplemented errors and telemetry for plpgsql #102625

Merged

e-mbrown mentioned this pull request May 11, 2023

release-23.1: sql: add unimplemented errors and telemetry for plpgsql #103132

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sql: add unimplemented errors and telemetry for plpgsql #99517

sql: add unimplemented errors and telemetry for plpgsql #99517

e-mbrown commented Mar 24, 2023

cockroach-teamcity commented Mar 24, 2023

knz left a comment

chengxiong-ruan commented Mar 27, 2023

e-mbrown left a comment

knz commented Mar 27, 2023

knz commented Mar 27, 2023

chengxiong-ruan commented Mar 27, 2023

knz commented Mar 28, 2023

ZhouXing19 commented Mar 28, 2023 •

edited

Loading

knz commented Mar 28, 2023

chengxiong-ruan commented Mar 28, 2023 •

edited

Loading

knz left a comment

chengxiong-ruan left a comment

chengxiong-ruan Apr 20, 2023

chengxiong-ruan Apr 20, 2023

chengxiong-ruan Apr 20, 2023

chengxiong-ruan Apr 26, 2023

chengxiong-ruan Apr 26, 2023

chengxiong-ruan Apr 26, 2023

chengxiong-ruan Apr 26, 2023

chengxiong-ruan Apr 26, 2023

chengxiong-ruan Apr 26, 2023

chengxiong-ruan left a comment

e-mbrown left a comment

chengxiong-ruan left a comment

knz left a comment

e-mbrown left a comment

knz left a comment

knz left a comment

e-mbrown left a comment

e-mbrown commented Apr 27, 2023

craig bot commented Apr 27, 2023


		for _, thenStmt := range s.ThenBody {

		visitor.Err = Walk(visitor, thenStmt)

sql: add unimplemented errors and telemetry for plpgsql #99517

sql: add unimplemented errors and telemetry for plpgsql #99517

Conversation

e-mbrown commented Mar 24, 2023

cockroach-teamcity commented Mar 24, 2023

knz left a comment

Choose a reason for hiding this comment

chengxiong-ruan commented Mar 27, 2023

e-mbrown left a comment

Choose a reason for hiding this comment

knz commented Mar 27, 2023

knz commented Mar 27, 2023

chengxiong-ruan commented Mar 27, 2023

knz commented Mar 28, 2023

ZhouXing19 commented Mar 28, 2023 • edited Loading

knz commented Mar 28, 2023

chengxiong-ruan commented Mar 28, 2023 • edited Loading

knz left a comment

Choose a reason for hiding this comment

chengxiong-ruan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chengxiong-ruan left a comment

Choose a reason for hiding this comment

e-mbrown left a comment

Choose a reason for hiding this comment

chengxiong-ruan left a comment

Choose a reason for hiding this comment

knz left a comment

Choose a reason for hiding this comment

e-mbrown left a comment

Choose a reason for hiding this comment

knz left a comment

Choose a reason for hiding this comment

knz left a comment

Choose a reason for hiding this comment

e-mbrown left a comment

Choose a reason for hiding this comment

e-mbrown commented Apr 27, 2023

craig bot commented Apr 27, 2023

ZhouXing19 commented Mar 28, 2023 •

edited

Loading

chengxiong-ruan commented Mar 28, 2023 •

edited

Loading