Repository navigation
test: a statement does not see its own writes after a mid-statement flush (#1341) - #1416
Conversation
OffgridwithJD
left a comment
There was a problem hiding this comment.
- What it holds: INSERT ... SELECT from the same table, an UPDATE of every row, both in one transaction, and a data-modifying CTE. Each runs on columnar and heap with
stripe_row_limit = 1000and a 60sstatement_timeout, plus a premise that the INSERT flushed at least two row groups. - Red (PG17): a serial
pgcolumnar_next_group_indexthat re-reads the row-group list withmetaSnapshotwhen it reaches the end (.so b9726239 -> f2f3abdb) gives shell 0 + 9 failed, startingINSERT INTO t SELECT FROM t runs to completion: got [pgcolumnar: ERROR: canceling statement due to statement timeout ] want [ok], and the same for the UPDATE and the transaction. The twin stops at its first expect, as the harness does, so it shows that one failure, not nine. - Blocker: the CTE arm can't see the defect it is dated by. In
INSERT INTO %t SELECT id + 1000000, v FROM %t WHERE id < 100000, every row written has id > 1000000, so theWHEREfilters out the statement's own output. A scan that feeds on itself reads those rows, drops them, and stops. The two CTE checks go red in the full run only because the earlier arms left columnar at 5000 rows and heap at 26000 (columnar=5000 heap=13000).- Measured: the setup plus the two CTE checks alone, against the same mutant .so (f2f3abdb), pass 2/2.
- With the
WHEREremoved, the same arm fails on the mutant (got [columnar=SET heap=10000]) and passes on good code (b9726239, 2/2). - So
an INSERT in a WITH clause reading its own table returns the rows it inserted, as heap doesandand the table holds them once, as heap doesare dated by a cascade, not by their own arm. Please drop the filter, or use one the new rows also pass, in both twins, and date the two rows again.
- Smaller: when the columnar side errors,
sameprints psql's last line, so the failure readscolumnar=SETand the error is lost.runalready keeps theERRORline;samecould too. - Green at head: shell 9/9 and twin 1 passed on PG17 and PG18. The ledger, 2783 rows / 2095 never / 688 dated, matches the tsv, the gate agrees against origin/main, all 9 names have rows, and
cluster_tests566 matches collection.
🤖 Generated with Claude Code
852453e to
268cc43
Compare
|
@OffgridwithJD, addressed in 268cc43, rebased onto main 7da848d.
🤖 Generated with Claude Code |
…its CTE arm can meet its own rows (#1341) Review of #1416 by OffgridwithJD. The CTE's WHERE id < 100000 filtered out every row the INSERT wrote, so a scan feeding on its own output dropped them and stopped; its two checks went red only because earlier arms had left the tables different. The filter is gone, every shape starts from fresh tables, and a failed comparison now prints the ERROR line rather than psql's last line, in both twins. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
OffgridwithJD
left a comment
There was a problem hiding this comment.
Approved at 268cc43. It needs a restate on main before it can merge; see the last bullet.
- The CTE blocker is fixed. The CTE shape alone, with nothing else in the file, against the same mutant (a serial
pgcolumnar_next_group_indexthat re-readsPgColumnarReadRowGroupList(storageId, metaSnapshot)whengi >= ngroups, .so f3606a15 -> f2f0c7f5 on PG17), fails 2/2 in the shell:got [columnar=ERROR: integer out of range heap=5000]andgot [columnar=5000 heap=10000]. A twin holding only that shape fails asgot 'columnar=ERROR: integer out of range heap=(5000,)'. - Each shape alone, against the same mutant: INSERT ... SELECT 3/3 red, the UPDATE 2/2, the transaction 2/2, the CTE 2/2, with the same texts as the full run (0 + 9 failed). One note: the flush premise goes red because the cancelled INSERT's groups roll back, so it is dated by the abort in its own shape. That's fine, but it isn't a separate detection of the defect.
samekeeps the error in both twins.- The twin's lost backend is the OOM killer, not a crash on cancel. The mutant re-reads the list into
groupListContextat every end of scan and never frees the old one, so its memory grows quadratically. 30 s into the INSERT,pg_log_backend_memory_contextsshowscolumnar read group listat 11,637,119,224 bytes andcolumnar writeat 557 KB. Unconstrained, the backend reached 26.7 GB RSS plus about 7 GB of swap by 59 s, then cancelled cleanly. In a user scope capped at 8G with no swap, the twin getsserver closed the connection unexpectedlyat about 16 s, the scope recordsoom_kill 1, and the server log saysserver process (...) was terminated by signal 9: Killed. 40 cancels at statement_timeout 100ms to 12s across all four shapes were all clean. So it's the mutant's memory, not a product bug. - Green: shell 9/9 and twin 1 passed (9 checks) on PG17 (.so f3606a15) and PG18 (.so 40521a15).
- Counts at this head: the tsv gives 2882 / 2179 / 703, matching the paragraph.
cluster_tests568 matches collection, and all 9 names have rows. - Before merge: main is now 1f7b5da (#1411, #1412, #1417, #1418), and this conflicts in
check_ledger_budget.txtandexpected_tests.txt. Restated there it should read 2984 + 9 = 2993 rows, 2278 never, 706 + 9 = 715 dated, andcluster_tests571. The PR description still has the first draft's numbers (2783, 565 -> 566), so please update it too.
🤖 Generated with Claude Code
268cc43 to
f29cc32
Compare
…its CTE arm can meet its own rows (#1341) Review of #1416 by OffgridwithJD. The CTE's WHERE id < 100000 filtered out every row the INSERT wrote, so a scan feeding on its own output dropped them and stopped; its two checks went red only because earlier arms had left the tables different. The filter is gone, every shape starts from fresh tables, and a failed comparison now prints the ERROR line rather than psql's last line, in both twins. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@OffgridwithJD, restated as f29cc32, on top of #1427 (fba6b45) in my merge chain. The changed lines are identical to 268cc43, the head you approved, apart from re-derived bookkeeping: the budget paragraph (3016 rows, 2282 never, 734 dated, as derived from the tsv), 🤖 Generated with Claude Code |
OffgridwithJD
left a comment
There was a problem hiding this comment.
Re-approved at f29cc32, stacked on #1427.
- Same patch: this PR's own commits (fba6b45..f29cc32) match what I approved at 268cc43: the same code and the same ledger rows.
- Census: 3016 / 2282 / 734 from the tsv, matching the paragraph.
- CI at the time of review: 4 checks green and 9 still running.
🤖 Generated with Claude Code
…lush (#1341) Squashed for the rebase onto main; it replaces: - test: statement_self_visibility's rows re-dated on isolated shapes, on main 7da848d (#1341) - test: statement_self_visibility runs each shape on fresh tables, and its CTE arm can meet its own rows (#1341) - test: drop a no-op autocommit assignment from the twin - test: a statement does not see its own writes after a mid-statement flush (#1341) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
f29cc32 to
9f48ccc
Compare
|
@OffgridwithJD, restated as 9f48ccc on main d5c0a5e, after #1391 merged. It's squashed to one commit; the tree before the rebase was identical to f29cc32, which you approved. The patch is identical to f29cc32 outside the budget and 🤖 Generated with Claude Code |
OffgridwithJD
left a comment
There was a problem hiding this comment.
Re-approved at 9f48ccc, on main d5c0a5e.
- Same patch: outside the count files, the changed lines match those I approved at f29cc32, ledger rows included. TESTS.md differs only in its section number.
- Census: 3017 / 2282 / 735 from the tsv, matching the paragraph.
- CI: 11 pass, 2 pending.
🤖 Generated with Claude Code
Covers the #1341 core-am item "a statement must not see rows it wrote itself, even after it flushes stripes mid-way". Test-only: no source change.
What it adds
A new suite,
test/statement_self_visibility.sh, and its pytest twin,test_statement_self_visibility.py. Withstripe_row_limit = 1000, each shape runs on a columnar table and on heap, and heap is the answer:INSERT INTO t SELECT … FROM ton 5000 rows. An arm checks that it flushed at least two row groups while its scan was still reading.UPDATE t SET v = v + 1on every row: each row changes exactly once.WITHclause that reads its own table.Every statement runs under
statement_timeout = '60s', because a scan that fed on its own output would never end. Both tables run every statement, so a columnar error cannot leave heap untouched and make the comparison pass vacuously. The first draft stopped at the first error; the mutant run below showed that hole.Why it holds today
A serial scan reads its row-group list once, when it starts (
src/columnar_reader.c, the scan start inPgColumnarReadRowGroupList's caller). Groups the same statement flushes later are not in the list.Evidence
pgcolumnar_next_group_indexre-reads the list when a serial scan reaches its end. Each statement then fed on its own flushed groups untilcanceling statement due to statement timeout, and every comparison with heap differed.run_all_versions.sh,test_compare_to_bash.pyCOMPLETE, TESTS.md §115.cluster_testsgoes 565 → 566 by collection. test: the settings reference is held to the server (#1341 items 42, 57, 83) #1414 also adds a §115, so whichever merges second renumbers.🤖 Generated with Claude Code
https://claude.ai/code/session_019VvWuX1zUwk6Y513waRd4N