Skip to content

Commit e38ecc2

Browse files
committed
plpgsql: prevent a crash in an edge case during formatting
This commit fixes an issue that is similar to the one that was fixed in 22dabb0. In particular, each parser (both SQL one and PLpgSQL one) keeps track of the number of annotations found in the current stmt. Annotations are used during object name resolution, and the AST nodes can reference annotation indices. If an annotation index is set, then it assumes that during formatting the corresponding annotation will be available in the supplied Annotations container. In the PLpgSQL parser we use an ephemeral SQL parser instance to process SQL statements. Previously, we would simply discard the information about whether any annotations were found in the SQL stmt which could lead to undefined behavior down the line. This commit audits all usages of the SQL parser from PLpgSQL one to ensure that we capture the number of annotations used in that stmt and create the Annotations container with that capacity, stored in the AST. This container is then substituted into the FmtCtx for those ASTs. Note that since we currently don't support DDLs from PLpgSQL, the annotation slots in the container won't ever be set, so the contribution of this patch is preventing the crash. Release note (bug fix): Previously, CockroachDB node could crash when executing DO stmts when they contain currently unsupported DDL stmts like CREATE TYPE in non-default configuration (additional logging like the one controlled via `sql.log.all_statements.enabled` cluster setting needed to be enabled). This bug was introduced in 25.1 release and is now fixed.
1 parent dbc8fba commit e38ecc2

File tree

7 files changed

+191
-94
lines changed

7 files changed

+191
-94
lines changed

pkg/sql/logictest/testdata/logic_test/do

+13
Original file line numberDiff line numberDiff line change
@@ -274,3 +274,16 @@ CALL p();
274274
NOTICE: Hello, world!
275275

276276
subtest end
277+
278+
subtest regression_143974
279+
280+
statement error pgcode 0A000 unimplemented: CREATE TYPE usage inside a function definition.*\n.*\n.*issue-v/110080
281+
DO $$
282+
BEGIN
283+
IF NOT EXISTS (SELECT 1 FROM pg_type WHERE typname = 'mood') THEN
284+
CREATE TYPE mood AS ENUM ('happy', 'sad', 'neutral');
285+
END IF;
286+
END
287+
$$;
288+
289+
subtest end

pkg/sql/parser/testdata/do

+42
Original file line numberDiff line numberDiff line change
@@ -364,3 +364,45 @@ at or near "EOF": syntax error
364364
DETAIL: source SQL:
365365
BEGIN DO
366366
^
367+
368+
parse
369+
DO $$
370+
BEGIN
371+
IF NOT EXISTS (SELECT 1 FROM pg_type WHERE typname = 'mood') THEN
372+
CREATE TYPE mood AS ENUM ('happy', 'sad', 'neutral');
373+
END IF;
374+
END
375+
$$;
376+
----
377+
DO $$
378+
BEGIN
379+
IF NOT EXISTS (SELECT 1 FROM pg_type WHERE typname = 'mood') THEN
380+
CREATE TYPE mood AS ENUM ('happy', 'sad', 'neutral');
381+
END IF;
382+
END;
383+
$$;
384+
-- normalized!
385+
DO $$
386+
BEGIN
387+
IF (NOT (EXISTS (SELECT (1) FROM pg_type WHERE ((typname) = ('mood'))))) THEN
388+
CREATE TYPE mood AS ENUM ('happy', 'sad', 'neutral');
389+
END IF;
390+
END;
391+
$$;
392+
-- fully parenthesized
393+
DO $$
394+
BEGIN
395+
IF NOT EXISTS (SELECT _ FROM pg_type WHERE typname = '_') THEN
396+
CREATE TYPE mood AS ENUM ('happy', 'sad', 'neutral');
397+
END IF;
398+
END;
399+
$$;
400+
-- literals removed
401+
DO $$
402+
BEGIN
403+
IF NOT EXISTS (SELECT 1 FROM _ WHERE _ = 'mood') THEN
404+
CREATE TYPE _ AS ENUM (_, _, _);
405+
END IF;
406+
END;
407+
$$;
408+
-- identifiers removed

pkg/sql/plpgsql/parser/lexer.go

+15-7
Original file line numberDiff line numberDiff line change
@@ -181,10 +181,12 @@ func (l *lexer) MakeExecSqlStmt() (*plpgsqltree.Execute, error) {
181181
if target != nil && sqlStmt.AST.StatementReturnType() != tree.Rows {
182182
return nil, pgerror.New(pgcode.Syntax, "INTO used with a command that cannot return data")
183183
}
184+
ann := tree.MakeAnnotations(sqlStmt.NumAnnotations)
184185
return &plpgsqltree.Execute{
185-
SqlStmt: sqlStmt.AST,
186-
Strict: haveStrict,
187-
Target: target,
186+
SqlStmt: sqlStmt.AST,
187+
Strict: haveStrict,
188+
Target: target,
189+
Annotations: &ann,
188190
}, nil
189191
}
190192

@@ -295,10 +297,12 @@ func (l *lexer) MakeFetchOrMoveStmt(isMove bool) (plpgsqltree.Statement, error)
295297
}
296298
// Move past the semicolon.
297299
l.lastPos++
300+
ann := tree.MakeAnnotations(sqlStmt.NumAnnotations)
298301
return &plpgsqltree.Fetch{
299-
Cursor: cursor,
300-
Target: target,
301-
IsMove: isMove,
302+
Cursor: cursor,
303+
Target: target,
304+
IsMove: isMove,
305+
Annotations: &ann,
302306
}, nil
303307
}
304308

@@ -396,7 +400,11 @@ func (l *lexer) ParseReturnQuery() (plpgsqltree.Statement, error) {
396400
if err != nil {
397401
return nil, err
398402
}
399-
return &plpgsqltree.ReturnQuery{SqlStmt: stmt.AST}, nil
403+
ann := tree.MakeAnnotations(stmt.NumAnnotations)
404+
return &plpgsqltree.ReturnQuery{
405+
SqlStmt: stmt.AST,
406+
Annotations: &ann,
407+
}, nil
400408
}
401409

402410
// peekForExecute checks whether the next token is EXECUTE, used to identify

pkg/sql/plpgsql/parser/plpgsql.y

+11-2
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import (
66

77
"github.com/cockroachdb/cockroach/pkg/build"
88
"github.com/cockroachdb/cockroach/pkg/sql/parser"
9+
"github.com/cockroachdb/cockroach/pkg/sql/parser/statements"
910
"github.com/cockroachdb/cockroach/pkg/sql/scanner"
1011
"github.com/cockroachdb/cockroach/pkg/sql/sem/tree"
1112
"github.com/cockroachdb/cockroach/pkg/sql/sem/plpgsqltree"
@@ -173,7 +174,11 @@ func (u *plpgsqlSymUnion) cursorScrollOption() tree.CursorScrollOption {
173174
}
174175

175176
func (u *plpgsqlSymUnion) sqlStatement() tree.Statement {
176-
return u.val.(tree.Statement)
177+
return u.val.(statements.Statement[tree.Statement]).AST
178+
}
179+
180+
func (u *plpgsqlSymUnion) numAnnotations() tree.AnnotationIdx {
181+
return u.val.(statements.Statement[tree.Statement]).NumAnnotations
177182
}
178183

179184
func (u *plpgsqlSymUnion) variables() []plpgsqltree.Variable {
@@ -479,10 +484,12 @@ decl_statement: decl_varname decl_const decl_datatype decl_collate decl_notnull
479484
}
480485
| decl_varname opt_scrollable CURSOR decl_cursor_args decl_is_for decl_cursor_query
481486
{
487+
ann := tree.MakeAnnotations($6.numAnnotations())
482488
$$.val = &plpgsqltree.CursorDeclaration{
483489
Name: plpgsqltree.Variable($1),
484490
Scroll: $2.cursorScrollOption(),
485491
Query: $6.sqlStatement(),
492+
Annotations: &ann,
486493
}
487494
}
488495
;
@@ -510,7 +517,7 @@ decl_cursor_query: stmt_until_semi ';'
510517
if len(stmts) != 1 {
511518
return setErr(plpgsqllex, errors.New("expected exactly one SQL statement for cursor"))
512519
}
513-
$$.val = stmts[0].AST
520+
$$.val = stmts[0]
514521
}
515522
;
516523

@@ -1412,10 +1419,12 @@ stmt_open: OPEN IDENT ';'
14121419
if len(stmts) != 1 {
14131420
return setErr(plpgsqllex, errors.New("expected exactly one SQL statement for cursor"))
14141421
}
1422+
ann := tree.MakeAnnotations(stmts[0].NumAnnotations)
14151423
$$.val = &plpgsqltree.Open{
14161424
CurVar: plpgsqltree.Variable($2),
14171425
Scroll: $3.cursorScrollOption(),
14181426
Query: stmts[0].AST,
1427+
Annotations: &ann,
14191428
}
14201429
}
14211430
;

0 commit comments

Comments
 (0)