sql: mark implicit transactions with AOST as read-only

[michae2]

sql: mark implicit transactions with AOST as read-only

In handleAOST we were setting the transaction AOST but not marking it as read-only. It needs to also be marked as read-only to disallow mutation statements and locking statements.

Fixes: #120081

Release note (sql change): Mutation statements such as UPDATE and DELETE as well as locking statements such as SELECT FOR UPDATE are not allowed in read-only transactions or AS OF SYSTEM TIME transactions. Fix an oversight where we were allowing mutation statements and locking statements in implicit single-statement transactions using AS OF SYSTEM TIME.

---

sql: disallow SET transaction_read_only = false during AOST txn

Fixes: #44200

Release note (bug fix): Fix a bug in which it was possible to SET transaction_read_only = false during an AS OF SYSTEM TIME transaction.

[cockroach-teamcity]

This change is 

[michae2]

Should we backport this? I'd like to but am slightly worried that someone is inadvertently already using SELECT ... AS OF SYSTEM TIME ... FOR UPDATE.

[mgartner]

Should we backport this? I'd like to but am slightly worried that someone is inadvertently already using SELECT ... AS OF SYSTEM TIME ... FOR UPDATE.

Is the current behavior well-defined and reasonable? What about the current behavior of UPDATE or DELETE statements?

[yuzefovich]

Nice finds!

I'm guessing the current code could lead to an undefined behavior / broken assumptions somewhere, so I'd lean towards backporting it.

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

[rafiss]

super small nits. thanks for the fix!

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

---

pkg/sql/conn_executor_exec.go line 1298 at r1 (raw file):

				return err
			}
			p.extendedEvalCtx.TxnReadOnly = ex.state.readOnly.Load()

nit: i think we may not need p.extendedEvalCtx.TxnReadOnly = ex.state.readOnly.Load() here, since this should happen when reesetPlanner is called later. but i don't think it hurts either.

---

pkg/sql/exec_util.go line 3306 at r2 (raw file):

	// call into the connEx, which modifies its TxnState.
	// NOTE(andrei): I couldn't find good documentation on transaction_read_only,
	// but I've tested its behavior in Postgres 11.

unrelated to your PR, but this comment could mention that transaction_read_only/default_transaction_read_only have similar behavior as transaction_isolation/default_transaction_isolation. (just to make it seem less like a scary special thing)

---

pkg/sql/logictest/testdata/logic_test/txn line 1334 at r1 (raw file):

statement error cannot execute DELETE in a read-only transaction
DELETE FROM kv

nit: seems like this is a duplicate of the existing line above this

[michae2]

Is the current behavior well-defined and reasonable? What about the current behavior of UPDATE or DELETE statements?

Good point, it is not well-defined. Might even allow corruption. I'll backport.

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @rafiss and @yuzefovich)

---

pkg/sql/conn_executor_exec.go line 1298 at r1 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

nit: i think we may not need p.extendedEvalCtx.TxnReadOnly = ex.state.readOnly.Load() here, since this should happen when reesetPlanner is called later. but i don't think it hurts either.

I left it in for consistency, since we're setting other fields of the evalctx here.

---

pkg/sql/exec_util.go line 3306 at r2 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

unrelated to your PR, but this comment could mention that transaction_read_only/default_transaction_read_only have similar behavior as transaction_isolation/default_transaction_isolation. (just to make it seem less like a scary special thing)

Good point, done.

---

pkg/sql/logictest/testdata/logic_test/txn line 1334 at r1 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

nit: seems like this is a duplicate of the existing line above this

Whoops, thanks! Done.

[rafiss]

lgtm! thank you

[michae2]

YW, TFTRs!

Failure is #119288

bors r=yuzefovich,rafiss

[craig]

Build succeeded:

• Bazel Essential CI (Cockroach)

[blathers-crl]

Encountered an error creating backports. Some common things that can go wrong:

1. The backport branch might have already existed.
2. There was a merge conflict.
3. The backport branch contained merge commits.

You might need to create your backport manually using the backport tool.

---

error creating merge commit from 238ca33 to blathers/backport-release-23.1-120097: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict []

you may need to manually resolve merge conflicts with the backport tool.

Backport to branch 23.1.x failed. See errors above.

---

error creating merge commit from 238ca33 to blathers/backport-release-23.2-120097: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict []

you may need to manually resolve merge conflicts with the backport tool.

Backport to branch 23.2.x failed. See errors above.

---

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