sql: disallow using cluster_logical_timestamp as column default when …

…backfilling

Previously, `ADD COLUMN ... DEFAULT cluster_logical_timestamp()` would
crash the node and leave the table in a corrupt state. The root cause
is a nil pointer dereference. This commit fixed it by returning an
unimplemented error and hence disallow using this builtin function as
default value when backfilling.

Release note (bug fix): fixed a bug as detailed in cockroachdb#98269.
Release justification: Fixed a bug that can crash node.

pkg/sql/backfill/BUILD.bazel

@@ -23,6 +23,8 @@ go_library(
         "//pkg/sql/catalog/typedesc",
         "//pkg/sql/execinfra",
         "//pkg/sql/execinfrapb",
+        "//pkg/sql/pgwire/pgcode",
+        "//pkg/sql/pgwire/pgerror",
         "//pkg/sql/row",
         "//pkg/sql/rowenc",
         "//pkg/sql/rowinfra",

pkg/sql/backfill/backfill.go

@@ -25,6 +25,8 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/schemaexpr"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/typedesc"
 	"github.com/cockroachdb/cockroach/pkg/sql/execinfra"
+	"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgcode"
+	"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror"
 	"github.com/cockroachdb/cockroach/pkg/sql/row"
 	"github.com/cockroachdb/cockroach/pkg/sql/rowenc"
 	"github.com/cockroachdb/cockroach/pkg/sql/rowinfra"
@@ -369,6 +371,11 @@ func (cb *ColumnBackfiller) RunColumnBackfillChunk(
 		for j, e := range cb.updateExprs {
 			val, err := eval.Expr(cb.evalCtx, e)
 			if err != nil {
+				if errors.Is(err, eval.ErrNilTxnInClusterContext) {
+					// Cannot use expressions that depend on the transaction of the
+					// evaluation context as the default value for backfill.
+					return roachpb.Key{}, pgerror.WithCandidateCode(err, pgcode.FeatureNotSupported)
+				}
 				return roachpb.Key{}, sqlerrors.NewInvalidSchemaDefinitionError(err)
 			}
 			if j < len(cb.added) && !cb.added[j].IsNullable() && val == tree.DNull {
@@ -875,6 +882,11 @@ func (ib *IndexBackfiller) BuildIndexEntriesChunk(
 			}
 			val, err := eval.Expr(ib.evalCtx, texpr)
 			if err != nil {
+				if errors.Is(err, eval.ErrNilTxnInClusterContext) {
+					// Cannot use expressions that depend on the transaction of the
+					// evaluation context as the default value for backfill.
+					err = pgerror.WithCandidateCode(err, pgcode.FeatureNotSupported)
+				}
 				return err
 			}
 			colIdx, ok := ib.colIdxMap.Get(colID)

pkg/sql/logictest/testdata/logic_test/alter_table

@@ -2828,3 +2828,14 @@ CREATE TABLE public.t_twocol (
   CONSTRAINT t_twocol_pkey PRIMARY KEY (id ASC),
   FAMILY fam_0 (id, a)
 )
+
+# This subtest disallows using builtin function `cluster_logical_timestamp()`
+# as the default expression when backfilling a column.
+subtest 98269
+
+statement ok
+CREATE TABLE t_98269 (i INT PRIMARY KEY);
+INSERT INTO t_98269 VALUES (0);
+
+statement error pgcode 0A000 .* cluster_logical_timestamp\(\): nil txn in cluster context
+ALTER TABLE t_98269 ADD COLUMN j DECIMAL NOT NULL DEFAULT cluster_logical_timestamp();

pkg/sql/sem/builtins/builtins.go

@@ -2818,8 +2818,8 @@ nearest replica.`, builtinconstants.DefaultFollowerReadDuration),
 		tree.Overload{
 			Types:      tree.ArgTypes{},
 			ReturnType: tree.FixedReturnType(types.Decimal),
-			Fn: func(ctx *eval.Context, args tree.Datums) (tree.Datum, error) {
-				return ctx.GetClusterTimestamp(), nil
+			Fn: func(evalCtx *eval.Context, args tree.Datums) (tree.Datum, error) {
+				return evalCtx.GetClusterTimestamp()
 			},
 			Info: `Returns the logical time of the current transaction as
 a CockroachDB HLC in decimal form.

pkg/sql/sem/eval/context.go

@@ -43,6 +43,8 @@ import (
 	"github.com/cockroachdb/errors"
 )
 
+var ErrNilTxnInClusterContext = errors.New("nil txn in cluster context")
+
 // Context defines the context in which to evaluate an expression, allowing
 // the retrieval of state such as the node ID or statement start time.
 //
@@ -404,12 +406,15 @@ func (ec *Context) GetStmtTimestamp() time.Time {
 
 // GetClusterTimestamp retrieves the current cluster timestamp as per
 // the evaluation context. The timestamp is guaranteed to be nonzero.
-func (ec *Context) GetClusterTimestamp() *tree.DDecimal {
+func (ec *Context) GetClusterTimestamp() (*tree.DDecimal, error) {
+	if ec.Txn == nil {
+		return nil, ErrNilTxnInClusterContext
+	}
 	ts := ec.Txn.CommitTimestamp()
 	if ts.IsEmpty() {
-		panic(errors.AssertionFailedf("zero cluster timestamp in txn"))
+		return nil, errors.AssertionFailedf("zero cluster timestamp in txn")
 	}
-	return TimestampToDecimalDatum(ts)
+	return TimestampToDecimalDatum(ts), nil
 }
 
 // HasPlaceholders returns true if this EvalContext's placeholders have been

pkg/sql/sem/eval/timeconv_test.go

@@ -23,6 +23,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/util/leaktest"
 	"github.com/cockroachdb/cockroach/pkg/util/log"
 	"github.com/cockroachdb/cockroach/pkg/util/stop"
+	"github.com/stretchr/testify/require"
 )
 
 // Test that EvalContext.GetClusterTimestamp() gets its timestamp from the
@@ -76,7 +77,8 @@ func TestClusterTimestampConversion(t *testing.T) {
 			),
 		}
 
-		dec := ctx.GetClusterTimestamp()
+		dec, err := ctx.GetClusterTimestamp()
+		require.NoError(t, err)
 		final := dec.Text('f')
 		if final != d.expected {
 			t.Errorf("expected %s, but found %s", d.expected, final)