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

sql: v24.1.14: nil pointer regression due to a type-checking logic change #142615

Open
cockroach-sentry opened this issue Mar 10, 2025 · 6 comments · May be fixed by #143170
Open

sql: v24.1.14: nil pointer regression due to a type-checking logic change #142615

cockroach-sentry opened this issue Mar 10, 2025 · 6 comments · May be fixed by #143170
Assignees
Labels
branch-release-24.1 Used to mark GA and release blockers, technical advisories, and bugs for 24.1 C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. O-support Would prevent or help troubleshoot a customer escalation - bugs, missing observability/tooling, docs regression Regression from a release. T-sql-queries SQL Queries Team X-blathers-triaged blathers was able to find an owner

Comments

@cockroach-sentry
Copy link
Collaborator

cockroach-sentry commented Mar 10, 2025

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/6382290044/?referrer=webhooks_plugin

Panic Message:

catch.go:24: runtime error: invalid memory address or nil pointer dereference
(1)
Wraps: (2) assertion failure
Wraps: (3) attached stack trace
  -- stack trace:
  | github.com/cockroachdb/cockroach/pkg/util/errorutil.ShouldCatch
  | 	github.com/cockroachdb/cockroach/pkg/util/errorutil/catch.go:24
  | github.com/cockroachdb/cockroach/pkg/sql.setupGenerator.func3.1.1
  | 	github.com/cockroachdb/cockroach/pkg/sql/virtual_table.go:130
  | runtime.gopanic
  | 	GOROOT/src/runtime/panic.go:770
  | runtime.panicmem
  | 	GOROOT/src/runtime/panic.go:261
  | runtime.sigpanic
  | 	GOROOT/src/runtime/signal_unix.go:881
  | github.com/cockroachdb/cockroach/pkg/sql/sem/builtins.init.func503
  | 	github.com/cockroachdb/cockroach/pkg/sql/sem/builtins/builtins.go:2189
  | github.com/cockroachdb/cockroach/pkg/sql/sem/eval.(*evaluator).EvalFuncExpr
  | 	github.com/cockroachdb/cockroach/pkg/sql/sem/eval/expr.go:484
  | github.com/cockroachdb/cockroach/pkg/sql/sem/tree.(*FuncExpr).Eval
  | 	github.com/cockroachdb/cockroach/bazel-out/k8-opt/bin/pkg/sql/sem/tree/eval_expr_generated.go:277
  | github.com/cockroachdb/cockroach/pkg/sql/sem/eval.(*evaluator).evalFuncArgs
  | 	github.com/cockroachdb/cockroach/pkg/sql/sem/eval/expr.go:501
  | github.com/cockroachdb/cockroach/pkg/sql/sem/eval.(*evaluator).EvalFuncExpr
  | 	github.com/cockroachdb/cockroach/pkg/sql/sem/eval/expr.go:466
  | github.com/cockroachdb/cockroach/pkg/sql/sem/tree.(*FuncExpr).Eval
  | 	github.com/cockroachdb/cockroach/bazel-out/k8-opt/bin/pkg/sql/sem/tree/eval_expr_generated.go:277
  | github.com/cockroachdb/cockroach/pkg/sql/sem/eval.Expr
  | 	github.com/cockroachdb/cockroach/pkg/sql/sem/eval/expr.go:21
  | github.com/cockroachdb/cockroach/pkg/sql/catalog/schemaexpr.deserializeExprForFormatting
  | 	github.com/cockroachdb/cockroach/pkg/sql/catalog/schemaexpr/expr.go:318
  | github.com/cockroachdb/cockroach/pkg/sql/catalog/schemaexpr.formatExprForDisplayImpl
  | 	github.com/cockroachdb/cockroach/pkg/sql/catalog/schemaexpr/expr.go:256
  | github.com/cockroachdb/cockroach/pkg/sql/catalog/schemaexpr.FormatExprForDisplay
  | 	github.com/cockroachdb/cockroach/pkg/sql/catalog/schemaexpr/expr.go:213
  | github.com/cockroachdb/cockroach/pkg/sql.init.func770
  | 	github.com/cockroachdb/cockroach/pkg/sql/pg_catalog.go:377
  | github.com/cockroachdb/cockroach/pkg/sql.makeAllRelationsVirtualTableWithDescriptorIDIndex.func1.1
  | 	github.com/cockroachdb/cockroach/pkg/sql/pg_catalog.go:1187
  | github.com/cockroachdb/cockroach/pkg/sql.forEachTableDescWithTableLookupInternalFromDescriptors
  | 	github.com/cockroachdb/cockroach/pkg/sql/information_schema.go:2846
  | github.com/cockroachdb/cockroach/pkg/sql.forEachTableDescWithTableLookupInternal
  | 	github.com/cockroachdb/cockroach/pkg/sql/information_schema.go:2719
  | github.com/cockroachdb/cockroach/pkg/sql.forEachTableDescWithTableLookup
  | 	github.com/cockroachdb/cockroach/pkg/sql/information_schema.go:2690
  | github.com/cockroachdb/cockroach/pkg/sql.makeAllRelationsVirtualTableWithDescriptorIDIndex.func1
  | 	github.com/cockroachdb/cockroach/pkg/sql/pg_catalog.go:1181
  | github.com/cockroachdb/cockroach/pkg/sql.(*virtualDefEntry).getPlanInfo.func1.1
  | 	github.com/cockroachdb/cockroach/pkg/sql/virtual_schema.go:656
  | github.com/cockroachdb/cockroach/pkg/sql.setupGenerator.func3.1
  | 	github.com/cockroachdb/cockroach/pkg/sql/virtual_table.go:138
  | github.com/cockroachdb/cockroach/pkg/sql.setupGenerator.func3
  | 	github.com/cockroachdb/cockroach/pkg/sql/virtual_table.go:139
  | github.com/cockroachdb/cockroach/pkg/util/stop.(*Stopper).RunAsyncTaskEx.func2
  | 	github.com/cockroachdb/cockroach/pkg/util/stop/stopper.go:480
  | runtime.goexit
  | 	src/runtime/asm_amd64.s:1695
Wraps: (4) runtime error: invalid memory address or nil pointer dereference
  | -- cause hidden behind barrier
  | runtime error: invalid memory address or nil pointer dereference
  | (1) runtime error: invalid memory address or nil pointer dereference
  | Error types: (1) runtime.errorString
Error types: (1) *colexecerror.notInternalError (2) *assert.withAssertionFailure (3) *withstack.withStack (4) *barriers.barrierErr
-- report composition:
*barriers.barrierErr: masked error: runtime error: invalid memory address or nil pointer dereference
catch.go:24: *withstack.withStack (top exception)
*assert.withAssertionFailure
*colexecerror.notInternalError
Stacktrace (expand for inline code snippets):

src/runtime/asm_amd64.s#L1694-L1696

sp.UpdateGoroutineIDToCurrent()
f(ctx)
}()

return worker(ctx, funcRowPusher(addRow))
}()
// Notify that we are done sending rows.

}()
return worker(ctx, funcRowPusher(addRow))
}()

generator, cleanup, setupError := setupGenerator(ctx, func(ctx context.Context, pusher rowPusher) error {
return def.populate(ctx, p, dbDesc, func(row ...tree.Datum) error {
if err := e.validateRow(row, columns); err != nil {

h := makeOidHasher()
if err := forEachTableDescWithTableLookup(
ctx,

) error {
return forEachTableDescWithTableLookupInternal(
ctx, p, dbContext, virtualOpts, false /* allowAdding */, fn,

}
return forEachTableDescWithTableLookupInternalFromDescriptors(
ctx, p, dbContext, virtualOpts, allowAdding, all, fn)

}
if err := fn(dbDesc, sc, table, lCtx); err != nil {
return err

func(db catalog.DatabaseDescriptor, sc catalog.SchemaDescriptor, table catalog.TableDescriptor, lookup tableLookupFn) error {
return populateFromTable(ctx, p, h, db, sc, table, lookup, addRow)
},

}
displayExpr, err := schemaexpr.FormatExprForDisplay(
ctx, table, expr, &p.semaCtx, p.SessionData(), tree.FmtPGCatalog,

) (string, error) {
return formatExprForDisplayImpl(
ctx,

) (string, error) {
expr, err := deserializeExprForFormatting(ctx, desc, exprStr, semaCtx, fmtFlags)
if err != nil {

// Immutable.
d, err := eval.Expr(ctx, &eval.Context{}, sanitizedExpr)
if err == nil {

func Expr(ctx context.Context, evalCtx *Context, n tree.TypedExpr) (tree.Datum, error) {
return n.Eval(ctx, (*evaluator)(evalCtx))
}

https://github.com/cockroachdb/cockroach/blob/c8459c085e2258f589949e970a527a0342871276/bazel-out/k8-opt/bin/pkg/sql/sem/tree/eval_expr_generated.go#L276-L278
nullResult, args, err := e.evalFuncArgs(ctx, expr)
if err != nil {

for i, argExpr := range expr.Exprs {
arg, err := argExpr.(tree.TypedExpr).Eval(ctx, e)
if err != nil {

https://github.com/cockroachdb/cockroach/blob/c8459c085e2258f589949e970a527a0342871276/bazel-out/k8-opt/bin/pkg/sql/sem/tree/eval_expr_generated.go#L276-L278
res, err := fn.Fn.(FnOverload)(ctx, e.ctx(), args)
if err != nil {

oid := tree.MustBeDOid(args[0])
res, err := evalCtx.Sequence.IncrementSequenceByID(ctx, int64(oid.Oid))
if err != nil {

GOROOT/src/runtime/signal_unix.go#L880-L882
GOROOT/src/runtime/panic.go#L260-L262
GOROOT/src/runtime/panic.go#L769-L771
if r := recover(); r != nil {
if ok, e := errorutil.ShouldCatch(r); ok {
retErr = e

// get reported to Sentry.
err = errors.HandleAsAssertionFailure(err)
}

src/runtime/asm_amd64.s in runtime.goexit at line 1695
pkg/util/stop/stopper.go in pkg/util/stop.(*Stopper).RunAsyncTaskEx.func2 at line 480
pkg/sql/virtual_table.go in pkg/sql.setupGenerator.func3 at line 139
pkg/sql/virtual_table.go in pkg/sql.setupGenerator.func3.1 at line 138
pkg/sql/virtual_schema.go in pkg/sql.(*virtualDefEntry).getPlanInfo.func1.1 at line 656
pkg/sql/pg_catalog.go in pkg/sql.makeAllRelationsVirtualTableWithDescriptorIDIndex.func1 at line 1181
pkg/sql/information_schema.go in pkg/sql.forEachTableDescWithTableLookup at line 2690
pkg/sql/information_schema.go in pkg/sql.forEachTableDescWithTableLookupInternal at line 2719
pkg/sql/information_schema.go in pkg/sql.forEachTableDescWithTableLookupInternalFromDescriptors at line 2846
pkg/sql/pg_catalog.go in pkg/sql.makeAllRelationsVirtualTableWithDescriptorIDIndex.func1.1 at line 1187
pkg/sql/pg_catalog.go in pkg/sql.init.func770 at line 377
pkg/sql/catalog/schemaexpr/expr.go in pkg/sql/catalog/schemaexpr.FormatExprForDisplay at line 213
pkg/sql/catalog/schemaexpr/expr.go in pkg/sql/catalog/schemaexpr.formatExprForDisplayImpl at line 256
pkg/sql/catalog/schemaexpr/expr.go in pkg/sql/catalog/schemaexpr.deserializeExprForFormatting at line 318
pkg/sql/sem/eval/expr.go in pkg/sql/sem/eval.Expr at line 21
bazel-out/k8-opt/bin/pkg/sql/sem/tree/eval_expr_generated.go in pkg/sql/sem/tree.(*FuncExpr).Eval at line 277
pkg/sql/sem/eval/expr.go in pkg/sql/sem/eval.(*evaluator).EvalFuncExpr at line 466
pkg/sql/sem/eval/expr.go in pkg/sql/sem/eval.(*evaluator).evalFuncArgs at line 501
bazel-out/k8-opt/bin/pkg/sql/sem/tree/eval_expr_generated.go in pkg/sql/sem/tree.(*FuncExpr).Eval at line 277
pkg/sql/sem/eval/expr.go in pkg/sql/sem/eval.(*evaluator).EvalFuncExpr at line 484
pkg/sql/sem/builtins/builtins.go in pkg/sql/sem/builtins.init.func503 at line 2189
GOROOT/src/runtime/signal_unix.go in runtime.sigpanic at line 881
GOROOT/src/runtime/panic.go in runtime.panicmem at line 261
GOROOT/src/runtime/panic.go in runtime.gopanic at line 770
pkg/sql/virtual_table.go in pkg/sql.setupGenerator.func3.1.1 at line 130
pkg/util/errorutil/catch.go in pkg/util/errorutil.ShouldCatch at line 24

Tags

Tag Value
Command server
Environment v24.1.14
Go Version go1.22.5 X:nocoverageredesign
Platform linux amd64
Distribution CCL
Cockroach Release v24.1.14
Cockroach SHA c8459c0
# of CPUs 16
# of Goroutines 3952

Jira issue: CRDB-48445

@cockroach-sentry cockroach-sentry added branch-release-24.1 Used to mark GA and release blockers, technical advisories, and bugs for 24.1 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. labels Mar 10, 2025
Copy link

blathers-crl bot commented Mar 10, 2025

CC'ing via the CODEOWNERS-based sentry heuristic:

  • @cockroachdb/sql-foundations

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

@blathers-crl blathers-crl bot added T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions) X-blathers-triaged blathers was able to find an owner labels Mar 10, 2025
@spilchen
Copy link
Contributor

This is an unusual case. We encounter a panic due to following a nil pointer dereference in the nextval builtin. However, we shouldn't have entered that builtin in the first place.

Here’s the calling function from the stack:

sanitizedExpr, err := SanitizeVarFreeExpr(ctx, expr, typedExpr.ResolvedType(), "FORMAT", semaCtx,
volatility.Immutable, false /*allowAssignmentCast*/)
// If the expr has no variables and has Immutable, we can evaluate
// it and turn it into a constant.
if err == nil {
// An empty EvalContext is fine here since the expression has
// Immutable.
d, err := eval.Expr(ctx, &eval.Context{}, sanitizedExpr)
if err == nil {
return d, nil
}

We check the expression to ensure its maximum volatility is immutable and, based on that, set up an empty evalContext. However, the nextval builtin requires a fully initialized evalContext (which is why we dereference nil) and is marked as volatility.Volatile.

@exalate-issue-sync exalate-issue-sync bot changed the title Sentry: catch.go:24: runtime error: invalid memory address or nil pointer dereference (1) Wraps: (2) assertion failure Wraps: (3) attached stack trace -- stack trace: | github.com/cockroachdb/cock... Sentry: catch.go:24: runtime error: invalid memory address or nil pointer dereference (1) Wraps: (2) assertion failure Wraps: (3) attached stack trace -- stack trace: | github.com/cockroachdb/cock... Mar 12, 2025
@spilchen
Copy link
Contributor

I have a local repro for this:

create function app_to_db_id(app_id INT8) RETURNS INT8 LANGUAGE plpgsql AS $$ BEGIN RETURN app_id * 2; END; $$;
create sequence seq1;
create table test (id int8 not null default app_to_db_id(nextval('seq1'::REGCLASS)));
select * from pg_catalog.pg_attrdef;

The last query fails on v24.1.14 but succeeds on v24.1.13. Other versions might be affected as well.

Eventually, this query reaches the following code path:

sanitizedExpr, err := SanitizeVarFreeExpr(ctx, expr, typedExpr.ResolvedType(), "FORMAT", semaCtx,
volatility.Immutable, false /*allowAssignmentCast*/)
// If the expr has no variables and has Immutable, we can evaluate
// it and turn it into a constant.
if err == nil {
// An empty EvalContext is fine here since the expression has
// Immutable.
d, err := eval.Expr(ctx, &eval.Context{}, sanitizedExpr)
if err == nil {
return d, nil
}

The expression passed to SanitizeVarFreeExpr (app_to_db_id(nextval('seq1'::REGCLASS))) does not return an error, whereas it did previously. The lack of error is that it will proceed to evaluate the expression in the expr.Expr call. This is unexpected, as the expression contains a volatile function (nextval) but we requested that max volatility is immutable. As a result, when evaluation continues, we encounter a nil dereference. This happens because the nextval builtin requires a valid eval.Context, but only a dummy one is set up.

I ran a git bisect between v24.1.13 and v24.1.14, and the issue first appeared in commit 978b109.

@spilchen spilchen added T-sql-queries SQL Queries Team and removed T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions) labels Mar 13, 2025
@github-project-automation github-project-automation bot moved this to Triage in SQL Queries Mar 13, 2025
@yuzefovich
Copy link
Member

cc @DrewKimball

@rafiss rafiss added the regression Regression from a release. label Mar 13, 2025
@cvansicklecrdb cvansicklecrdb added the O-support Would prevent or help troubleshoot a customer escalation - bugs, missing observability/tooling, docs label Mar 17, 2025
@cvansicklecrdb
Copy link

cvansicklecrdb commented Mar 17, 2025

I forgot to post in here, this also came through support - ZD#26099

@yuzefovich yuzefovich changed the title Sentry: catch.go:24: runtime error: invalid memory address or nil pointer dereference (1) Wraps: (2) assertion failure Wraps: (3) attached stack trace -- stack trace: | github.com/cockroachdb/cock... sql: v24.1.14: nil pointer regression due to a type-checking logic change Mar 17, 2025
@yuzefovich yuzefovich removed the O-sentry Originated from an in-the-wild panic report. label Mar 18, 2025
@yuzefovich yuzefovich moved this from Triage to Active in SQL Queries Mar 18, 2025
DrewKimball added a commit to DrewKimball/cockroach that referenced this issue Mar 19, 2025
This commit adds a `RoutineUseResolvedType` flag to the `SemaContext`
properties to indicate that type-checking for a routine that has already
been resolved to a concrete type should short-circuit. This is used to
determine the column type for a `Values` operator which depends on a
RECORD-returning routine, which only determines its type after the
body is built. This will prevent regressions in other code paths that
do not desire this short-circuiting behavior.

Fixes cockroachdb#142615

Release note (bug fix): Fixed a bug in `v24.1.14`, `v24.3.7`, `v24.3.8`,
and `v25.1` which could cause a nil-pointer error when a column's default
expression contained a volatile expression (like `nextval`) as a UDF
argument.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
branch-release-24.1 Used to mark GA and release blockers, technical advisories, and bugs for 24.1 C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. O-support Would prevent or help troubleshoot a customer escalation - bugs, missing observability/tooling, docs regression Regression from a release. T-sql-queries SQL Queries Team X-blathers-triaged blathers was able to find an owner
Projects
Status: Active
Development

Successfully merging a pull request may close this issue.

6 participants