cockroachdb / cockroachdb/cockroach

sql: WITH HOLD cursor persisted at COMMIT can observe writes performed after DECLARE

Open
#172,772 2 comments 0 reactions 0 assignees View on GitHub
A-sql-execution 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**

When a `WITH HOLD` cursor is persisted at `COMMIT`, the remaining rows are read without pinning the transaction's read sequence number to the cursor's declaration point. If (a) the cursor's scan spans more than one KV batch and (b) any statement runs after a write that follows `DECLARE` (advancing the read sequence), the persisted cursor observes writes performed after `DECLARE`. Postgres cursors are insensitive; CockroachDB's own `FETCH` path enforces insensitivity by pinning the read seqnum (there's a regression test for it in `pkg/sql/logictest/testdata/logic_test/cursor`), so the persisted cursor is inconsistent both with Postgres and with rows fetched from the same cursor before `COMMIT`. A post-`DECLARE` `DELETE` would conversely cause persisted rows to go missing.

**To Reproduce**

```sql
CREATE TABLE bigq (a INT PRIMARY KEY, b STRING);
INSERT INTO bigq SELECT g, repeat('a', 1024 * 1024) FROM generate_series(1, 11) g(g);
BEGIN;
DECLARE cq CURSOR WITH HOLD FOR SELECT a FROM bigq ORDER BY a;
INSERT INTO bigq VALUES (100, 'blargh');
SELECT count(*) FROM bigq; -- any statement here; advances the read seqnum
COMMIT;
FETCH ALL cq; -- returns 1..11 AND 100
```

The 1 MiB rows force the scan past the first KV batch (same technique as the existing FETCH sensitivity test). Without the intervening `SELECT`, the bug is masked because no sequence point covering the `INSERT` has been placed when persist runs.

**Expected behavior**

`FETCH ALL` returns rows 1-11 only, matching Postgres and matching the same cursor's behavior if fetched before `COMMIT`.

**Root cause analysis**

[`persistCursor`](https://github.com/cockroachdb/cockroach/blob/master/pkg/sql/sql_cursor.go#L655) runs the cursor to completion via bare `cursor.Next()` calls. The fetch path ([`fetchMoveNodeBase.startInternal`](https://github.com/cockroachdb/cockroach/blob/master/pkg/sql/sql_cursor.go#L264)) brackets the same iteration with `SetReadSeqNum(cursor.readSeqNum)` / restore to preserve insensitivity; `persistCursor` has no such bracketing. The cursor's internal-executor query reads directly through the shared root txn (the sync-iterator + RootTxn path disables DistSQL/Streamer, [internal.go](https://github.com/cockroachdb/cockroach/blob/master/pkg/sql/internal.go#L1223)), so its KV batches are stamped with the read seqnum current at drive time — at persist, that's the commit-time sequence point. The likely fix is to apply the same pin/restore around the persist drain loop.

**Environment:**
- CockroachDB master (v26.4.0-alpha dev build), single node, `cockroach sql`

**Additional context**

Found in the same investigation as #172771 (positioned FETCH broken after persist); independent defect, independent fix.

Jira issue: CRDB-66039

Contributor guide

Open the contributing guide

Research direction

Start in pkg/sql/sql_cursor.go at persistCursor and compare its drain loop with fetchMoveNodeBase.startInternal, then review the existing cursor regression coverage in pkg/sql/logictest/testdata/logic_test/cursor. Reproduce the multi-batch WITH HOLD case from the issue and verify that FETCH ALL after COMMIT returns only rows visible at DECLARE, matching the pre-COMMIT fetch behavior.

Written by the indexing model from the issue text.

Assessment

Tech stack
go, sql
Domain
databases
Issue type
Bug
Difficulty
3/5
Estimated time
1-2 days
Activity status
Quiet
Clarity
Clearly specified
Newbie friendliness
72/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.