cockroachdb / cockroachdb/cockroach
sql: WITH HOLD cursor persisted at COMMIT can observe writes performed after DECLARE
- 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
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