sql: v20.2.0: panic: unexpected node type when constructing plan for ALTER DATABASE _ OWNER TO _

[cockroach-teamcity]

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

Sentry link: https://sentry.io/organizations/cockroach-labs/issues/2039644062/?referrer=webhooks_plugin

Panic message:

exec_factory_util.go:44: unexpected node type
--
*errutil.leafError: unexpected node type (1)
exec_factory_util.go:44: *withstack.withStack (top exception)
*assert.withAssertionFailure
conn_executor.go:506: *withstack.withStack (2)
*safedetails.withSafeDetails: while executing: ALTER DATABASE _ OWNER TO _ (3)
conn_executor.go:506: *withstack.withStack (4)
(check the extra data payloads)

Stacktrace (expand for inline code snippets):

cockroach/pkg/sql/conn_executor.go

Lines 505 to 507 in 150c591

	r := recover()
	h.ex.closeWrapper(ctx, r)
	}()

in pkg/sql.(*Server).ServeConn.func1
/usr/local/go/src/runtime/panic.go#L678-L680 in runtime.gopanic

cockroach/pkg/sql/exec_factory_util.go

Lines 43 to 45 in 150c591

	default:
	panic(errors.AssertionFailedf("unexpected node type %T", node))
	}

in pkg/sql.constructPlan.func1

cockroach/pkg/sql/exec_factory_util.go

Lines 46 to 48 in 150c591

	}
	assignPlan(&res.main, root)
	if len(subqueries) > 0 {

in pkg/sql.constructPlan

cockroach/pkg/sql/opt_exec_factory.go

Lines 1025 to 1027 in 150c591

	}
	return constructPlan(ef.planner, root, subqueries, cascades, checks)
	}

in pkg/sql.(*execFactory).ConstructPlan

cockroach/pkg/sql/opt/exec/explain/explain_factory.go

Lines 150 to 152 in 150c591

	var err error
	p.WrappedPlan, err = f.wrappedFactory.ConstructPlan(
	p.Root.WrappedNode(), wrappedSubqueries, wrappedCascades, wrappedChecks,

in pkg/sql/opt/exec/explain.(*Factory).ConstructPlan

cockroach/pkg/sql/opt/exec/execbuilder/builder.go

Lines 140 to 142 in 150c591

	}
	return b.factory.ConstructPlan(plan.root, b.subqueries, b.cascades, b.checks)
	}

in pkg/sql/opt/exec/execbuilder.(*Builder).Build

cockroach/pkg/sql/plan_opt.go

Lines 580 to 582 in 150c591

	bld := execbuilder.New(explainFactory, mem, &opc.catalog, mem.RootExpr(), evalCtx, allowAutoCommit)
	plan, err := bld.Build()
	if err != nil {

in pkg/sql.(*optPlanningCtx).runExecBuilder

cockroach/pkg/sql/plan_opt.go

Lines 264 to 266 in 150c591

	// If we got here, we did not create a plan above.
	return opc.runExecBuilder(
	&p.curPlan,

in pkg/sql.(*planner).makeOptimizerPlan

cockroach/pkg/sql/conn_executor_exec.go

Lines 900 to 902 in 150c591

	if err := planner.makeOptimizerPlan(ctx); err != nil {
	log.VEventf(ctx, 1, "optimizer plan failed: %v", err)

in pkg/sql.(*connExecutor).makeExecPlan

cockroach/pkg/sql/conn_executor_exec.go

Lines 779 to 781 in 150c591

	// between here and there needs to happen even if there's an error.
	err := ex.makeExecPlan(ctx, planner)
	// We'll be closing the plan manually below after execution; this

in pkg/sql.(*connExecutor).dispatchToExecutionEngine

cockroach/pkg/sql/conn_executor_exec.go

Lines 638 to 640 in 150c591

	p.autoCommit = os.ImplicitTxn.Get() && !ex.server.cfg.TestingKnobs.DisableAutoCommit
	if err := ex.dispatchToExecutionEngine(ctx, p, res); err != nil {
	return nil, nil, err

in pkg/sql.(*connExecutor).execStmtInOpenState

cockroach/pkg/sql/conn_executor_exec.go

Lines 113 to 115 in 150c591

	} else {
	ev, payload, err = ex.execStmtInOpenState(ctx, stmt, res, pinfo)
	}

in pkg/sql.(*connExecutor).execStmt

cockroach/pkg/sql/conn_executor.go

Lines 1464 to 1466 in 150c591

	stmtCtx := withStatement(ctx, ex.curStmt)
	ev, payload, err = ex.execStmt(stmtCtx, curStmt, stmtRes, nil /* pinfo */)
	return err

in pkg/sql.(*connExecutor).execCmd.func1

cockroach/pkg/sql/conn_executor.go

Lines 1466 to 1468 in 150c591

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

in pkg/sql.(*connExecutor).execCmd

cockroach/pkg/sql/conn_executor.go

Lines 1390 to 1392 in 150c591

	var err error
	if err = ex.execCmd(ex.Ctx()); err != nil {
	if errors.IsAny(err, io.EOF, errDrainingComplete) {

in pkg/sql.(*connExecutor).run

cockroach/pkg/sql/conn_executor.go

Lines 507 to 509 in 150c591

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

in pkg/sql.(*Server).ServeConn

cockroach/pkg/sql/pgwire/conn.go

Lines 625 to 627 in 150c591

	reservedOwned = false // We're about to pass ownership away.
	retErr = sqlServer.ServeConn(ctx, connHandler, reserved, cancelConn)
	}()

in pkg/sql/pgwire.(*conn).processCommandsAsync.func1
/usr/local/go/src/runtime/asm_amd64.s#L1356-L1358 in runtime.goexit

pkg/sql/conn_executor.go in pkg/sql.(*Server).ServeConn.func1 at line 506
/usr/local/go/src/runtime/panic.go in runtime.gopanic at line 679
pkg/sql/exec_factory_util.go in pkg/sql.constructPlan.func1 at line 44
pkg/sql/exec_factory_util.go in pkg/sql.constructPlan at line 47
pkg/sql/opt_exec_factory.go in pkg/sql.(*execFactory).ConstructPlan at line 1026
pkg/sql/opt/exec/explain/explain_factory.go in pkg/sql/opt/exec/explain.(*Factory).ConstructPlan at line 151
pkg/sql/opt/exec/execbuilder/builder.go in pkg/sql/opt/exec/execbuilder.(*Builder).Build at line 141
pkg/sql/plan_opt.go in pkg/sql.(*optPlanningCtx).runExecBuilder at line 581
pkg/sql/plan_opt.go in pkg/sql.(*planner).makeOptimizerPlan at line 265
pkg/sql/conn_executor_exec.go in pkg/sql.(*connExecutor).makeExecPlan at line 901
pkg/sql/conn_executor_exec.go in pkg/sql.(*connExecutor).dispatchToExecutionEngine at line 780
pkg/sql/conn_executor_exec.go in pkg/sql.(*connExecutor).execStmtInOpenState at line 639
pkg/sql/conn_executor_exec.go in pkg/sql.(*connExecutor).execStmt at line 114
pkg/sql/conn_executor.go in pkg/sql.(*connExecutor).execCmd.func1 at line 1465
pkg/sql/conn_executor.go in pkg/sql.(*connExecutor).execCmd at line 1467
pkg/sql/conn_executor.go in pkg/sql.(*connExecutor).run at line 1391
pkg/sql/conn_executor.go in pkg/sql.(*Server).ServeConn at line 508
pkg/sql/pgwire/conn.go in pkg/sql/pgwire.(*conn).processCommandsAsync.func1 at line 626
/usr/local/go/src/runtime/asm_amd64.s in runtime.goexit at line 1357

Tag	Value
Cockroach Release	v20.2.0
Cockroach SHA:	150c591
Platform	linux amd64
Distribution	CCL
Environment	v20.2.0
Command	start-single-node
Go Version	``
# of CPUs
# of Goroutines

[yuzefovich]

cc @RaduBerinde

[RaduBerinde]

Looks like a bug in new functionality (alter owner). Returning nil, nil from an opaque operator is not ok:

cockroach/pkg/sql/alter_database.go

Line 48 in 150c591

return nil, nil

This probably executes properly by chance. It breaks when the statement is being explained (which happens periodically for all statements).

If we want a no-op, we should just return a zeroNode.

Repro is very easy:

root@127.0.0.1:39861/defaultdb> alter database foo owner to bob;
ALTER DATABSE OWNER

Time: 26ms total (execution 3ms / network 23ms)

root@127.0.0.1:39861/defaultdb> explain alter database foo owner to bob;
ERROR: internal error: unexpected node type <nil>
SQLSTATE: XX000
DETAIL: stack trace:

CC @solongordon

[RaduBerinde]

As part of the fix, please add a plan != nil assertion in buildOpaque to prevent this kind of thing in the future.

[rafiss]

@otan could you fix this on 20.2? i see that you have been working on alter_database.go recently, so this is fixed in master already.

[otan]

this got fixed by @angelapwen in 14c8d36. @angelapwen would you be interested in moving the ownership check into the exec plan like you did in your PR for just v20.2?

[angelapwen]

I'll move the ownership check for ALTER DATABASE for v20.2 sure!

@RaduBerinde If I understand correctly, returning nil, err is alright for a planNode right? It is only the case that nil, nil should not be allowed which is currently the case for only CCLOnlyStatement:

cockroach/pkg/sql/opaque.go

Line 178 in 81ac191

case tree.CCLOnlyStatement:

Did you want a general check for this nil, nil case to be a catch-all for all opaque statements? What would the error thrown be in this case — something like errors.AssertionFailedf("planNode cannot be nil %T", stmt)?

[otan]

you can't return a plan node which is nil.

the fix you introduced was that this bit

// If the owner we want to set to is the current owner, do a no-op.
	newOwner := string(n.n.Owner)
	if newOwner == privs.Owner {
		return nil, nil
	}

moved from AlterDatabaseOwner to func (n *alterDatabaseOwnerNode) startExec(params runParams) error {, which is allowed to return nil. that's what i expect for this fix!

[angelapwen]

Yep I understand that bit @otan! I was trying to work out how @RaduBerinde's previous comment above about adding a plan != nil assertion in buildOpaque would work.

[otan]

ah missed that comment, sorry!