cockroachdb / cockroachdb/cockroach

sql: automatic retries corrupt cursor state

Open
#173,505 2 comments 0 reactions 1 assignee Claimed by @DrewKimball View on GitHub
A-sql-executor branch-release-23.2 C-bug O-agent T-sql-queries
Dominant language
Go
Stars
32.5k
Forks
4.1k
PR merge metrics
PR metrics pending

Description

**Describe the problem**

A `WITH HOLD` cursor that has survived a transaction commit becomes session-scoped state that lives on the `connExecutor` across transaction boundaries. CockroachDB's transparent transaction auto-retry (the "rewind" mechanism) does not account for this state: when a later transaction is silently retried, statements that read or mutate a held cursor are replayed against the same live cursor, corrupting it. Because auto-retry is meant to be invisible to the client, the corruption is silent.

There are two observable symptoms, both from the same root cause:

1. **`FETCH` returns silently wrong rows.** Each replay of the transaction advances the held cursor further, so the client receives rows from far past where its single `FETCH` should have landed. No error is raised and the transaction commits normally — a silent data-correctness bug.
2. **`CLOSE` raises a spurious error.** The first execution deletes the cursor; a replay re-runs the `CLOSE`, finds it already gone, and raises `34000 cursor "..." does not exist`, aborting a transaction whose work actually succeeded.

**To Reproduce**

On a single-node cluster (`cockroach start-single-node --insecure`), in one SQL session. `crdb_internal.force_retry` is used to make the retry deterministic; on a recent build it requires `SET allow_unsafe_internals = true`. The organic trigger is any retryable error (e.g. a serialization conflict from a concurrent writer) that arrives while the statement's results are still buffered.

Setup:
```sql
CREATE TABLE t (a INT PRIMARY KEY);
INSERT INTO t SELECT generate_series(1, 1000000);
SET allow_unsafe_internals = true;
BEGIN;
DECLARE c CURSOR WITH HOLD FOR SELECT a FROM t ORDER BY a;
COMMIT; -- c is now a committed, session-scoped held cursor
```

Symptom 1 — wrong `FETCH` rows (send as a single query so results stay buffered and auto-retry is available):
```sql
BEGIN; FETCH 2 FROM c; SELECT crdb_internal.force_retry('500ms'); COMMIT;
```
Observed: the first `FETCH 2` returns
```
a
-------
33005
33006
```
instead of rows 1 and 2. The transaction was silently retried thousands of times over the 500ms window, each replay consuming two more rows from the cursor. It then commits with no error. A follow-up `FETCH 2 FROM c;` returns 33007, 33008.

Symptom 2 — spurious `CLOSE` error (fresh held cursor `c2` declared the same way):
```sql
BEGIN; CLOSE c2; SELECT crdb_internal.force_retry('500ms'); COMMIT;
```
Observed:
```
ERROR: cursor "c2" does not exist
```
The replayed `CLOSE` fails because the first execution already deleted the cursor, and the transaction aborts.

Control: when `force_retry` instead raises a non-retryable error (so no replay happens), `FETCH 2` correctly returns rows 1 and 2 — confirming the retry loop, not `FETCH` itself, is the cause.

**Expected behavior**

Transparent auto-retry must be invisible to the client. A single client `FETCH 2` should return exactly two rows (1 and 2 here) regardless of how many times the server retried the transaction, and a single `CLOSE` should succeed exactly once and never raise `does not exist`. This matches how the rewind mechanism already handles other transaction state (prepared statements, portals, the savepoint stack, and session data are all snapshotted and restored across a rewind).

**Additional data**

Root cause: on a rewind, transaction-scoped cursors are closed and re-created by replaying their `DECLARE`, which is correct. Committed held cursors are different — they are session-scoped and deliberately preserved across transaction end (`cursorMap.closeAll` in `pkg/sql/sql_cursor.go` skips cursors marked `committed`), but they are **not** included in the rewind snapshot taken by `connExecutor.setTxnRewindPos` / restored in the `rewind` case of `connExecutor.execCmd` (`pkg/sql/conn_executor.go`), and `getRewindTxnCapability` does not consider them when deciding whether a retry is safe. So a replayed statement mutates a held cursor with nothing to undo it.

A proportionate fix is to give committed cursors the same snapshot treatment portals already receive (snapshot the cursor reference and its position when the rewind point advances, and restore both on rewind), so `FETCH`/`CLOSE` against a held cursor is correctly undone by a rewind.

Introduced in v25.2.0 by holdable-cursor support (commit [`9716d4e3e3a`](https://github.com/cockroachdb/cockroach/commit/9716d4e3e3a)); the affected code path is unchanged since, so all releases from v25.2 onward are affected.

**Environment**
- CockroachDB version: reproduced on v26.4.0-alpha (dev build); affects v25.2 and later
- Client app: `psql` / any pgwire client

**Additional context**

Impact: silent wrong query results (symptom 1) and spurious transaction aborts (symptom 2) for applications that combine holdable cursors with ordinary transactions, with no error surfaced in the silent-corruption case. No special privileges are required beyond declaring a `WITH HOLD` cursor and running normal transactions that encounter retryable errors.

Jira issue: CRDB-66821

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.