sql: error in cluster_logical_timestamp when not SERIALIZABLE

[chrisseto]

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
Fixes: #103245
Release note (sql change): cluster_logical_timestamp now returns an error when called at isolation levels lower than SERIALIZABLE.

[cockroach-teamcity]

This change is 

[rafiss]

very nice! just some minor comments

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @chrisseto)

---

pkg/sql/sem/eval/context.go line 543 at r2 (raw file):

	// 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())

super nit: could you use kvTxnIsolationLevelToTree(ec.TxnIsoLevel).String(). we have this bit of translation to keep the user-facing grammar separate from the internals of KV, and it has slightly more familiar string formatting.

---

pkg/sql/logictest/testdata/logic_test/txn line 1536 at r2 (raw file):

statement error pq: cluster_logical_timestamp\(\): unsupported in ReadCommitted isolation
BEGIN TRANSACTION ISOLATION LEVEL READ COMMITTED;
SELECT cluster_logical_timestamp();

nit: this should have a

statement ok
ROLLBACK

after this

[chrisseto]

@rafiss could you point me in the right direction WRT the failing logic test? Seems like the test might be flake depending on which node you're executing against? If there's a way to read back the actual isolation level, we could skip the test if it isn't running at READ COMMITTED or we could gate the functionality on a specific version?

[20:06:40] --- done: /home/roach/.cache/bazel/_bazel_roach/c5a4e7d36696d9cd970af2045211a7df/sandbox/processwrapper-sandbox/12659/execroot/com_github_cockroachdb_cockroach/bazel-out/k8-fastbuild/bin/pkg/sql/logictest/tests/local-mixed-22.2-23.1/local-mixed-22_2-23_1_test_/local-mixed-22_2-23_1_test.runfiles/com_github_cockroachdb_cockroach/pkg/sql/logictest/testdata/logic_test/txn with config local-mixed-22.2-23.1: 371 tests, 0 failures
    logic.go:4068:
        /home/roach/.cache/bazel/_bazel_roach/c5a4e7d36696d9cd970af2045211a7df/sandbox/processwrapper-sandbox/12659/execroot/com_github_cockroachdb_cockroach/bazel-out/k8-fastbuild/bin/pkg/sql/logictest/tests/local-mixed-22.2-23.1/local-mixed-22_2-23_1_test_/local-mixed-22_2-23_1_test.runfiles/com_github_cockroachdb_cockroach/pkg/sql/logictest/testdata/logic_test/txn:1534: error while processing
    logic.go:4068:
        /home/roach/.cache/bazel/_bazel_roach/c5a4e7d36696d9cd970af2045211a7df/sandbox/processwrapper-sandbox/12659/execroot/com_github_cockroachdb_cockroach/bazel-out/k8-fastbuild/bin/pkg/sql/logictest/tests/local-mixed-22.2-23.1/local-mixed-22_2-23_1_test_/local-mixed-22_2-23_1_test.runfiles/com_github_cockroachdb_cockroach/pkg/sql/logictest/testdata/logic_test/txn:1534:
        expected "pq: cluster_logical_timestamp\\(\\): unsupported in READ COMMITTED isolation", but no error occurred
    panic.go:522: -- test log scope end --
test logs left over in: /artifacts/tmp/_tmp/c2c63d472fc75a33623ecf6091cebe9e/logTestLogic_txn879699003

[rafiss]

Yes I can help for sure. The issue with this failure is:

        /home/roach/.cache/bazel/_bazel_roach/c5a4e7d36696d9cd970af2045211a7df/sandbox/processwrapper-sandbox/12659/execroot/com_github_cockroachdb_cockroach/bazel-out/k8-fastbuild/bin/pkg/sql/logictest/tests/local-mixed-22.2-23.1/local-mixed-22_2-23_1_test_/local-mixed-22_2-23_1_test.runfiles/com_github_cockroachdb_cockroach/pkg/sql/logictest/testdata/logic_test/txn:1534: error while processing
    logic.go:4068:
        /home/roach/.cache/bazel/_bazel_roach/c5a4e7d36696d9cd970af2045211a7df/sandbox/processwrapper-sandbox/12659/execroot/com_github_cockroachdb_cockroach/bazel-out/k8-fastbuild/bin/pkg/sql/logictest/tests/local-mixed-22.2-23.1/local-mixed-22_2-23_1_test_/local-mixed-22_2-23_1_test.runfiles/com_github_cockroachdb_cockroach/pkg/sql/logictest/testdata/logic_test/txn:1534:
        expected "pq: cluster_logical_timestamp\\(\\): unsupported in READ COMMITTED isolation", but no error occurred

To interpret that a bit, this is saying that the txn is failing under the local-mixed-22_2-23_1 logictest configuration. That logictest configuration is defined here:

cockroach/pkg/sql/logictest/logictestbase/logictestbase.go

Lines 471 to 478 in 4618dbf

	{
	Name: "local-mixed-22.2-23.1",
	NumNodes: 1,
	OverrideDistSQLMode: "off",
	BootstrapVersion: clusterversion.V22_2,
	DisableUpgrade: true,
	DeclarativeCorpusCollection: true,
	},

The test configuration uses a current binary, but a "logical cluster version" from the previous release. This is so that it tests the mixed-version state of a CRDB cluster, which occurs during the process of an online upgrade. One of the things that we did for the READ COMMITTED project is we added a version gate so that READ COMMITTED can only be used if the cluster upgrade was finalized, and the cluster is no longer in a mixed version state. Otherwise, it falls back to the old behavior of automatically promoting READ COMMITTED to the SERIALIZABLE level. That gate is in this function:

cockroach/pkg/sql/conn_executor.go

Line 3389 in 4618dbf

func (ex *connExecutor) txnIsolationLevelToKV(

To put this all together, the issue you are seeing where no error occurs is actually expected under the local-mixed-22_2-23_1 configuration. So the right thing to do in this case is to use the skipif config local-mixed-22_2-23_1 directive. Check out the logictest docs for more information on skipif:

cockroach/pkg/sql/logictest/logic.go

Lines 392 to 395 in 4618dbf

	// - skipif <mysql/mssql/postgresql/cockroachdb/config CONFIG [ISSUE]>
	// Skips the following `statement` or `query` if the argument is
	// postgresql, cockroachdb, or a config matching the currently
	// running configuration.

[chrisseto]

Thanks! This round of CI should pass now 🤞

[rafiss]

nice!

[chrisseto]

bors r+

[craig]

Build succeeded:

• Bazel Essential CI (Cockroach)