From 4b7aa6ea798f370593d847e4b60eb74a250d508a Mon Sep 17 00:00:00 2001 From: Xiang Gu Date: Wed, 15 Mar 2023 14:03:54 -0400 Subject: [PATCH] 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 #98269. --- pkg/sql/backfill/BUILD.bazel | 3 +++ pkg/sql/backfill/backfill.go | 11 +++++++++++ pkg/sql/colexecerror/error.go | 2 ++ pkg/sql/logictest/testdata/logic_test/alter_table | 11 +++++++++++ pkg/sql/sem/builtins/builtins.go | 2 +- pkg/sql/sem/eval/BUILD.bazel | 1 + pkg/sql/sem/eval/context.go | 10 +++++++--- pkg/sql/sem/eval/timeconv_test.go | 4 +++- 8 files changed, 39 insertions(+), 5 deletions(-) diff --git a/pkg/sql/backfill/BUILD.bazel b/pkg/sql/backfill/BUILD.bazel index 553c7621a1ca..4e6e0ff418e7 100644 --- a/pkg/sql/backfill/BUILD.bazel +++ b/pkg/sql/backfill/BUILD.bazel @@ -24,9 +24,12 @@ go_library( "//pkg/sql/catalog/schemaexpr", "//pkg/sql/catalog/tabledesc", "//pkg/sql/catalog/typedesc", + "//pkg/sql/colexecerror", "//pkg/sql/execinfra", "//pkg/sql/execinfrapb", "//pkg/sql/isql", + "//pkg/sql/pgwire/pgcode", + "//pkg/sql/pgwire/pgerror", "//pkg/sql/row", "//pkg/sql/rowenc", "//pkg/sql/rowinfra", diff --git a/pkg/sql/backfill/backfill.go b/pkg/sql/backfill/backfill.go index e005dd423322..f9c26e1d09d1 100644 --- a/pkg/sql/backfill/backfill.go +++ b/pkg/sql/backfill/backfill.go @@ -25,8 +25,11 @@ import ( "github.com/cockroachdb/cockroach/pkg/sql/catalog/fetchpb" "github.com/cockroachdb/cockroach/pkg/sql/catalog/schemaexpr" "github.com/cockroachdb/cockroach/pkg/sql/catalog/typedesc" + "github.com/cockroachdb/cockroach/pkg/sql/colexecerror" "github.com/cockroachdb/cockroach/pkg/sql/execinfra" "github.com/cockroachdb/cockroach/pkg/sql/isql" + "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" @@ -372,6 +375,10 @@ func (cb *ColumnBackfiller) RunColumnBackfillChunk( for j, e := range cb.updateExprs { val, err := eval.Expr(ctx, cb.evalCtx, e) if err != nil { + if errors.Is(err, colexecerror.ErrNilTxnAccessedInColBackfill) { + // Issue #98269; Cannot use `cluster_logical_timestamp()` as column default value. + 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 { @@ -876,6 +883,10 @@ func (ib *IndexBackfiller) BuildIndexEntriesChunk( } val, err := eval.Expr(ctx, ib.evalCtx, texpr) if err != nil { + if errors.Is(err, colexecerror.ErrNilTxnAccessedInColBackfill) { + // Issue #98269; Cannot use `cluster_logical_timestamp()` as column default value. + err = pgerror.WithCandidateCode(err, pgcode.FeatureNotSupported) + } return err } colIdx, ok := ib.colIdxMap.Get(colID) diff --git a/pkg/sql/colexecerror/error.go b/pkg/sql/colexecerror/error.go index 78dce002bb72..fbcf24bca22f 100644 --- a/pkg/sql/colexecerror/error.go +++ b/pkg/sql/colexecerror/error.go @@ -24,6 +24,8 @@ import ( const panicLineSubstring = "runtime/panic.go" +var ErrNilTxnAccessedInColBackfill = errors.New("attempted to access nil txn in column backfill") + // CatchVectorizedRuntimeError executes operation, catches a runtime error if // it is coming from the vectorized engine, and returns it. If an error not // related to the vectorized engine occurs, it is not recovered from. diff --git a/pkg/sql/logictest/testdata/logic_test/alter_table b/pkg/sql/logictest/testdata/logic_test/alter_table index 820f0b43dcc4..473c3dc3066e 100644 --- a/pkg/sql/logictest/testdata/logic_test/alter_table +++ b/pkg/sql/logictest/testdata/logic_test/alter_table @@ -3173,3 +3173,14 @@ SHOW CONSTRAINTS FROM t_96728 table_name constraint_name constraint_type details validated t_96728 t_96728_j_k_key UNIQUE UNIQUE (j ASC, k ASC) WHERE (i > 0) true t_96728 t_96728_pkey PRIMARY KEY PRIMARY KEY (i ASC) true + +# 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\(\): attempted to access nil txn in column backfill +ALTER TABLE t_98269 ADD COLUMN j DECIMAL NOT NULL DEFAULT cluster_logical_timestamp(); diff --git a/pkg/sql/sem/builtins/builtins.go b/pkg/sql/sem/builtins/builtins.go index efd1fd80b0c1..fe3b552979fc 100644 --- a/pkg/sql/sem/builtins/builtins.go +++ b/pkg/sql/sem/builtins/builtins.go @@ -2751,7 +2751,7 @@ nearest replica.`, builtinconstants.DefaultFollowerReadDuration), Types: tree.ParamTypes{}, ReturnType: tree.FixedReturnType(types.Decimal), Fn: func(ctx context.Context, evalCtx *eval.Context, args tree.Datums) (tree.Datum, error) { - return evalCtx.GetClusterTimestamp(), nil + return evalCtx.GetClusterTimestamp() }, Info: `Returns the logical time of the current transaction as a CockroachDB HLC in decimal form. diff --git a/pkg/sql/sem/eval/BUILD.bazel b/pkg/sql/sem/eval/BUILD.bazel index 3223c990c573..5edc3d467a6a 100644 --- a/pkg/sql/sem/eval/BUILD.bazel +++ b/pkg/sql/sem/eval/BUILD.bazel @@ -53,6 +53,7 @@ go_library( "//pkg/settings/cluster", "//pkg/sql/catalog/catpb", "//pkg/sql/catalog/descpb", + "//pkg/sql/colexecerror", "//pkg/sql/lex", "//pkg/sql/parser", "//pkg/sql/pgwire/pgcode", diff --git a/pkg/sql/sem/eval/context.go b/pkg/sql/sem/eval/context.go index 83a5da1bdb3f..11b266ea6135 100644 --- a/pkg/sql/sem/eval/context.go +++ b/pkg/sql/sem/eval/context.go @@ -26,6 +26,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/roachpb" "github.com/cockroachdb/cockroach/pkg/settings/cluster" "github.com/cockroachdb/cockroach/pkg/sql/catalog/catpb" + "github.com/cockroachdb/cockroach/pkg/sql/colexecerror" "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgcode" "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror" "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgwirecancel" @@ -495,12 +496,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, colexecerror.ErrNilTxnAccessedInColBackfill + } 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 diff --git a/pkg/sql/sem/eval/timeconv_test.go b/pkg/sql/sem/eval/timeconv_test.go index 8ab1d9e434c9..603d2841523e 100644 --- a/pkg/sql/sem/eval/timeconv_test.go +++ b/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)