logictest: add a local-read-committed default config

[rafiss]

fixes #100178
Release note: None

[cockroach-teamcity]

This change is 

[michae2]

Thanks for creating this!

I guess basically every logictest does CREATE TABLE. 🤔

[blathers-crl]

Your pull request contains more than 1000 changes. It is strongly encouraged to split big PRs into smaller chunks.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.}

[rafiss]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @michae2 and @nvanbenschoten)

---

pkg/sql/logictest/testdata/logic_test/cascade line 3947 at r6 (raw file):

statement ok
BEGIN TRANSACTION ISOLATION LEVEL SERIALIZABLE;
SET LOCAL autocommit_before_ddl = false;

to reviewers: the 3rd commit updates many logic test files in a mechanical way: in many situations where the test file had a multi-statement transaction, the transaction initialization was changed to explicitly use SERIALIZABLE and turn of the autocommit_before_ddl setting. this is because the schema changer has different behavior under multi-statement transactions, so rather than updating all the tests to account for the autocommit_before_ddl setting, it seemed easier to just force the test to use a multi-statement txn.

this makes the diff pretty large. let me know if you prefer that i break up this PR further in some way.

[michae2]

Thank you for doing this! It is monumental!

I'm a little under halfway through. It's looking good.

So far, the only thing I'm finding difficult is the occasional skipif config local-read-committed on statements in the middle of explicit transactions that have presumably been affected by autocommit_before_ddl. The silent commit-on-DDL behavior is tricky, especially since there's not much in the test reminding the reader that the state of the txn might not match what they read.

What would you think of setting autocommit_before_ddl directly in tests, rather than for the config? You can say no, since that's probably much more painful... but it might make these tests easier to understand.

Or alternatively, what if we mostly used serializable transactions in these cases, and kept autocommit_before_ddl false, with only a few specific tests for autocommit_before_ddl rather than enabling it by default? That would reduce the need to SET LOCAL autocommit_before_ddl = false; everywhere and I think would be closer to what people expect.

Reviewed 21 of 45 files at r3, 30 of 30 files at r4, 1 of 1 files at r5, 26 of 118 files at r6, 1 of 3 files at r7, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @nvanbenschoten and @rafiss)

---

pkg/sql/conn_executor_exec.go line 942 at r4 (raw file):

	if tree.CanModifySchema(ast) &&
		((!ex.planner.EvalContext().TxnIsSingleStmt && ex.extraTxnState.firstStmtExecuted) ||
			!ex.implicitTxn()) &&

Should we also be checking ex.extraTxnState.firstStmtExecuted in the explicit transaction case?

---

pkg/ccl/logictestccl/testdata/logic_test/plpgsql_cursor line 814 at r7 (raw file):

<unnamed portal 10>

skipif config local-read-committed

I think you mentioned this during the meeting, but why don't these cursors work, again? This first case seems to work when I try it by hand, at least.

---

pkg/sql/logictest/testdata/logic_test/alter_table line 1566 at r7 (raw file):

# The unique constraint prevents new duplicate values.
skipif config local-read-committed 110873

Thanks for linking to the issue!

---

pkg/sql/logictest/testdata/logic_test/cascade line 3947 at r6 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

to reviewers: the 3rd commit updates many logic test files in a mechanical way: in many situations where the test file had a multi-statement transaction, the transaction initialization was changed to explicitly use SERIALIZABLE and turn of the autocommit_before_ddl setting. this is because the schema changer has different behavior under multi-statement transactions, so rather than updating all the tests to account for the autocommit_before_ddl setting, it seemed easier to just force the test to use a multi-statement txn.

this makes the diff pretty large. let me know if you prefer that i break up this PR further in some way.

This is fine with me.

---

pkg/sql/logictest/testdata/logic_test/check_constraints line 152 at r7 (raw file):

statement ok
UPDATE t4 SET a = 0 WHERE c = 3

Nice, thank you for trying it both ways.

---

pkg/sql/logictest/testdata/logic_test/grant_in_txn line 216 at r7 (raw file):

GRANT role_bar TO testuser;

skipif config local-read-committed

Why is this one needed?

[rafiss]

i think you're right. the silent autocommit behavior is tricky when it's not obvious that the setting was enabled.

what if we mostly used serializable transactions in these cases, and kept autocommit_before_ddl false, with only a few specific tests for autocommit_before_ddl rather than enabling it by default? That would reduce the need to SET LOCAL autocommit_before_ddl = false; everywhere and I think would be closer to what people expect.

i will try this approach. it seems much easier to understand as a reader

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @michae2 and @nvanbenschoten)

---

pkg/sql/conn_executor_exec.go line 942 at r4 (raw file):

Previously, michae2 (Michael Erickson) wrote…

Should we also be checking ex.extraTxnState.firstStmtExecuted in the explicit transaction case?

that seems like a good change to make

---

pkg/ccl/logictestccl/testdata/logic_test/plpgsql_cursor line 814 at r7 (raw file):

Previously, michae2 (Michael Erickson) wrote…

I think you mentioned this during the meeting, but why don't these cursors work, again? This first case seems to work when I try it by hand, at least.

i get this error:

plpgsql_cursor:815: SELECT name FROM pg_cursors;
        expected:
            <unnamed portal 10>
            foo

        but found (query options: "rowsort" -> ignore the following ordering of rows) :100: 

but i realize now this is just because the CREATE OR REPLACE FUNCTION made the transaction commit, and close the existing cursors. i can make this turn off the autocommit_before_ddl setting and use serializable, like in the other tests

---

pkg/sql/logictest/testdata/logic_test/grant_in_txn line 216 at r7 (raw file):

Previously, michae2 (Michael Erickson) wrote…

Why is this one needed?

this would fail with a "savepoint before_invalid_grant does not exist" error, since the transaction would have been auto-committed by this point.

aside: this is related to one of the reasons why i didn't think it was that beneficial to implement the autocommit_before_ddl setting as COMMIT; DDL; BEGIN; -- it feels potentially more confusing for the user to end up in a new explicit transaction, but none of the state from the earlier transaction state would carry over. it seemed easier to reason that the transaction was simply over due to the presence of DDL. (that's the mysql behavior too fwiw: https://dev.mysql.com/doc/refman/8.0/en/implicit-commit.html)

[rafiss]

This is ready for another look

[michae2]

Thanks! That helped a lot.

Reviewed 5 of 118 files at r6, 116 of 116 files at r8, 1 of 1 files at r9, 129 of 130 files at r10, 1 of 1 files at r11, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @nvanbenschoten and @rafiss)

---

pkg/ccl/logictestccl/testdata/logic_test/schema_change_in_txn line 26 at r11 (raw file):

# Before the change which introduced this test, it would erroneously return 2.
skipif config local-read-committed

Maybe this should be a serializable transaction instead?

---

pkg/ccl/logictestccl/testdata/logic_test/udf_plpgsql line 2866 at r11 (raw file):

$$ LANGUAGE PLpgSQL;

skipif config local-read-committed

Why is this one skipped?

---

pkg/sql/logictest/testdata/logic_test/fk line 4175 at r11 (raw file):

statement ok
DELETE FROM parent80828 WHERE id = 1;

Does this table exist under RC?

---

pkg/sql/logictest/testdata/logic_test/grant_in_txn line 216 at r7 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

this would fail with a "savepoint before_invalid_grant does not exist" error, since the transaction would have been auto-committed by this point.

aside: this is related to one of the reasons why i didn't think it was that beneficial to implement the autocommit_before_ddl setting as COMMIT; DDL; BEGIN; -- it feels potentially more confusing for the user to end up in a new explicit transaction, but none of the state from the earlier transaction state would carry over. it seemed easier to reason that the transaction was simply over due to the presence of DDL. (that's the mysql behavior too fwiw: https://dev.mysql.com/doc/refman/8.0/en/implicit-commit.html)

Ack. Thanks for explaining. It's kind of a "worse is better" argument, and I buy that.

---

pkg/sql/logictest/testdata/logic_test/lock_timeout line 3 at r11 (raw file):

# LogicTest: !local-read-committed
# The locks created in this test are intended to work with SERIALIZABLE
# transactions.

What happens when this one is run under RC? Do lock timeouts not work with replicated locks?

---

pkg/sql/logictest/testdata/logic_test/manual_retry line 1 at r11 (raw file):

# LogicTest: !local-read-committed

Might be good to add a comment about automatically retrying under RC?

---

pkg/sql/logictest/testdata/logic_test/read_committed line 3 at r11 (raw file):

# LogicTest: !local-mixed-23.1 !local-read-committed
# Don't run this test under local-read-committed, since that automatically
# sets autocommit_before_ddl and default_transaction_isolation, which we

I guess the autocommit_before_ddl part is outdated now?

---

pkg/sql/logictest/testdata/logic_test/schema_change_in_txn line 3 at r11 (raw file):

# LogicTest: !local-read-committed
# This test is skipped under local-read-committed, since it uses the
# autocommit_before_ddl setting, which makes it impossible to test schema

Outdated comment.

---

pkg/sql/logictest/testdata/logic_test/show_commit_timestamp line 331 at r11 (raw file):

# The timestamps are different under READ COMMITTED.
skipif config local-read-committed

Hmm. Is SHOW COMMIT TIMESTAMP broken under RC? Oh, is it because the transaction isn't pushed? But it seems like ts2 should still be the timestamp of the insert (2), (4) transaction. 🤔

---

pkg/sql/logictest/testdata/logic_test/show_tenant_fingerprints line 1 at r11 (raw file):

# LogicTest: !3node-tenant-default-configs !fakedist !fakedist-vec-off !fakedist-disk !local-read-committed

Why doesn't this one work?

---

pkg/sql/logictest/testdata/logic_test/strict_ddl_atomicity line 2 at r11 (raw file):

# LogicTest: !local-read-committed
# The local-read-committed configuration enables autocommit_before_ddl, which

Outdated comment?

---

pkg/sql/logictest/testdata/logic_test/txn line 2 at r11 (raw file):

# LogicTest: !local-read-committed
# The local-read-committed config enables the autocommit_before_ddl, which

Outdated comment?

---

pkg/sql/logictest/testdata/logic_test/txn_retry line 1 at r11 (raw file):

# LogicTest: !local-read-committed

Would be nice to add a comment about retries.

[rafiss]

thanks for catching all of those skips! ready for another look

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @michae2 and @nvanbenschoten)

---

pkg/ccl/logictestccl/testdata/logic_test/schema_change_in_txn line 26 at r11 (raw file):

Previously, michae2 (Michael Erickson) wrote…

Maybe this should be a serializable transaction instead?

fixed

---

pkg/ccl/logictestccl/testdata/logic_test/udf_plpgsql line 2866 at r11 (raw file):

Previously, michae2 (Michael Erickson) wrote…

Why is this one skipped?

that was an oversight, nice catch

---

pkg/sql/logictest/testdata/logic_test/fk line 4175 at r11 (raw file):

Previously, michae2 (Michael Erickson) wrote…

Does this table exist under RC?

it does exist. could you expand on the question a bit?

---

pkg/sql/logictest/testdata/logic_test/lock_timeout line 3 at r11 (raw file):

Previously, michae2 (Michael Erickson) wrote…

What happens when this one is run under RC? Do lock timeouts not work with replicated locks?

good question, i should have investigated if the differing behavior is expected or a bug with RC.

the error that happens is:

        .../pkg/sql/logictest/testdata/logic_test/lock_timeout:24:
        expected "canceling statement due to lock timeout on row \\(k\\)=\\(1\\) in t@t_pkey", but no error occurred

maybe @nvanbenschoten would have a clue if we'd expect that under read committed?

---

pkg/sql/logictest/testdata/logic_test/manual_retry line 1 at r11 (raw file):

Previously, michae2 (Michael Erickson) wrote…

Might be good to add a comment about automatically retrying under RC?

thanks for the nudge. we actually can run this under RC, just need to look for a slightly different error message in one case

---

pkg/sql/logictest/testdata/logic_test/read_committed line 3 at r11 (raw file):

Previously, michae2 (Michael Erickson) wrote…

I guess the autocommit_before_ddl part is outdated now?

nice catch

---

pkg/sql/logictest/testdata/logic_test/schema_change_in_txn line 3 at r11 (raw file):

Previously, michae2 (Michael Erickson) wrote…

Outdated comment.

indeed, nice catch

---

pkg/sql/logictest/testdata/logic_test/show_commit_timestamp line 331 at r11 (raw file):

Previously, michae2 (Michael Erickson) wrote…

Hmm. Is SHOW COMMIT TIMESTAMP broken under RC? Oh, is it because the transaction isn't pushed? But it seems like ts2 should still be the timestamp of the insert (2), (4) transaction. 🤔

the error here is:

            /private/var/tmp/_bazel_rafiss/9796b4657e3f3e4bab52d5a958783c22/sandbox/darwin-sandbox/79514/execroot/com_github_cockroachdb_cockroach/bazel-out/darwin_arm64-fastbuild/bin/pkg/sql/logictest/tests/local-read-committed/local-read-committed_test_/local-read-committed_test.runfiles/com_github_cockroachdb_cockroach/pkg/sql/logictest/testdata/logic_test/show_commit_timestamp:331: SELECT i,
                     CASE
                     WHEN ts = 1707500530525854000.0000000000 THEN 'ts1'
                     WHEN ts = 1707500530527876000.0000000000 THEN 'ts2'
                     END
                FROM (SELECT i, crdb_internal_mvcc_timestamp AS ts FROM foo)
            ORDER BY i ASC;
            expected:
                1  ts1
                2  ts2
                3  ts1
                4  ts2

            but found (query options: "") :
                1  ts1
                2  NULL
                3  ts1
                4  NULL

i may as well add an onlyif read committed version so that's more obvious. but also, i am unsure of the expected behavior here.

---

pkg/sql/logictest/testdata/logic_test/show_tenant_fingerprints line 1 at r11 (raw file):

Previously, michae2 (Michael Erickson) wrote…

Why doesn't this one work?

added a comment:

# This is skipped under local-read-committed, since it uses
# cluster_logical_timestamp() heavily, which does not work under
# READ COMMITTED.

---

pkg/sql/logictest/testdata/logic_test/strict_ddl_atomicity line 2 at r11 (raw file):

Previously, michae2 (Michael Erickson) wrote…

Outdated comment?

yeah, nice catch

---

pkg/sql/logictest/testdata/logic_test/txn line 2 at r11 (raw file):

Previously, michae2 (Michael Erickson) wrote…

Outdated comment?

fixed

---

pkg/sql/logictest/testdata/logic_test/txn_retry line 1 at r11 (raw file):

Previously, michae2 (Michael Erickson) wrote…

Would be nice to add a comment about retries.

rather than skipping the whole file, i skipped the one statement that doesn't work. (that statement seems to be the whole point of the test, but at least now if this file is augmented later, it will benefit from RC testing.)

[nvanbenschoten]

Reviewed 4 of 30 files at r4, 116 of 116 files at r8, 1 of 1 files at r9, 118 of 130 files at r10, 1 of 1 files at r11, 11 of 11 files at r12, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @michae2 and @rafiss)

---

pkg/ccl/logictestccl/testdata/logic_test/plpgsql_cursor line 20 at r12 (raw file):

  END
$$ LANGUAGE PLpgSQL;
BEGIN TRANSACTION ISOLATION LEVEL SERIALIZABLE;

Do we not support cursors in read committed transactions? Or is this because of the stored proc creation? Isn't that performed before this transaction begins?

---

pkg/ccl/logictestccl/testdata/logic_test/plpgsql_cursor line 814 at r12 (raw file):

<unnamed portal 10>

skipif config local-read-committed

Why do we need to skip here if we're also forcing the txn to run under serializable?

---

pkg/sql/logictest/testdata/logic_test/alter_type line 31 at r12 (raw file):

ROLLBACK

# We should be able to rename a type multiple types in a transaction.

s/types/times/?

---

pkg/sql/logictest/testdata/logic_test/lock_timeout line 3 at r11 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

good question, i should have investigated if the differing behavior is expected or a bug with RC.

the error that happens is:

        /private/var/tmp/_bazel_rafiss/9796b4657e3f3e4bab52d5a958783c22/sandbox/darwin-sandbox/78941/execroot/com_github_cockroachdb_cockroach/bazel-out/darwin_arm64-fastbuild/bin/pkg/sql/logictest/tests/local-read-committed/local-read-committed_test_/local-read-committed_test.runfiles/com_github_cockroachdb_cockroach/pkg/sql/logictest/testdata/logic_test/lock_timeout:24:
        expected "canceling statement due to lock timeout on row \\(k\\)=\\(1\\) in t@t_pkey", but no error occurred

maybe @nvanbenschoten would have a clue if we'd expect that under read committed?

I don't know which line that error is referring to, but I suspect the issue is that non-locking reads don't block on writes under RC, so there's no need for the lock timeout. So this would only be affecting the SELECTs that don't use FOR UPDATE or FOR SHARE.

It seems worthwhile to add a skipif config local-read-committed / onlyif config local-read-committed case for each of these.

---

pkg/sql/logictest/testdata/logic_test/select_for_share line 2 at r12 (raw file):

# LogicTest: !local-mixed-23.1 !local-read-committed
# This test assumes that SERIALIZABLE isolation is being used.

Does it need to? It would be nice to have test coverage for FOR SHARE under RC. Would it help avoid differences between RC and SSI if we set enable_durable_locking_for_serializable in addition to enable_shared_locking_for_serializable?

[rafiss]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @michae2 and @nvanbenschoten)

---

pkg/sql/logictest/testdata/logic_test/lock_timeout line 3 at r11 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

I don't know which line that error is referring to, but I suspect the issue is that non-locking reads don't block on writes under RC, so there's no need for the lock timeout. So this would only be affecting the SELECTs that don't use FOR UPDATE or FOR SHARE.

It seems worthwhile to add a skipif config local-read-committed / onlyif config local-read-committed case for each of these.

sorry, the line number is at the end of the file name: lock_timeout:24

making it assert each case explicitly sounds good to me

[rafiss]

thanks for checking all my work / making sure nothing is unnecessarily skipped. RFAL

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @michae2 and @nvanbenschoten)

---

pkg/ccl/logictestccl/testdata/logic_test/plpgsql_cursor line 20 at r12 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Do we not support cursors in read committed transactions? Or is this because of the stored proc creation? Isn't that performed before this transaction begins?

nice catch, i used an overly aggressive find/replace. i've changed it so it only uses SERIALIZABLE when the CREATE FUNCTION schema change needs to happen in the same transaction as the one using the function.

---

pkg/ccl/logictestccl/testdata/logic_test/plpgsql_cursor line 814 at r12 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Why do we need to skip here if we're also forcing the txn to run under serializable?

oops, this one was vestigial. removed this skip and the ones below

---

pkg/sql/logictest/testdata/logic_test/alter_type line 31 at r12 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

s/types/times/?

fixed

---

pkg/sql/logictest/testdata/logic_test/lock_timeout line 3 at r11 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

sorry, the line number is at the end of the file name: lock_timeout:24

making it assert each case explicitly sounds good to me

there were some SFU/UPDATE/DELETE cases that did not timeout under read committed. please see the latest patch and if there's any commentary that i can add for those cases, i'll do so

---

pkg/sql/logictest/testdata/logic_test/select_for_share line 2 at r12 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Does it need to? It would be nice to have test coverage for FOR SHARE under RC. Would it help avoid differences between RC and SSI if we set enable_durable_locking_for_serializable in addition to enable_shared_locking_for_serializable?

nice catch, this was also a vestigial skip from my first attempt at making this config. i've broken it down just to 3 cases that differ between RC/serializable

[nvanbenschoten]

We should wait for a sign off from @michae2 as well.

Reviewed 5 of 5 files at r13, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @michae2)

[michae2]

Nice work Rafi!

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

[rafiss]

tftrs!

bors r+

[craig]

Build succeeded:

• Bazel Essential CI (Cockroach)