roachtest: sqlsmith: index out of range in schema changer with CREATE FUNCTION

[cockroach-teamcity]

roachtest.sqlsmith/setup=rand-tables/setting=no-mutations failed with artifacts on master @ bf25b7bf93a6b8e57557813957da263389796bb4:

test artifacts and logs in: /artifacts/sqlsmith/setup=rand-tables/setting=no-mutations/run_1
(sqlsmith.go:248).func3: error: pq: internal error: building declarative schema change targets for CREATE FUNCTION: runtime error: index out of range [1] with length 1
stmt:
CREATE FUNCTION "fun""c159"(IN p0 FLOAT8, IN p1 NAME)
	RETURNS RECORD
	NOT LEAKPROOF
	LANGUAGE SQL
	AS $funcbody$SELECT NULL AS col10641, 0.02274592984327417:::FLOAT8 AS col10642 FROM defaultdb.public."table\\U000A6F2A2"@"table\\U000A6F2A2_co}l2_1_%vcol2😊_4_col2_0_col2_2_co%ql2 _3_key" AS tab4292 WHERE NULL::rand_typ_0 IN () LIMIT 95:::INT8;
SELECT (true,) AS col10643 FROM defaultdb.public.table4@"table4_expr_col4_0_col4\\u00D8_*4_col4_1_c ol/4_2_key" AS " t\\U000BEE29ab�4293", defaultdb.public."t	able3"@[0] AS t😶ab😨4294 WHERE NULL ORDER BY " t\\U000BEE29ab�4293".col4_0 DESC NULLS FIRST, t😶ab😨4294.col3😋_0 NULLS FIRST, t😶ab😨4294.col3_13 DESC LIMIT 91:::INT8$funcbody$;

Parameters: ROACHTEST_cloud=gce , ROACHTEST_cpu=4 , ROACHTEST_encrypted=false , ROACHTEST_ssd=0

Help

See: roachtest README

See: How To Investigate (internal)

/cc @cockroachdb/sql-queries

This test on roachdash | Improve this report!

Jira issue: CRDB-28424

[mgartner]

Here's a reduced reproduction:

CREATE TABLE t (a INT);

CREATE TYPE typ AS ENUM ('foo');

CREATE FUNCTION f() RETURNS INT LANGUAGE SQL AS $$
  SELECT 1 FROM t WHERE NULL::typ IN ()
$$;

It reproduces on v23.1.2 and master. It does not reproduce on v22.2.2.

Stack trace:

ERROR: internal error: building declarative schema change targets for CREATE FUNCTION: runtime error: index out of range [1] with length 1
SQLSTATE: XX000
DETAIL: stack trace:
github.com/cockroachdb/cockroach/pkg/sql/schemachanger/scerrors/errors.go:81: HandlePanicAndLogError()
GOROOT/src/runtime/panic.go:884: gopanic()
GOROOT/src/runtime/panic.go:113: goPanicIndex()
github.com/cockroachdb/cockroach/pkg/sql/sem/tree/annotation.go:53: Get()
github.com/cockroachdb/cockroach/pkg/sql/sem/tree/annotation.go:30: GetAnnotation()
github.com/cockroachdb/cockroach/pkg/sql/sem/tree/object_name.go:180: Resolved()
github.com/cockroachdb/cockroach/pkg/sql/sem/tree/object_name.go:201: Format()
github.com/cockroachdb/cockroach/pkg/sql/sem/tree/format.go:692: formatNodeMaybeMarkRedaction()
github.com/cockroachdb/cockroach/pkg/sql/sem/tree/format.go:466: FormatNode()
github.com/cockroachdb/cockroach/pkg/sql/sem/tree/type_name.go:213: FormatTypeReference()
github.com/cockroachdb/cockroach/pkg/sql/sem/tree/expr.go:1509: Format()
github.com/cockroachdb/cockroach/pkg/sql/sem/tree/format.go:692: formatNodeMaybeMarkRedaction()
github.com/cockroachdb/cockroach/pkg/sql/sem/tree/format.go:466: FormatNode()
github.com/cockroachdb/cockroach/pkg/sql/sem/tree/expr.go:135: exprFmtWithParen()
github.com/cockroachdb/cockroach/pkg/sql/sem/tree/expr.go:168: binExprFmtWithParen()
github.com/cockroachdb/cockroach/pkg/sql/sem/tree/expr.go:392: Format()
github.com/cockroachdb/cockroach/pkg/sql/sem/tree/format.go:692: formatNodeMaybeMarkRedaction()
github.com/cockroachdb/cockroach/pkg/sql/sem/tree/format.go:466: FormatNode()
github.com/cockroachdb/cockroach/pkg/sql/sem/tree/select.go:745: Format()
github.com/cockroachdb/cockroach/pkg/sql/sem/tree/format.go:692: formatNodeMaybeMarkRedaction()
github.com/cockroachdb/cockroach/pkg/sql/sem/tree/format.go:466: FormatNode()
github.com/cockroachdb/cockroach/pkg/sql/sem/tree/select.go:125: Format()
github.com/cockroachdb/cockroach/pkg/sql/sem/tree/format.go:692: formatNodeMaybeMarkRedaction()
github.com/cockroachdb/cockroach/pkg/sql/sem/tree/format.go:466: FormatNode()
github.com/cockroachdb/cockroach/pkg/sql/sem/tree/select.go:57: Format()
github.com/cockroachdb/cockroach/pkg/sql/sem/tree/format.go:692: formatNodeMaybeMarkRedaction()
github.com/cockroachdb/cockroach/pkg/sql/sem/tree/format.go:466: FormatNode()
github.com/cockroachdb/cockroach/pkg/sql/sem/tree/udf.go:134: Format()
github.com/cockroachdb/cockroach/pkg/sql/sem/tree/format.go:692: formatNodeMaybeMarkRedaction()
github.com/cockroachdb/cockroach/pkg/sql/sem/tree/format.go:466: FormatNode()
github.com/cockroachdb/cockroach/pkg/sql/schemachanger/scbuild/ast_annotator.go:87: ValidateAnnotations()
github.com/cockroachdb/cockroach/pkg/sql/schemachanger/scbuild/build.go:86: Build()

[mgartner]

Passing to SQL Foundations to take a look. Feel free to pass back if you think this belongs to SQL Queries.

[rafiss]

Seems similar to #103541. We didn't have a repro before, so we can take a closer look now.

[rafiss]

It looks like the issue is with resolving the type name typ (since the stack trace shows that the error happens when formatting the IN binary expr).

@Xiang-Gu can you take a look at this panic? If you need help, perhaps you can work with @chengxiong-ruan since he's been in this area recently.

[mgartner]

@Xiang-Gu this failure is pretty noisy. Is there a way we can prevent our random SQL tests from hitting this until a fix is merged? Is there something we can do that's less heavy-handed then disabling CREATE FUNCTION statements entirely?

[Xiang-Gu]

looking at it

[Xiang-Gu]

When we parse a create function f … string, the returned stmt usually has n.NumAnnotations = 1 and that annotation is for f. But the function body itself can contain many descriptor references which would result in a larger NumAnnotations if we were just to parse the function body string (e.g. parsing select 1 from t where null::typ in () returns a stmt with nb.NumAnnotations=2).

This difference can cause format(n) to fail because formatting a create function AST will recursively format its BodyStatements which can be an AnnotatedNode with a larger AnnotationIdx than n.NumAnnotation. This leads to out-of-bound panic as we saw in this issue.

Unsurprisingly, for the provided repro, the legacy schema changer also panics with the same error but under a different code path (whenever format(n) is called will trigger this panic).

This issue overall seems to be a minor inconvenience of a few moving pieces: how we chose to treat function body as a string in a create function stmt, how parsing "ignores" the body when it comes NumAnnotations, and finally how format(n) uses annotations.

As for fixes, I don't think I am going to touch any parser code (although it might seem tempting to fix the root cause; I am not familiar with sql.y and sql.go and I felt this is not easy). Instead, I am going to overwrite n.NumAnnotations to be max(n.NumAnnotations, parse(funcBody).NumAnnotations) in the code paths to create a function in both legacy and declarative schema changer.

[Xiang-Gu]

It seems rather straightforward to fix the declarative schema changer case. Namely, in declarative schema changer, we cloned and operated a new annotation on the create function here

cockroach/pkg/sql/schemachanger/scbuild/ast_annotator.go

Line 41 in af730ee

annotation: tree.MakeAnnotations(statement.NumAnnotations),

It will suffice if we squeezed in some logic here to parse the function body and overwrite statement.NumAnnotations to be max(statement.NumAnnotations, max(funcBodyStmt.NumAnnotations within funcBodyStmts)), and thus initializing a "long" enough annotations slice.

What puzzles me now is how to fix the legacy schema changer case where things seems to be more "coupled". Namely, I learned that annotations are stored in semaCtx.Annotations in a *planner. The issue occurs, in the legacy schema changer, when it attempts to log the CREATE FUNCTION event, which in turns attempts to format this CREATE FUNCTION node, which consults semaCtx.Annotations (in declarative schema changer, it consults a cloned annotation which is initialized from the overwritten NumAnnotations). It seems to me it's now a little hard/rigid to overwrite semaCtx.Annotations because it's been initialized before.

[Xiang-Gu]

I will think of it a bit more, but I will fine for short-term mitigation by fixing only the declarative schema changer to silence those test failures.

[Xiang-Gu]

@rafiss @mgartner Any thoughts you all might have?

[DrewKimball]

It will suffice if we squeezed in some logic here to parse the function body and overwrite statement.NumAnnotations to be max(statement.NumAnnotations, max(funcBodyStmt.NumAnnotations within funcBodyStmts)), and thus initializing a "long" enough annotations slice.

I'm not sure this works - even if it prevents an out-of-bounds error, there will be multiple expressions (from different UDF body statements as well as the parent query) that reference the same annotation index, which stores the resolved object name for an unresolved name in the AST. I think the way we save only the tree.Statement for each body statement is broken, because the annotations are part of the state of the AST. The correct way to fix this will probably be to store the annotations along with the AST for each body statement in the tree.CreateFunction struct, and then have the CreateFunction.Format method set the correct annotations in FmtCtx while formatting the body. This would happen in optbuilder/create_function.go.

@Xiang-Gu does that sound reasonable? It sounds like it might be a simpler fix than modifying both schema changer paths.