Merge pull request #160476 from cockroachdb/blathers/backport-release-25.4-160346

release-25.4: sql: step read seq on rollback

Author: jeffswenson

pkg/sql/conn_executor_exec.go

@@ -2113,6 +2113,18 @@ func (ex *connExecutor) execStmtInOpenStateWithPausablePortal(
 	return nil, nil, nil
 }
 
+func (ex *connExecutor) stepReadSequence(ctx context.Context) error {
+	prevSteppingMode := ex.state.mu.txn.ConfigureStepping(ctx, kv.SteppingEnabled)
+	if prevSteppingMode == kv.SteppingEnabled {
+		if err := ex.state.mu.txn.Step(ctx, false /* allowReadTimestampStep */); err != nil {
+			return err
+		}
+	} else {
+		ex.state.mu.txn.ConfigureStepping(ctx, prevSteppingMode)
+	}
+	return nil
+}
+
 // handleAOST gets the AsOfSystemTime clause from the statement, and sets
 // the timestamps of the transaction accordingly.
 func (ex *connExecutor) handleAOST(ctx context.Context, stmt tree.Statement) error {
@@ -2385,13 +2397,8 @@ func (ex *connExecutor) commitSQLTransactionInternal(ctx context.Context) (retEr
 	// write-write contention between transactions by inflating the contention
 	// footprint of each transaction (i.e. the duration measured in MVCC time that
 	// the transaction holds locks).
-	prevSteppingMode := ex.state.mu.txn.ConfigureStepping(ctx, kv.SteppingEnabled)
-	if prevSteppingMode == kv.SteppingEnabled {
-		if err := ex.state.mu.txn.Step(ctx, false /* allowReadTimestampStep */); err != nil {
-			return err
-		}
-	} else {
-		ex.state.mu.txn.ConfigureStepping(ctx, prevSteppingMode)
+	if err := ex.stepReadSequence(ctx); err != nil {
+		return err
 	}
 
 	if err := ex.createJobs(ctx); err != nil {
@@ -2516,7 +2523,14 @@ func (ex *connExecutor) rollbackSQLTransaction(
 		isKVTxnOpen = ex.state.mu.txn.IsOpen()
 	}
 	if isKVTxnOpen {
-		if err := ex.state.mu.txn.Rollback(ctx); err != nil {
+		// Step the read sequence before rolling back because the read sequence
+		// may be in the span reverted by a savepoint.
+		err := ex.stepReadSequence(ctx)
+		err = errors.CombineErrors(err, ex.state.mu.txn.Rollback(ctx))
+		if err != nil {
+			if buildutil.CrdbTestBuild && errors.IsAssertionFailure(err) {
+				log.Dev.Fatalf(ctx, "txn rollback failed: %+v", err)
+			}
 			log.Dev.Warningf(ctx, "txn rollback failed: %s", err)
 		}
 	}

pkg/sql/logictest/testdata/logic_test/savepoints

@@ -33,3 +33,22 @@ SELECT * FROM a
 
 statement ok
 COMMIT
+
+# Regression test for https://github.com/cockroachdb/cockroach/issues/135118.
+# Rolling back a savepoint before rolling back the transaction caused the
+# transaction rollback to fail.
+
+statement ok
+BEGIN
+
+statement ok
+SAVEPOINT sp1
+
+statement ok
+INSERT INTO a(id) VALUES (1);
+
+statement ok
+ROLLBACK TO SAVEPOINT sp1
+
+statement ok
+ROLLBACK