Looks pretty good!

Some test ideas:

• something with cockroach_restart
• something with pausable portals
• something with implicit txn retry

Reviewed 4 of 4 files at r1, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @mgartner and @rafiss)

pkg/sql/conn_executor_savepoints.go line 34 at r1 (raw file):

	ctx context.Context, s *tree.Savepoint, res RestrictedCommandResult,
) (fsm.Event, fsm.EventPayload, error) {
	ex.state.mu.hasSavepoints = true

Would we want to set hasSavepoints to false somewhere in execRelease to allow the eager rollback of a transaction that fails after all created savepoints have been released?

something with cockroach_restart

I added a test with savepoints. I don't think cockroach_restart needs any other special testing, but I could be missing something, so just let me know what else you had in mind.

something with pausable portals

I was thinking this would be covered by our metamorphic testing. But I can add a test for a specific scenario if you can think of one.

something with implicit txn retry

Added this test.

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

pkg/sql/conn_executor_savepoints.go line 34 at r1 (raw file):

done. good point.

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

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

					ts.mu.Lock()
					defer ts.mu.Unlock()
					if !ts.mu.hasSavepoints {

Something occurred to me: if this is a nested connExecutor created by InternalExecutor.initConnEx, sharing a transaction with its parent connExecutor, then will it know about the savepoints in its parent connExecutor?

Maybe we also need to check that this is a top-level connExecutor? Or we need to add savepoint info to InternalExecutor.extraTxnState?

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

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

When we're running an internal executor delegated from a parent transaction, the connExecutor uses a different state machine. It's further down below:

The transaction is never rolled back in those state transitions.

Reviewed 7 of 7 files at r2, 3 of 3 files at r3, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @mgartner)

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

Great, thank you!

pkg/sql/conn_executor_test.go line 1905 at r2 (raw file):

		err = conn1.QueryRowContext(ctx, `SELECT v FROM t WHERE k = 4`).Scan(&v)
		require.NoError(t, err)
		require.Equal(t, 400, v)

one idea for a cockroach_restart test is something like:

-- conn 1
INSERT INTO t VALUES (5, 5), (6, 6);
BEGIN;
SAVEPOINT cockroach_restart;
SELECT * FROM t WHERE k = 5;
    -- conn 2 (to break serializability)
    UPDATE t SET v = 50 WHERE k = 5;
    SELECT * FROM t WHERE k = 6;
-- conn 1
UPDATE t SET v = 60 WHERE k = 6;
    -- conn 2 (blocks, to prove locks are still held)
    UPDATE t SET v = v + 600 WHERE k = 6;
-- conn 1 (should fail with RETRY_SERIALIZABLE)
RELEASE SAVEPOINT cockroach_restart;
-- simulate the advanced retry
ROLLBACK TO SAVEPOINT cockroach_restart;
SAVEPOINT cockroach_restart;
SELECT * FROM t WHERE k = 5;
UPDATE t SET v = 61 WHERE k = 6;
RELEASE SAVEPOINT cockroach_restart;
    -- conn 2 (to prove that ordering was correct and locks are now released)
    SELECT * FROM t WHERE k = 6;
-- conn 1
COMMIT;

pkg/sql/conn_executor_test.go line 1916 at r4 (raw file):

// layer, but for now this is the expected behavior.
// See https://github.com/cockroachdb/cockroach/issues/117020.
func TestRetriableErrorDuringTransactionHoldsLocks(t *testing.T) {

Thanks for adding this!

pkg/sql/conn_executor_exec.go line 186 at r3 (raw file):

	ex.startIdleInSessionTimeout()

	if ex.sessionData().IdleInTransactionSessionTimeout > 0 {

If both IdleInTransactionSessionTimeout and txnTimeoutRemaining are > 0, do we need to set both timers? Seems like we only need to set the one that will fire first?

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

pkg/sql/conn_executor_exec.go line 186 at r3 (raw file):

the two timers have slightly different behaviors. as per the commend i added below:

				// The transaction_timeout setting should move the transaction to the
				// aborted state.
				// NOTE: In Postgres, the transaction_timeout causes the entire session
				// to be terminated. We intentionally diverge from that behavior.

however, the IdleInTransactionSessionTimeout timer causes the session to be terminated.

pkg/sql/conn_executor_test.go line 1905 at r2 (raw file):

nice! let me give this a try

thanks for the reviews! this is ready for another look.

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

Just one suggestion. This is great!

Reviewed 4 of 4 files at r5, 3 of 3 files at r6, 1 of 1 files at r7, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @mgartner)

pkg/sql/conn_executor_exec.go line 186 at r3 (raw file):

Ah, I missed that. Thanks!

pkg/sql/conn_executor_test.go line 1936 at r5 (raw file):

		_, err = conn1.ExecContext(ctx, `RELEASE SAVEPOINT cockroach_restart`)
		require.ErrorContains(t, err, "failed preemptive refresh due to encountered recently written committed value")

nit: Because we're not letting the conn2 update block the entire time, I think it would be a slightly better test to try the conn2 update right after releasing the cockroach_restart savepoint. This would validate that locks are still held after releasing the cockroach_restart savepoint.

tftr!

bors r+

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

pkg/sql/conn_executor_test.go line 1936 at r5 (raw file):

good point! added the assertion after too

Thanks!!!

Reviewed 3 of 7 files at r2, 4 of 4 files at r8, 3 of 3 files at r9, 1 of 1 files at r10, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (and 1 stale)