sql: use user transaction if we have one to prepare queries

[ajwerner]

Prepartion of certain queries requires performing reads against the database.
If the user has already laid down intents, these reads may become part of a
dependency cycle. Prior to this commit, these reads would be on a different
transaction and thus this cycle would not be detected by our deadlock detection
mechanism.

This change opts to use the user's transaction for planning if there is one
and thus will properly interact with deadlock detection.

Fixes #46447.

Release justification: fixes for high-priority or high-severity bugs in
existing functionality

Release note: None

[cockroach-teamcity]

This change is 

[ajwerner]

hold off on looking at this, the testing is busted.

[ajwerner]

Alright, fixed the tests, RFAL.

[petermattis]

@ajwerner Is this something that can be and should be backported to 19.2? The schemachange workload can reproduce wedging on both 19.1 and 19.2. I'm presuming it is this same deadlock.

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

[ajwerner]

Yes this and the two other high priority txn usage fixes should all be backported to 19.2 and probably 19.1. Will do that after this goes in.

There’s still one more PR inbound to deal with a different sort of deadlock uncovered by the test.

[andreimatei]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner and @andreimatei)

---

pkg/sql/conn_executor_prepare.go, line 184 at r1 (raw file):

	flags, err := ex.populatePrepared(ctx, txn, placeholderHints, p)
	if err != nil {
		// NB: if this is not a new transaction then we should let the connExecutor

what do you mean "we should"? Do we or don't we?
I see that we do, except not very well. The code below doesn't handle retriable errors:

cockroach/pkg/sql/conn_executor_prepare.go

Line 35 in e7afadb

return eventNonRetriableErr{IsCommit: fsm.False}, eventNonRetriableErrPayload{err: err}

---

pkg/sql/conn_executor_test.go, line 599 at r1 (raw file):

	case <-time.After(time.Millisecond):
	case <-errCh:
		t.Fatal("expected the transaction to block")

put the error in the message

---

pkg/sql/conn_executor_test.go, line 654 at r1 (raw file):

	// This test will create an explicit transaction that encounters an error on
	// a latter statement during planning of SHOW COLUMNS. The planning for this
	// SHOW COLUMNS	will be run in the user's transaction. The test will inject

nit: spaces

---

pkg/sql/conn_executor_test.go, line 690 at r1 (raw file):

	_, err = tx.Prepare("SELECT NULL FROM [SHOW COLUMNS FROM bar] LIMIT 1")
	require.Regexp(t,
		"restart transaction: TransactionRetryWithProtoRefreshError: boom", err)

As I was saying above, I suspect this is currently broken since we're not actually returning the retriable code here. If you switch the code to pgx, you can assert on the error code.
In fact, I think that if things were to function correctly, this prepare should be auto-retried (endlessly, I guess, given your filter).

[ajwerner]

@andreimatei thanks for the review, it's ready for another look.

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

---

pkg/sql/conn_executor_prepare.go, line 184 at r1 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

what do you mean "we should"? Do we or don't we?
I see that we do, except not very well. The code below doesn't handle retriable errors:

cockroach/pkg/sql/conn_executor_prepare.go

Line 35 in e7afadb

return eventNonRetriableErr{IsCommit: fsm.False}, eventNonRetriableErrPayload{err: err}

yeah makes sense. Fixed.

---

pkg/sql/conn_executor_test.go, line 599 at r1 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

put the error in the message

Done.

---

pkg/sql/conn_executor_test.go, line 654 at r1 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

nit: spaces

Done.

---

pkg/sql/conn_executor_test.go, line 690 at r1 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

As I was saying above, I suspect this is currently broken since we're not actually returning the retriable code here. If you switch the code to pgx, you can assert on the error code.
In fact, I think that if things were to function correctly, this prepare should be auto-retried (endlessly, I guess, given your filter).

We got back a response from inserting a row, I don't think it should auto-retry (and it doesn't).

[andreimatei]

LGTM

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

[ajwerner]

TFTR!

bors r=andreimatei

[craig]

Build succeeded

• GitHub CI (Cockroach)