From 31c798a3528dc4baa136db92332116ecd42c69b2 Mon Sep 17 00:00:00 2001 From: Chris Seto Date: Tue, 18 Jul 2023 13:31:30 -0400 Subject: [PATCH] sql: error in cluster_logical_timestamp when not SERIALIZABLE Previously, running `cluster_logical_timestamp` within an isolation level that tolerated write skew would result in an unhandled panic. This commit adds a gate at the SQL level that will instead return a FeatureNotSupported pgerror. This behavior may be permanent or may be removed once we determine how CommitTimestamp should function at these isolation levels. Epic: CRDB-26546 Part of: #103245 Release note (sql change): cluster_logical_timestamp now returns an error when called at isolation levels lower than SERIALIZABLE. --- pkg/sql/logictest/testdata/logic_test/txn | 11 +++++++++++ pkg/sql/sem/builtins/builtins.go | 4 +++- pkg/sql/sem/eval/context.go | 8 ++++++++ 3 files changed, 22 insertions(+), 1 deletion(-) diff --git a/pkg/sql/logictest/testdata/logic_test/txn b/pkg/sql/logictest/testdata/logic_test/txn index 20c710de791d..80f635225236 100644 --- a/pkg/sql/logictest/testdata/logic_test/txn +++ b/pkg/sql/logictest/testdata/logic_test/txn @@ -1523,3 +1523,14 @@ regular statement error pq: SET CLUSTER SETTING cannot be used inside a multi-statement transaction SET CLUSTER SETTING sql.defaults.use_declarative_schema_changer = 'on'; SET CLUSTER SETTING sql.defaults.use_declarative_schema_changer = 'off'; + +statement ok +BEGIN TRANSACTION ISOLATION LEVEL SERIALIZABLE; +SELECT cluster_logical_timestamp(); + +statement ok +ROLLBACK + +statement error pq: cluster_logical_timestamp\(\): unsupported in ReadCommitted isolation +BEGIN TRANSACTION ISOLATION LEVEL READ COMMITTED; +SELECT cluster_logical_timestamp(); diff --git a/pkg/sql/sem/builtins/builtins.go b/pkg/sql/sem/builtins/builtins.go index 676ea0206fcc..50df4994d481 100644 --- a/pkg/sql/sem/builtins/builtins.go +++ b/pkg/sql/sem/builtins/builtins.go @@ -2779,7 +2779,9 @@ nearest replica.`, builtinconstants.DefaultFollowerReadDuration), a CockroachDB HLC in decimal form. Note that uses of this function disable server-side optimizations and -may increase either contention or retry errors, or both.`, +may increase either contention or retry errors, or both. + +Returns an error if run in a transaction with an isolation level weaker than SERIALIZABLE.` Volatility: volatility.Volatile, }, ), diff --git a/pkg/sql/sem/eval/context.go b/pkg/sql/sem/eval/context.go index 7c0dd9248ddc..294e3ca652dd 100644 --- a/pkg/sql/sem/eval/context.go +++ b/pkg/sql/sem/eval/context.go @@ -535,6 +535,14 @@ func (ec *Context) GetClusterTimestamp() (*tree.DDecimal, error) { if ec.Txn == nil { return nil, ErrNilTxnInClusterContext } + + // CommitTimestamp panics for isolation levels that can operate across + // multiple timestamps. Prevent this with a gate at the SQL level and return + // a pgerror until we decide how this will officially behave. See #103245. + if ec.TxnIsoLevel.ToleratesWriteSkew() { + return nil, pgerror.Newf(pgcode.FeatureNotSupported, "unsupported in %s isolation", ec.TxnIsoLevel.String()) + } + ts := ec.Txn.CommitTimestamp() if ts.IsEmpty() { return nil, errors.AssertionFailedf("zero cluster timestamp in txn")