Skip to content

sql: track cursors across savepoint rollback - #174678

Open
Alignyx wants to merge 1 commit into
cockroachdb:masterfrom
Alignyx:fix-173449-cursor-savepoint-scope
Open

sql: track cursors across savepoint rollback#174678
Alignyx wants to merge 1 commit into
cockroachdb:masterfrom
Alignyx:fix-173449-cursor-savepoint-scope

Conversation

@Alignyx

@Alignyx Alignyx commented Sep 4, 2026

Copy link
Copy Markdown

Fixes #173449.

SQL savepoints currently do not track cursor lifecycle state. This change records a cursor creation boundary on each SQL savepoint, closes cursors created after that boundary when it is rolled back, and preserves earlier cursors during the transaction restart used for an initial savepoint.

Tests:

  • Added a cursor logic-test regression covering initial and non-initial savepoints, cursors opened before and after each savepoint, preservation of cursor position, and prevention of reads from rolled-back writes.
  • Ran the regression in all 13 default logic-test configurations.
  • Ran the complete local and local-read-committed cursor suites.
  • Ran the full 16-shard //pkg/sql:sql_test target.

Release note (bug fix): ROLLBACK TO SAVEPOINT now closes cursors opened after the savepoint and preserves cursors opened before it.

Record the cursor creation boundary at each SQL savepoint. Rollback closes cursors opened after that boundary while preserving earlier cursors and their current positions, including when an initial savepoint uses the transaction-restart cleanup path.

Fixes cockroachdb#173449

Release note (bug fix): ROLLBACK TO SAVEPOINT now closes cursors opened after the savepoint and preserves cursors opened before it.
@blathers-crl

blathers-crl Bot commented Sep 4, 2026

Copy link
Copy Markdown

Thank you for contributing to CockroachDB. Please ensure you have followed the guidelines for creating a PR.

My owl senses detect your PR is good for review. Please keep an eye out for any test failures in CI.

I have added a few people who may be able to assist in reviewing:

🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.

@blathers-crl blathers-crl Bot added O-community Originated from the community X-blathers-triaged blathers was able to find an owner labels Sep 4, 2026
@blathers-crl
blathers-crl Bot requested a review from DrewKimball September 4, 2026 10:10
@Alignyx

Alignyx commented Sep 4, 2026

Copy link
Copy Markdown
Author

Root cause

SQL cursor ownership is kept in connExecutor.extraTxnState.sqlCursors, while an SQL savepoint previously recorded only its KV savepoint token, DDL count, name, and release behavior. It had no cursor-lifecycle boundary.

That omission produced two opposite results depending on the KV savepoint state:

  1. Non-initial savepoint: execRollbackToSavepointInOpenState rolls the KV transaction back, restores the savepoint/session/advisory-lock stacks, and returns without a transaction-state-machine event. Because no cursor cleanup runs on this path, a cursor declared after the savepoint remains registered with the read sequence number from before the rollback. It can therefore continue returning rows written after the savepoint even though those writes have been discarded.

  2. Initial savepoint: rolling back an initial KV savepoint emits eventTxnRestart. resetExtraTxnState handles that event with the generic transaction-rollback cursor cleanup and closes every non-holdable cursor. This includes cursors opened before the SQL savepoint, even though PostgreSQL semantics require those cursors and their current FETCH/MOVE positions to survive.

The query plans on the good and bad cases are the same; the first divergence is cursor lifecycle state at savepoint rollback, not query planning or row execution.

Fix

Each SQL savepoint now records cursorCreationTime when its KV savepoint is created. This uses the existing sqlCursor.created timestamps and follows the same scope-boundary approach already used by PL/pgSQL exception blocks.

cursorMap.closeCursorsCreatedAfter walks the complete cursor registry and closes only cursors whose creation timestamp is after the savepoint boundary. It deletes each closed cursor and combines close errors so one cleanup failure does not prevent the remaining in-scope cursors from being closed. Both Open and Aborted rollback handlers invoke this scoped cleanup.

For an initial savepoint, the scoped cleanup has already established the correct cursor set before eventTxnRestart is emitted. A one-shot preserveCursorsOnTxnRestart flag tells resetExtraTxnState to skip only that subsequent generic all-cursor close. The flag is consumed and cleared immediately. Automatic transaction retries and every other restart continue using the existing cursor cleanup because they never set the flag.

Cursors opened before the savepoint are deliberately left as the same iterator objects, so their positions are not rewound. Cursors opened after the savepoint, including holdable cursors, are closed.

Regression coverage

The new regression_173449 logic test covers the complete lifecycle matrix:

  • pre-savepoint cursor + non-initial savepoint: survives and resumes at the next row;
  • post-savepoint cursor + non-initial savepoint: closes and cannot expose a rolled-back table row;
  • pre-savepoint cursor + initial savepoint: survives the internal transaction restart and keeps its position;
  • post-savepoint cursor + initial savepoint: closes before restart cleanup.

The regression passes in all 13 default logic-test configurations. The complete local and local-read-committed cursor suites, focused savepoint/retry/cursor tests, and all 16 shards of //pkg/sql:sql_test also pass.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

O-community Originated from the community X-blathers-triaged blathers was able to find an owner

Projects

None yet

Development

Successfully merging this pull request may close these issues.

sql: cursors are not closed by ROLLBACK TO SAVEPOINT and can return rolled-back rows

1 participant