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: v23.1.8-custom: parentheses is badly nested in ParseAndCollectTelemetryForPLpgSQLFunc #109342

Closed
cockroach-sentry opened this issue Aug 23, 2023 · 1 comment · Fixed by #112200
Closed

sql: v23.1.8-custom: parentheses is badly nested in ParseAndCollectTelemetryForPLpgSQLFunc #109342

cockroach-sentry opened this issue Aug 23, 2023 · 1 comment · Fixed by #112200
Assignees
@DrewKimball
Labels
C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. O-sentry Originated from an in-the-wild panic report.

Comments

@cockroach-sentry
Copy link
Collaborator

cockroach-sentry commented Aug 23, 2023 •
edited by cockroach-jira-scripts
Loading

This issue was auto filed by Sentry. It represents a crash or reported error on a live cluster with telemetry enabled.

Sentry Link: https://cockroach-labs.sentry.io/issues/4415320627/?referrer=webhooks_plugin

Panic Message:

lexer.go:316: parentheses is badly nested
(1) assertion failure
Wraps: (2) attached stack trace
  -- stack trace:
  | github.com/cockroachdb/cockroach/pkg/sql/plpgsql/parser.(*lexer).ReadSqlConstruct
  | 	github.com/cockroachdb/cockroach/pkg/sql/plpgsql/parser/lexer.go:316
  | github.com/cockroachdb/cockroach/pkg/sql/plpgsql/parser.(*lexer).ReadSqlExpressionStr
  | 	github.com/cockroachdb/cockroach/pkg/sql/plpgsql/parser/lexer.go:277
  | github.com/cockroachdb/cockroach/pkg/sql/plpgsql/parser.(*plpgsqlParserImpl).Parse
  | 	plpgsql-gen.y:1150
  | github.com/cockroachdb/cockroach/pkg/sql/plpgsql/parser.(*Parser).parse
  | 	github.com/cockroachdb/cockroach/pkg/sql/plpgsql/parser/parse.go:98
  | github.com/cockroachdb/cockroach/pkg/sql/plpgsql/parser.(*Parser).parseWithDepth
  | 	github.com/cockroachdb/cockroach/pkg/sql/plpgsql/parser/parse.go:82
  | github.com/cockroachdb/cockroach/pkg/sql/plpgsql/parser.Parse
  | 	github.com/cockroachdb/cockroach/pkg/sql/plpgsql/parser/parse.go:132
  | github.com/cockroachdb/cockroach/pkg/sql/sem/plpgsqltree/utils.CountPLpgSQLStmt
  | 	github.com/cockroachdb/cockroach/pkg/sql/sem/plpgsqltree/utils/plpg_visitor.go:90
  | github.com/cockroachdb/cockroach/pkg/sql/sem/plpgsqltree/utils.ParseAndCollectTelemetryForPLpgSQLFunc
  | 	github.com/cockroachdb/cockroach/pkg/sql/sem/plpgsqltree/utils/plpg_visitor.go:112
  | github.com/cockroachdb/cockroach/pkg/sql/opt/optbuilder.(*Builder).buildCreateFunction
  | 	github.com/cockroachdb/cockroach/pkg/sql/opt/optbuilder/create_function.go:111
  | github.com/cockroachdb/cockroach/pkg/sql/opt/optbuilder.(*Builder).buildStmt
  | 	github.com/cockroachdb/cockroach/pkg/sql/opt/optbuilder/builder.go:359
  | github.com/cockroachdb/cockroach/pkg/sql/opt/optbuilder.(*Builder).buildStmtAtRoot
  | 	github.com/cockroachdb/cockroach/pkg/sql/opt/optbuilder/builder.go:272
  | github.com/cockroachdb/cockroach/pkg/sql/opt/optbuilder.(*Builder).Build
  | 	github.com/cockroachdb/cockroach/pkg/sql/opt/optbuilder/builder.go:246
  | github.com/cockroachdb/cockroach/pkg/sql.(*optPlanningCtx).buildExecMemo
  | 	github.com/cockroachdb/cockroach/pkg/sql/plan_opt.go:576
  | github.com/cockroachdb/cockroach/pkg/sql.(*planner).makeOptimizerPlan
  | 	github.com/cockroachdb/cockroach/pkg/sql/plan_opt.go:240
  | github.com/cockroachdb/cockroach/pkg/sql.(*connExecutor).makeExecPlan
  | 	github.com/cockroachdb/cockroach/pkg/sql/conn_executor_exec.go:1978
  | github.com/cockroachdb/cockroach/pkg/sql.(*connExecutor).dispatchToExecutionEngine
  | 	github.com/cockroachdb/cockroach/pkg/sql/conn_executor_exec.go:1484
  | github.com/cockroachdb/cockroach/pkg/sql.(*connExecutor).execStmtInOpenState
  | 	github.com/cockroachdb/cockroach/pkg/sql/conn_executor_exec.go:964
  | github.com/cockroachdb/cockroach/pkg/sql.(*connExecutor).execStmt.func1
  | 	github.com/cockroachdb/cockroach/pkg/sql/conn_executor_exec.go:142
  | github.com/cockroachdb/cockroach/pkg/sql.(*connExecutor).execWithProfiling
  | 	github.com/cockroachdb/cockroach/pkg/sql/conn_executor_exec.go:2986
  | github.com/cockroachdb/cockroach/pkg/sql.(*connExecutor).execStmt
  | 	github.com/cockroachdb/cockroach/pkg/sql/conn_executor_exec.go:141
  | github.com/cockroachdb/cockroach/pkg/sql.(*connExecutor).execCmd.func1
  | 	github.com/cockroachdb/cockroach/pkg/sql/conn_executor.go:2181
  | github.com/cockroachdb/cockroach/pkg/sql.(*connExecutor).execCmd
  | 	github.com/cockroachdb/cockroach/pkg/sql/conn_executor.go:2186
  | github.com/cockroachdb/cockroach/pkg/sql.(*connExecutor).run
  | 	github.com/cockroachdb/cockroach/pkg/sql/conn_executor.go:2103
  | github.com/cockroachdb/cockroach/pkg/sql.(*Server).ServeConn
  | 	github.com/cockroachdb/cockroach/pkg/sql/conn_executor.go:900
  | github.com/cockroachdb/cockroach/pkg/sql/pgwire.(*conn).processCommandsAsync.func1
  | 	github.com/cockroachdb/cockroach/pkg/sql/pgwire/conn.go:310
  | runtime.goexit
  | 	GOROOT/src/runtime/asm_amd64.s:1594
Wraps: (3) parentheses is badly nested
Error types: (1) *assert.withAssertionFailure (2) *withstack.withStack (3) *errutil.leafError
-- report composition:
*errutil.leafError: parentheses is badly nested
lexer.go:316: *withstack.withStack (top exception)
*assert.withAssertionFailure
Stacktrace (expand for inline code snippets):

GOROOT/src/runtime/asm_amd64.s#L1593-L1595

cockroach/pkg/sql/pgwire/conn.go

Lines 309 to 311 in c2fa404

reservedOwned = false // We're about to pass ownership away.
retErr = sqlServer.ServeConn(
ctx,

cockroach/pkg/sql/conn_executor.go

Lines 899 to 901 in c2fa404

}(ctx, h)
return h.ex.run(ctx, s.pool, reserved, cancel)
}

cockroach/pkg/sql/conn_executor.go

Lines 2102 to 2104 in c2fa404

var err error
if err = ex.execCmd(); err != nil {
// Both of these errors are normal ways for the connExecutor to exit.

cockroach/pkg/sql/conn_executor.go

Lines 2185 to 2187 in c2fa404

return err
}()
// Note: we write to ex.statsCollector.PhaseTimes, instead of ex.phaseTimes,

cockroach/pkg/sql/conn_executor.go

Lines 2180 to 2182 in c2fa404

tcmd.LastInBatch || !implicitTxnForBatch)
ev, payload, err = ex.execStmt(
ctx, tcmd.Statement, nil /* portal */, nil /* pinfo */, stmtRes, canAutoCommit,

cockroach/pkg/sql/conn_executor_exec.go

Lines 140 to 142 in c2fa404

}
err = ex.execWithProfiling(ctx, ast, preparedStmt, func(ctx context.Context) error {
ev, payload, err = ex.execStmtInOpenState(ctx, parserStmt, portal, pinfo, res, canAutoCommit)

cockroach/pkg/sql/conn_executor_exec.go

Lines 2985 to 2987 in c2fa404

} else {
err = op(ctx)
}

cockroach/pkg/sql/conn_executor_exec.go

Lines 141 to 143 in c2fa404

err = ex.execWithProfiling(ctx, ast, preparedStmt, func(ctx context.Context) error {
ev, payload, err = ex.execStmtInOpenState(ctx, parserStmt, portal, pinfo, res, canAutoCommit)
return err

cockroach/pkg/sql/conn_executor_exec.go

Lines 963 to 965 in c2fa404

if err = ex.dispatchToExecutionEngine(stmtCtx, p, res); err != nil {
stmtThresholdSpan.Finish()

cockroach/pkg/sql/conn_executor_exec.go

Lines 1483 to 1485 in c2fa404

// between here and there needs to happen even if there's an error.
err = ex.makeExecPlan(ctx, planner)
defer planner.curPlan.close(ctx)

cockroach/pkg/sql/conn_executor_exec.go

Lines 1977 to 1979 in c2fa404

func (ex *connExecutor) makeExecPlan(ctx context.Context, planner *planner) error {
if err := planner.makeOptimizerPlan(ctx); err != nil {
log.VEventf(ctx, 1, "optimizer plan failed: %v", err)

cockroach/pkg/sql/plan_opt.go

Lines 239 to 241 in c2fa404

execMemo, err := opc.buildExecMemo(ctx)
if err != nil {

cockroach/pkg/sql/plan_opt.go

Lines 575 to 577 in c2fa404

bld := optbuilder.New(ctx, &p.semaCtx, p.EvalContext(), opc.catalog, f, opc.p.stmt.AST)
if err := bld.Build(); err != nil {
return nil, err

cockroach/pkg/sql/opt/optbuilder/builder.go

Lines 245 to 247 in c2fa404

// and physical properties.
outScope := b.buildStmtAtRoot(b.stmt, nil /* desiredTypes */)

cockroach/pkg/sql/opt/optbuilder/builder.go

Lines 271 to 273 in c2fa404

b.ctes = nil
outScope = b.buildStmt(stmt, desiredTypes, inScope)
// Build With operators for any CTEs hoisted to the top level.

cockroach/pkg/sql/opt/optbuilder/builder.go

Lines 358 to 360 in c2fa404

case *tree.CreateFunction:
return b.buildCreateFunction(stmt, inScope)

cockroach/pkg/sql/opt/optbuilder/create_function.go

Lines 110 to 112 in c2fa404

if opt == tree.FunctionLangPLpgSQL {
if err := utils.ParseAndCollectTelemetryForPLpgSQLFunc(cf); err != nil {
// Until plpgsql is fully implemented DealWithPlpgSQlFunc will always

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

Lines 111 to 113 in c2fa404

if _, err := CountPLpgSQLStmt(funcBodyStr); err != nil {
return errors.Wrap(err, "plpgsql not supported in user-defined functions")

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

Lines 89 to 91 in c2fa404

v := MakePLpgSQLTelemetryVisitor()
stmt, err := parser.Parse(sql)
if err != nil {

cockroach/pkg/sql/plpgsql/parser/parse.go

Lines 131 to 133 in c2fa404

var p Parser
return p.parseWithDepth(1, sql, defaultNakedIntType)
}

cockroach/pkg/sql/plpgsql/parser/parse.go

Lines 81 to 83 in c2fa404

sql, tokens, done := p.scanFnBlock()
stmt, err := p.parse(depth+1, sql, tokens, nakedIntType)
if err != nil {

cockroach/pkg/sql/plpgsql/parser/parse.go

Lines 97 to 99 in c2fa404

defer p.lexer.cleanup()
if p.parserImpl.Parse(&p.lexer) != 0 {
if p.lexer.lastError == nil {

plpgsql-gen.y#L1149-L1151

cockroach/pkg/sql/plpgsql/parser/lexer.go

Lines 276 to 278 in c2fa404

func (l *lexer) ReadSqlExpressionStr(terminator int) (sqlStr string) {
sqlStr, _ = l.ReadSqlConstruct(terminator, 0, 0)
return sqlStr

cockroach/pkg/sql/plpgsql/parser/lexer.go

Lines 315 to 317 in c2fa404

if parenLevel != 0 {
panic(errors.AssertionFailedf("parentheses is badly nested"))
} 

GOROOT/src/runtime/asm_amd64.s in runtime.goexit at line 1594
pkg/sql/pgwire/conn.go in pkg/sql/pgwire.(*conn).processCommandsAsync.func1 at line 310
pkg/sql/conn_executor.go in pkg/sql.(*Server).ServeConn at line 900
pkg/sql/conn_executor.go in pkg/sql.(*connExecutor).run at line 2103
pkg/sql/conn_executor.go in pkg/sql.(*connExecutor).execCmd at line 2186
pkg/sql/conn_executor.go in pkg/sql.(*connExecutor).execCmd.func1 at line 2181
pkg/sql/conn_executor_exec.go in pkg/sql.(*connExecutor).execStmt at line 141
pkg/sql/conn_executor_exec.go in pkg/sql.(*connExecutor).execWithProfiling at line 2986
pkg/sql/conn_executor_exec.go in pkg/sql.(*connExecutor).execStmt.func1 at line 142
pkg/sql/conn_executor_exec.go in pkg/sql.(*connExecutor).execStmtInOpenState at line 964
pkg/sql/conn_executor_exec.go in pkg/sql.(*connExecutor).dispatchToExecutionEngine at line 1484
pkg/sql/conn_executor_exec.go in pkg/sql.(*connExecutor).makeExecPlan at line 1978
pkg/sql/plan_opt.go in pkg/sql.(*planner).makeOptimizerPlan at line 240
pkg/sql/plan_opt.go in pkg/sql.(*optPlanningCtx).buildExecMemo at line 576
pkg/sql/opt/optbuilder/builder.go in pkg/sql/opt/optbuilder.(*Builder).Build at line 246
pkg/sql/opt/optbuilder/builder.go in pkg/sql/opt/optbuilder.(*Builder).buildStmtAtRoot at line 272
pkg/sql/opt/optbuilder/builder.go in pkg/sql/opt/optbuilder.(*Builder).buildStmt at line 359
pkg/sql/opt/optbuilder/create_function.go in pkg/sql/opt/optbuilder.(*Builder).buildCreateFunction at line 111
pkg/sql/sem/plpgsqltree/utils/plpg_visitor.go in pkg/sql/sem/plpgsqltree/utils.ParseAndCollectTelemetryForPLpgSQLFunc at line 112
pkg/sql/sem/plpgsqltree/utils/plpg_visitor.go in pkg/sql/sem/plpgsqltree/utils.CountPLpgSQLStmt at line 90
pkg/sql/plpgsql/parser/parse.go in pkg/sql/plpgsql/parser.Parse at line 132
pkg/sql/plpgsql/parser/parse.go in pkg/sql/plpgsql/parser.(*Parser).parseWithDepth at line 82
pkg/sql/plpgsql/parser/parse.go in pkg/sql/plpgsql/parser.(*Parser).parse at line 98
plpgsql-gen.y in pkg/sql/plpgsql/parser.(*plpgsqlParserImpl).Parse at line 1150
pkg/sql/plpgsql/parser/lexer.go in pkg/sql/plpgsql/parser.(*lexer).ReadSqlExpressionStr at line 277
pkg/sql/plpgsql/parser/lexer.go in pkg/sql/plpgsql/parser.(*lexer).ReadSqlConstruct at line 316

Tags

Tag Value
Command mt start-sql
Environment v23.1.8
Go Version go1.19.10
Platform linux amd64
Distribution CCL
Cockroach Release v23.1.8-2-gc2fa404f72
Cockroach SHA c2fa404
# of CPUs 32
# of Goroutines 330

Jira issue: CRDB-30884
@cockroach-sentry cockroach-sentry added O-sentry Originated from an in-the-wild panic report. C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. labels Aug 23, 2023
@yuzefovich yuzefovich changed the title Sentry: lexer.go:316: parentheses is badly nested (1) assertion failure Wraps: (2) attached stack trace -- stack trace: | github.com/cockroachdb/cockroach/pkg/sql/plpgsql/parser.(*lexer).ReadSqlCo... sql: v23.1.8-custom: parentheses is badly nested in ParseAndCollectTelemetryForPLpgSQLFunc Aug 23, 2023
@github-project-automation github-project-automation bot moved this to Triage in SQL Queries Aug 23, 2023
@mgartner
Copy link
Collaborator

mgartner commented Sep 5, 2023

This looks like a syntax error that is being incorrect raised as an internal error and not a user error.

@mgartner mgartner moved this from Triage to 23.2 Release in SQL Queries Sep 5, 2023
@DrewKimball DrewKimball moved this from 23.2 Release to Active in SQL Queries Oct 10, 2023
DrewKimball added a commit to DrewKimball/cockroach that referenced this issue Oct 11, 2023
@DrewKimball
plpgsql: handle multiple expressions when one expression is expected
066f164 
Previously, the PLpgSQL parser could panic when the user supplied more
than one expression in a location where only one was expected, for example,
in a return statement. This was because the PLpgSQL parser delegated to
the SQL parser's `ParseExpr` function, which expects exactly one input
expression. This commit returns a syntax error instead of the panic by
switching to use `ParseExprs`, which can handle multiple input expressions.

Informs cockroachdb#109342

Release note: None
DrewKimball added a commit to DrewKimball/cockroach that referenced this issue Oct 11, 2023
@DrewKimball
plpgsql: return correct error for invalid parantheses and missing exp…
00ffa21 
…ression

This patch fixes error messages in the PLpgSQL parser for the case when
the parenthesis nesting is invalid, and for the case when no expression
(or statement) is supplied. Previously, invalid parentheses would cause
a panic without an error code, and a missing expression had the incorrect
message, since it wasn't checked until the SQL parser attempted to read
an empty string. Now, both cases are checked immediately by the PLpgSQL
parser and the correct error is propagated.

Fixes cockroachdb#109342

Release note: None
@DrewKimball DrewKimball mentioned this issue Oct 11, 2023
Merged
DrewKimball added a commit to DrewKimball/cockroach that referenced this issue Oct 16, 2023
@DrewKimball
plpgsql: handle multiple expressions when one expression is expected
7f40d3f 
Previously, the PLpgSQL parser could panic when the user supplied more
than one expression in a location where only one was expected, for example,
in a return statement. This was because the PLpgSQL parser delegated to
the SQL parser's `ParseExpr` function, which expects exactly one input
expression. This commit returns a syntax error instead of the panic by
switching to use `ParseExprs`, which can handle multiple input expressions.

Informs cockroachdb#109342

Release note: None
craig bot pushed a commit that referenced this issue Oct 17, 2023
@DrewKimball
Merge #112200
45f6344 
112200: plpgsql: correctly handle parsing errors r=DrewKimball a=DrewKimball

#### plpgsql: correctly handle parsing errors

This patch ensures that PLpgSQL parsing errors are correctly propagated
in all cases. Previously, there were a few cases (like variable declaration
type parsing) where an error didn't halt parsing. The contract for
`GetTypeFromValidSQLSyntax` is also clarified, since it is ok to call with
an invalid type name as long as the error is properly handled.

Informs #105254

Release note: None

#### plpgsql: handle multiple expressions when one expression is expected

Previously, the PLpgSQL parser could panic when the user supplied more
than one expression in a location where only one was expected, for example,
in a return statement. This was because the PLpgSQL parser delegated to
the SQL parser's `ParseExpr` function, which expects exactly one input
expression. This commit returns a syntax error instead of the panic by
switching to use `ParseExprs`, which can handle multiple input expressions.

Informs #109342

Release note: None

#### plpgsql: return correct error for invalid parantheses and missing expression

This patch fixes error messages in the PLpgSQL parser for the case when
the parenthesis nesting is invalid, and for the case when no expression
(or statement) is supplied. Previously, invalid parentheses would cause
a panic without an error code, and a missing expression had the incorrect
message, since it wasn't checked until the SQL parser attempted to read
an empty string. Now, both cases are checked immediately by the PLpgSQL
parser and the correct error is propagated.

Fixes #109342

Release note: None

Co-authored-by: Drew Kimball <[email protected]>
@craig craig bot closed this as completed in 6e331e6 Oct 17, 2023
@github-project-automation github-project-automation bot moved this from Active to Done in SQL Queries Oct 17, 2023
blathers-crl bot pushed a commit that referenced this issue Oct 17, 2023
@DrewKimball
plpgsql: handle multiple expressions when one expression is expected
04fc528 
Previously, the PLpgSQL parser could panic when the user supplied more
than one expression in a location where only one was expected, for example,
in a return statement. This was because the PLpgSQL parser delegated to
the SQL parser's `ParseExpr` function, which expects exactly one input
expression. This commit returns a syntax error instead of the panic by
switching to use `ParseExprs`, which can handle multiple input expressions.

Informs #109342

Release note: None
blathers-crl bot pushed a commit that referenced this issue Oct 17, 2023
@DrewKimball
plpgsql: return correct error for invalid parantheses and missing exp…
16e9564 
…ression

This patch fixes error messages in the PLpgSQL parser for the case when
the parenthesis nesting is invalid, and for the case when no expression
(or statement) is supplied. Previously, invalid parentheses would cause
a panic without an error code, and a missing expression had the incorrect
message, since it wasn't checked until the SQL parser attempted to read
an empty string. Now, both cases are checked immediately by the PLpgSQL
parser and the correct error is propagated.

Fixes #109342

Release note: None
@blathers-crl blathers-crl bot mentioned this issue Oct 17, 2023
Merged
@yuzefovich yuzefovich mentioned this issue Jun 27, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@DrewKimball DrewKimball

Labels
C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. O-sentry Originated from an in-the-wild panic report.
Projects
SQL Queries
Archived in project
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

plpgsql: correctly handle parsing errors DrewKimball/cockroach
3 participants
@mgartner @DrewKimball @cockroach-sentry