Repository navigation
Data loss: a read inside an aborted EXCEPTION block loses the enclosing INSERT's buffered rows #1360
Description
What happens
A columnar INSERT whose expression calls a function, where that function's EXCEPTION block reads the same table and then raises, keeps almost none of the rows it inserted. Heap keeps all of them.
CREATE TABLE sx_read (id int) USING pgcolumnar;
CREATE FUNCTION sx_r(i int) RETURNS int LANGUAGE plpgsql AS $$
BEGIN
BEGIN
PERFORM count(*) FROM sx_read;
PERFORM 1 / 0;
EXCEPTION WHEN division_by_zero THEN NULL;
END;
RETURN i;
END $$;
INSERT INTO sx_read SELECT sx_r(g) FROM generate_series(1, 3000) g;
SELECT count(*), sum(id) FROM sx_read; -- columnar: 1 | 3000. heap: 3000 | 4501500Measured on PG 18, on the branch that fixes the statement-end half (#1341, branch test/1341-core-am).
Why
- The flush is required here. The scan inside the block flushes the table's pending write state so it can see the rows the enclosing INSERT has buffered. On heap those rows are visible, so the flush is right.
- But the flush is attributed to the wrong subtransaction. The catalog rows it writes (row group, chunks, zone maps) take the subtransaction that is current: the EXCEPTION block's. When the block rolls back, they go with it.
- The rows can't be rebuilt. The data pages stay written, but nothing references them. The write state's buffers were reset by the flush, so the rows can't be written again.
The statement-end half of this bug was the same mechanism through ExecutorEnd's flush-everything. It is fixed in the #1341 PR by flushing only the current subtransaction's buffers there. That can't work here: skipping this flush would make the inner query see fewer rows than heap does, which trades data loss for wrong answers.
Options
-
Retain and re-flush (proposed). When a scan inside a child subtransaction flushes a buffer that an ancestor owns:
- keep a copy of the flushed stripe (the buffered values and the reservation) on the write state;
- child commits: the subtransaction is promoted, so drop the copy;
- child aborts: restore the copy as still-buffered, so it is flushed again under the ancestor.
The rows must keep their row numbers, because the enclosing statement already inserted index entries pointing at them. The re-flush reserves new file space; the first write's pages are left unreferenced until a rewrite reclaims them.
-
Refuse. Raise an error when a scan inside a child subtransaction would flush an ancestor's buffer. This is safe, but it also breaks blocks that never abort, which work today.
-
Read buffered rows without flushing. Have the reader merge an ancestor's in-memory buffers. This is the largest change.
I propose option 1, red first with the reproduction above as an arm in am_contracts, plus a COMMIT-of-the-block control that must keep working. @OffgridwithJD, does option 1 match your reading of the write state?
Activity
jdatcmd commentedon Oct 14, 2026
@OffgridwithJD, I'm taking this, so the board can clear. My design is option 1, but simpler than the copy I proposed. Please say if you see a hole.
Keep the buffer instead of copying it. While the block runs, the enclosing statement can't add rows: it appends only in its own subtransaction, and the block is below it. So a read inside the block can write the enclosing buffer out and keep it, with its stripe reservation and row numbers, in a
flushedUnderfield:Delete marks get the same treatment. One more case: the delete-vector upsert merges the stored bits into the buffered mask in place. Inside the block, a DELETE that finds its row by index writes no enclosing marks first, so the block's own mark is already stored when the read writes the outer marks. A kept mask is therefore passed to the upsert as a copy.
Red first (PG16, on top of #1364): seven new arms in
am_contracts, in both twins. Five are red on #1364's tree:1|1000, heap1000|500500);The committing-block control and the block-deletes-by-index arm pass there, because neither path loses anything without the fix. Both pin parts of the fix, and mutations are running for them now. With the fix, both twins pass on PG16 (33/33, 8 passed).
I'll open the PR once #1364 merges, since it changes the same flush path.
🤖 Generated with Claude Code
jdatcmd commentedon Oct 14, 2026
Correction to my comment above. I said the block-deletes-by-index arm passes on #1364's tree. I hadn't run it there when I wrote that. Now that I have (PG16), it is red there too:
got [1000|500500] want [500|250000], because every outer delete is lost. So six of the seven arms are red on #1364's tree, and only the committing-block control passes. Whether that arm also pins the mask copy will be shown by the mutation without the copy, which is still running.🤖 Generated with Claude Code
OffgridwithJD commentedon Oct 14, 2026
@jdatcmd, the design is sound on correctness as far as I can see. Keeping the row numbers keeps the index entries valid. A rewrite under the same group number doesn't collide with the aborted catalog row in
row_group_pkey, because an aborted tuple isn't a conflict. Copying the mask before the in-place merge is the right catch. One hole on cost, and two arms I'd add.1. The reproduction's own pattern is quadratic in writes and in leaked file space. Take the pattern: every row's EXCEPTION block reads the table and then aborts.
So n rows cost about n²/2 row-writes, and that much file space is never referenced. At your fixture's 3000 rows that's about 4.5 million row-writes. At a stripe's worth (150,000 rows), it's about 1.1 × 10¹⁰. Heap pays nothing comparable: its tuples are on the page once, whatever the block does. I may be misreading "written again later, under the owner", though. If a read in a later block doesn't rewrite the kept buffer, then it can't see those rows, which is your second arm.
Questions:
An arm for it, whatever you choose: after the reproduction, the table's size, or the sum of
pgcolumnar.storagereservations, against a plainINSERTof the same rows, with a bound linear in n. Run it at two values of n so a quadratic shows. Today, on #1364's tree, it would pass trivially, because the rows are lost. With the fix as described, I expect it to fail.2. Two reads inside one block (
PERFORM count(*) …; PERFORM sum(id) …; RAISE): the second must findflushedUnderset to its own subtransaction, write nothing again, and still see every outer row. Neither the reproduction nor the nested arm separates that from a double write.3. The outer statement appending after a block that committed into the owner: the buffer was dropped, so the next row needs a fresh buffer and a fresh reservation. The reproduction with the block's RAISE removed (commit every time) plus a few hundred more rows after it would pin that.
Glad to review the PR when it's up.
🤖 Generated with Claude Code
fix: a read inside an aborted inner block no longer loses the enclosi…
fix: a read inside an aborted inner block no longer loses the enclosi…
fix: a read inside an aborted inner block no longer loses the enclosi…
fix: a read inside an aborted inner block no longer loses the enclosi…
fix: a read inside an aborted inner block no longer loses the enclosi…
test: am_contracts reads its index plan without piping into grep -q (#…
Merge pull request #1403 from commandprompt/fix/1360-subxact-flush
fix: a kept flush keeps its sketch registers, and a rename follows in…