Ability to crash crdb on DDL using cluster_logical_timestamp()

[sean-]

Describe the problem

Possible to crash crdb using cluster_logical_timestamp() as a DEFAULT expression.

To Reproduce

CREATE TABLE t(i int);
INSERT INTO t VALUES(1);
ALTER TABLE t ADD COLUMN ts decimal NOT NULL DEFAULT cluster_logical_timestamp();

After the node comes back up, the schema appears to be in an inconsistent state.

root@localhost:26257/defaultdb> SELECT * FROM t;
  i
-----
  1
(1 row)
root@localhost:26257/defaultdb> ALTER TABLE t ADD COLUMN ts decimal NOT NULL DEFAULT cluster_logical_timestamp();
ERROR: column "ts" of relation "t" already exists
SQLSTATE: 42701

Environment:

• CockroachDB version CockroachDB CCL v22.2.5 (aarch64-apple-darwin21.2, built 2023/02/16 16:37:38, go1.19.4)
• Server OS: Darwin 22.3.0 Darwin Kernel Version 22.3.0: Mon Jan 30 20:38:37 PST 2023; root:xnu-8792.81.3~2/RELEASE_ARM64_T6000 arm64

Jira issue: CRDB-25159

[ajwerner]

The bigger deal is the uncaught panic which crashes the server. Related to #58164. The backfill code itself could catch some panics, but it's orchestrated through distsql, and it seems better to always catch the panics there.

[ajwerner]

cockroach/pkg/sql/sem/eval/context.go

Lines 499 to 502 in d2594fc

	ts := ec.Txn.CommitTimestamp()
	if ts.IsEmpty() {
	panic(errors.AssertionFailedf("zero cluster timestamp in txn"))
	}

This code can panic in two places!

[ajwerner]

@Xiang-Gu can you have a look at fixing the proximate problem (nil check in the above code) and then have the caller check for a nil value?

[sean-]

After the panic, I'm unable to perform DDL on the table. For instance:

root@localhost:26257/defaultdb> ALTER TABLE t add COLUMN foo INT;

This just hangs. I suspect there are a few things going on, in addition to the panic.

[ajwerner]

What's actually going on is that the schema change that crashed is still in the running state, but we have exponential backoff to deal with failures like this so it doesn't just keep killing nodes. You should be able to cancel the original job.

[ajwerner]

I think pausing the job before canceling it will help clear the backoff.