Re: BUG #19519: REPACK can fail due to missing chunk for toast value

Hi Matthias,

Thanks for updating the patch. I tested v7 on 4b97fd15fb4 and found the
UPDATE case from my previous mail still fails, plus two cleanup leaks.

1. VACUUM FULL and REPACK fail when a live tuple resolves a stashed
prior version whose TOAST data is missing.

In rewrite_stale_xmin.spec, replace the DELETE with:

UPDATE rewrite_test SET data = repeat('z', 2500) WHERE id = 1;

Both the VACUUM FULL and REPACK permutations abort with assertions
enabled:

TRAP: failed Assert("recently_dead"),
File: "heapam_handler.c", Line: 993

The old version A is stashed before the scan reaches its live successor
B. rewrite_heap_tuple() writes B, then tries A using A's recently_dead
flag. That flag is now correct, but when A's TOAST fetch fails, the
function still returns false. The caller is processing B, so its
Assert(recently_dead) fails.

0001 adapts the failure handling and counting from my previous patch
to v7. rewrite_heap_tuple() returns false only when the caller's own
tuple is skipped. It counts skipped stashed tuples separately, including
those processed by end_heap_rewrite(), and corrects the rewrite counts.
The added tests cover one and two UPDATEs before VACUUM FULL and REPACK,
and check both the surviving rows and pg_class.reltuples.

2. Two cleanup loops leave detoasted values allocated. 0002 fixes both.

In heapam_relation_copy_for_cluster(), cleanup starts at the first
bitmap member but then walks backward:

for (int i = bms_next_member(detoasted, -1);
i >= 0;
i = bms_prev_member(detoasted, i))

Only the first value is freed. The last call should be
bms_next_member(). I used two EXTERNAL text columns with 100,000 bytes
each, deleted the rows while another connection held an older snapshot,
and forced CLUSTER to use a sequential scan and sort. Just before
tuplesort_performsort(), PortalContext used 10,022,200 bytes for 100
rows and 40,041,400 for 400. With 0002, it used 15,800 bytes in both
cases.

In toast_tuple_init(), the missing-TOAST cleanup stops before attribute
zero:

for (; i > 0; i--)

It should include zero. I tested a table whose first two columns are
EXTERNAL text and updated only the second column. As in
rewrite_stale_xmin.spec, I held an XID in another database and an older
snapshot in the test database. I ran VACUUM (PROCESS_TOAST false),
released the local snapshot, then vacuumed the TOAST table. This removed
the old second-column TOAST values but kept the first-column values,
which the new versions still referenced.

VACUUM FULL then reads the old row version. It fetches the first column,
but skips the row when the second column's TOAST data is missing.
The cleanup loop stops before column zero, leaving the first column's
detoasted value allocated. 0002 changes i > 0 to i >= 0 to free it.

Attached are 0001 and 0002 on top of v7. All tests I ran passed.

Regards,
Rui