sql: put connExecutor's AutoRetry fields into txnState's mutex

[RichardJCai]

Auto retry variables could be used outside the connExecutor's goroutine in calls to
serialize. If this is the case, the field should be in txnState's mutex struct.

Release note: None

Fixes #82506
Fixes #78178

[cockroach-teamcity]

This change is 

[andreimatei]

don't forget to update the PR message

[rafiss]

i like the direction, but have questions

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

---

pkg/sql/conn_executor.go line 2726 at r2 (raw file):

	// data which may be after the schema change when we retry.
	var minTSErr *roachpb.MinTimestampBoundUnsatisfiableError
	if err := ex.state.mu.autoRetryReason; err != nil && errors.As(err, &minTSErr) {

are these functions allowed to access ex.state.mu whenever? i don't believe the lock is held here right?

---

pkg/sql/conn_executor.go line 3033 at r2 (raw file):

	var autoRetryReasonStr string

	if ex.state.mu.autoRetryReason != nil {

ditto

---

pkg/sql/conn_executor.go line 3044 at r2 (raw file):

			NumStatementsExecuted: int32(ex.state.mu.stmtCount),
			NumRetries:            int32(txn.Epoch()),
			NumAutoRetries:        ex.state.mu.autoRetryCounter,

ditto

---

pkg/sql/conn_executor_exec.go line 1062 at r2 (raw file):

			ctx,
			ex.executorType,
			int(ex.state.mu.autoRetryCounter),

ditto

---

pkg/sql/conn_executor_exec.go line 1140 at r2 (raw file):

	}

	ex.sessionTracing.TraceRetryInformation(ctx, int(ex.state.mu.autoRetryCounter), ex.state.mu.autoRetryReason)

ditto

---

pkg/sql/conn_executor_exec.go line 1177 at r2 (raw file):

	ex.recordStatementSummary(
		ctx, planner,
		int(ex.state.mu.autoRetryCounter), res.RowsAffected(), res.Err(), stats,

ditto

---

pkg/sql/conn_executor_exec.go line 2251 at r2 (raw file):

		Committed:               ev.eventType == txnCommit,
		ImplicitTxn:             implicit,
		RetryCount:              int64(ex.state.mu.autoRetryCounter),

ditto

---

pkg/sql/conn_fsm.go line 538 at r2 (raw file):

func prepareTxnForRetryWithRewind(args fsm.Args) error {
	pl := args.Payload.(eventRetriableErrPayload)

is this safe or should we use pl, ok := ...

[ajwerner]

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

---

pkg/sql/conn_executor.go line 2726 at r2 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

are these functions allowed to access ex.state.mu whenever? i don't believe the lock is held here right?

See this for this and the dittos

cockroach/pkg/sql/txn_state.go

Lines 49 to 52 in 6f8be65

	// Note that reads of mu.txn from the session's main goroutine do not require
	// acquiring a read lock - since only that goroutine will ever write to
	// mu.txn. Writes to mu.txn do require a write lock to guarantee safety with
	// reads by other goroutines.

---

pkg/sql/conn_fsm.go line 469 at r2 (raw file):

	ts.mu.Lock()
	ts.mu.autoRetryCounter = 0

why not inside resetForNewSQLTxn?

Code quote:

	ts.mu.autoRetryCounter = 0
	ts.mu.autoRetryReason = nil
	ts.mu.Unlock()

---

pkg/sql/conn_fsm.go line 538 at r2 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

is this safe or should we use pl, ok := ...

this is safe, see below

---

pkg/sql/conn_fsm.go line 548 at r2 (raw file):

	ts.setAdvanceInfo(
		rewind,
		args.Payload.(eventRetriableErrPayload).rewCap,

We already did the type assertion, you can use pl.rewCap

[RichardJCai]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner, @rafiss, @RichardJCai, and @ZhouXing19)

---

pkg/sql/conn_executor.go line 2726 at r2 (raw file):

Previously, ajwerner wrote…

See this for this and the dittos

cockroach/pkg/sql/txn_state.go

Lines 49 to 52 in 6f8be65

	// Note that reads of mu.txn from the session's main goroutine do not require
	// acquiring a read lock - since only that goroutine will ever write to
	// mu.txn. Writes to mu.txn do require a write lock to guarantee safety with
	// reads by other goroutines.

Resolved.

---

pkg/sql/conn_fsm.go line 469 at r2 (raw file):

Previously, ajwerner wrote…

why not inside resetForNewSQLTxn?

Good call, done.

---

pkg/sql/conn_fsm.go line 538 at r2 (raw file):

Previously, ajwerner wrote…

this is safe, see below

Done.

[ajwerner]

Reviewed 2 of 2 files at r3, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @rafiss, @RichardJCai, and @ZhouXing19)

[RichardJCai]

Thanks for the reviews!

bors r=ajwerner

[craig]

Build succeeded:

• GitHub CI (Cockroach)