Skip to content

sql: step txn after restoring from a savepoint for exception handling #156902

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+50 −1
Open

sql: step txn after restoring from a savepoint for exception handling #156902

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 39 additions & 0 deletions pkg/ccl/logictestccl/testdata/logic_test/udf_plpgsql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3409,3 +3409,42 @@ NOTICE: i: 4 j: 2
NOTICE: outer_counter: 4 inner_counter: 8

subtest end

# Regression test for a failure to step the read sequence number after catching
# an exception and restoring a savepoint (#156412).
subtest regression_156412

statement ok
CREATE TABLE t122278 (pk INT PRIMARY KEY);

statement ok
CREATE FUNCTION f(n INT) RETURNS INT AS $$
BEGIN
BEGIN
IF n = 0 THEN
RETURN 1 // 0;
END IF;
EXCEPTION
WHEN division_by_zero THEN
RETURN (SELECT 100 + count(*) FROM t122278);
END;
RETURN 1 // 0;
EXCEPTION
WHEN division_by_zero THEN
RETURN (SELECT 200 + count(*) FROM t122278);
END
$$ LANGUAGE PLpgSQL;

statement ok
BEGIN;

statement ok
INSERT INTO t122278 values (1);

statement ok
SELECT f(0);

statement ok
COMMIT;

subtest end
3 changes: 3 additions & 0 deletions pkg/kv/txn.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1825,6 +1825,9 @@ func (txn *Txn) CreateSavepoint(ctx context.Context) (SavepointToken, error) {
// and can be reused later (e.g. to release or roll back again).
//
// This method is only valid when called on RootTxns.
//
// NB: after calling RollbackToSavepoint, the transaction's read sequence number
// must be stepped by calling Step() before any further reads are performed.
Comment on lines +1829 to +1830
Copy link
Collaborator

@stevendanna stevendanna Nov 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if it is viable to change how RollbackToSavepoint works. If we adopted my PR here: #156366 and then changed RollbackToSavepoint to always step the read sequence what problems might we cause?

Copy link
Collaborator Author

@DrewKimball DrewKimball Nov 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That behavior seems more intuitive for users of txns with stepping enabled. The only case I'm aware of where we want to keep an older sequence number is after returning from a volatile routine, and for that we already explicitly restore the sequence number.

func (txn *Txn) RollbackToSavepoint(ctx context.Context, s SavepointToken) error {
txn.mu.Lock()
defer txn.mu.Unlock()
Expand Down
9 changes: 8 additions & 1 deletion pkg/sql/routine.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -460,12 +460,19 @@ func (g *routineGenerator) handleException(ctx context.Context, err error) error
g.reset(ctx, g.p, branch, args)

// Configure stepping for volatile routines so that mutations made by the
// invoking statement are visible to the routine.
// invoking statement are visible to the routine. Make sure to also step
// before caching the read sequence number, since the read sequence number
// may be part of "ignored" list set when restoring the savepoint above.
var prevSteppingMode kv.SteppingMode
var prevSeqNum enginepb.TxnSeq
txn := g.p.Txn()
if g.expr.EnableStepping {
prevSteppingMode = txn.ConfigureStepping(ctx, kv.SteppingEnabled)
stepErr := txn.Step(ctx, false /* allowReadTimestampStep */)
if stepErr != nil {
// This error is unexpected, so return immediately.
return errors.CombineErrors(err, errors.WithAssertionFailure(stepErr))
}
prevSeqNum = txn.GetReadSeqNum()
}

Expand Down
Loading