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

Hi,

I read the approaches this thread went through: Srinath's ForRewrite()
recomputation of the rewrite's OldestXmin, the registered non-vacuumable xmin,
and the tolerant detoast v5 implements. The last one looks right to me:
Alexander's stress runs showed the first still races, Matthias found the
second cannot be guaranteed, and a recently-dead tuple whose chunks a vacuum
already removed is dead to every snapshot, so dropping it at detoast time
loses nothing. But one shape still fails on v5: an ordinary UPDATE, followed
by VACUUM FULL or REPACK, hits "missing chunk" as before. Details and a fix
below.

The mechanics first: v5 needs one rebase step -- master took
t/055_cascade_reconnect.pl in b614de4876b, so the new TAP test has to move to
056, in the file name and in src/test/recovery/meson.build. With that, both
patches apply cleanly to master as of 9716f88ddf6; I built and tested on
db0c984b1d1. No warnings; make check, the rewrite_stale_xmin spec and the 056
TAP test pass. On the reported shape all four rewrite paths -- CREATE INDEX,
CLUSTER, VACUUM FULL, REPACK -- fail on master and pass with v5.

1) The failing shape. In rewrite_stale_xmin.spec:

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

and two of its four permutations then error out:

step s3_vacuum_full:
VACUUM FULL rewrite_test;

ERROR: missing chunk number 0 for toast value 16463 in rewrite_test_toast

REPACK fails the same way; CLUSTER and CREATE INDEX pass.

The UPDATE leaves an update pair in rewrite_test: A, the old version, recently
dead, its chunks reclaimed by s3_vacuum_toast; B, the new version, live. The
scan meets A first:

/* the caller asks for tolerance: A is recently dead */
reform_and_rewrite_tuple(tuple, ...,
recently_dead ? TOAST_MISSING_OK : 0)

/* but rewrite_heap_tuple() stashes A without the flags */
unresolved->old_tid = old_tuple->t_self;
unresolved->tuple = heap_copytuple(new_tuple);

Then the scan meets B, and one rewrite_heap_tuple(..., flags = 0) call writes
both tuples:

for (;;)
{
/*
* Pass 1 writes B: flags = 0, fine, B is live.
* Pass 2 writes the stashed A, still with flags = 0: re-toasting
* A for the new heap reads its reclaimed chunks without the
* tolerance -> "missing chunk".
*/
if (!raw_heap_insert(state, new_tuple, flags))
...
if (unresolved != NULL)
{
new_tuple = unresolved->tuple;
...
/* loop back to insert the previous tuple in the chain */
continue;

Note the failure above never reaches end_heap_rewrite() -- the site Zhijie
named, rewriteheap.c:313 with its hardcoded 0: B is live and always written,
so A is resolved inside this loop. A stashed tuple reaches
end_heap_rewrite() only when its successor was never written, i.e. B was
found DEAD -- the case the header comment calls "very unusual". A fix at
:313 alone would not cover this. 0001 below adds UPDATE permutations for
both failing commands to the spec.

2) The fix has a wrinkle: the failure cannot be reported through
rewrite_heap_tuple()'s return value. The caller's failure branch runs once
per scanned tuple:

if (!reform_and_rewrite_tuple(tuple, OldHeap, NewHeap,
values, isnull, rwstate,
recently_dead ? TOAST_MISSING_OK : 0))
{
Assert(recently_dead);
/*
* Missing TOAST chunks for a recently-dead tuple. Treat
* it as dead.
*/
*tups_vacuumed += 1;
*num_tuples -= 1;
*tups_recently_dead -= 1;
continue;
}

For the update pair above it executes twice:

scanning A recently dead, so flags = TOAST_MISSING_OK -- but
rewrite_heap_tuple() only stashes A into
rs_unresolved_tups and returns true; nothing is written,
the branch does not run
scanning B live, so flags = 0; the loop writes B, finds the A
stashed one round earlier waiting in rs_unresolved_tups,
and fails writing it -- still with flags = 0

If A's failed insert just returned false, this branch would now run with
tuple = B: the Assert fires (B is not recently dead), num_tuples uncounts
the B that was in fact written, and the dropped A stays counted as kept.

The root cause is that the flags are lost at the rs_unresolved_tups stash,
so 0001 attached stores them there:

typedef struct
{
TidHashKey key; /* expected xmin/old location of B tuple */
ItemPointerData old_tid; /* A's location in the old heap */
HeapTuple tuple; /* A's tuple contents */
+ uint32 flags; /* raw_heap_insert flags A was scanned with */
} UnresolvedTupData;

writes both sites -- the loop and end_heap_rewrite() -- with them, and drops a
stashed tuple whose insert fails on the spot -- the treatment this branch
gives its own tuple. end_heap_rewrite() reports the drops for the caller to
recount:

-extern void end_heap_rewrite(RewriteState state);
+extern double end_heap_rewrite(RewriteState state);

/* caller: dropped tuples were all recently dead -- count as vacuumed */
*tups_vacuumed += tuples_dropped;
*num_tuples -= tuples_dropped;
*tups_recently_dead -= tuples_dropped;

A tuple dropped in the loop can leave a still-older version waiting in
rs_unresolved_tups; it is written by end_heap_rewrite(), now with its own
flags, which also covers the end-of-scan case Zhijie described.

With 0001 the UPDATE permutations pass and the DELETE permutations' output is
unchanged; Zhijie's repro script from upthread, which fails the same way on
v5, completes ("found 500 removable, 500 nonremovable row versions"); make
check, the spec and the 056 TAP test stay green.

3) The leak Andrey mentioned on the ExecQual() continue path is easy to see at
scale. 800 recently-dead rows with a 391 kB external value each, CREATE INDEX
on v5: with a plain index the backend peaks at 17 MB RSS; with a partial index
whose predicate rejects every row, at 451 MB. The missing-TOAST skip path a
few lines above has the mirror problem -- it frees the values but not the
tracking set:

/* final cleanup of this iteration's memory */
bms_free(detoasted_attrs);
/* detoasted_attrs = NULL is missing -- the success path has it */
continue;

so the next recently-dead tuple calls bms_add_member() on the freed set.
0002 attached moves the cleanup into one helper called on all three exits of
the scan-loop iteration. With it the partial-index build holds one value at
a time: the backend's memory context total peaks at 1.4 MB
(pg_log_backend_memory_contexts), where v5 passes 200 MB mid-build. make
check, the spec and the 056 TAP test stay green with 0002 as well.

4) On the scan-and-sort check/use gap: for CLUSTER, VACUUM FULL and REPACK it
is already closed against VACUUM by the lock copy_table_data() takes:

/*
* If the OldHeap has a toast table, get lock on the toast table to keep
* it from being vacuumed.
...
*/
if (OldHeap->rd_rel->reltoastrelid)
LockRelationOid(OldHeap->rd_rel->reltoastrelid, lmode);

Measured on a 4.7 GB toast relation, with a VACUUM of it started from a second
session mid-command:

CLUSTER blocked, ungranted in pg_locks, until the command ends
VACUUM FULL blocked
REPACK blocked
CREATE INDEX runs to completion (it holds only detoast's
AccessShareLock on the toast relation)

So in the sort path, chunks cannot go away through a plain VACUUM between the
pre-check and the sort read-out; the index build is the path where reclamation
proceeds concurrently.

One caveat: the reclamation itself does not need a VACUUM. Once the deleting
transaction is behind the database-local horizon, any plain read of the toast
heap prunes the chunks:

-- toast page of a just-deleted row, before and after a bare
-- SELECT count(*) FROM pg_toast.<toast rel>; no VACUUM anywhere
SELECT lp, lp_flags FROM heap_page_items(get_raw_page(..., 0));
-- before: lp_flags = 1 (normal) for all six chunks
-- after: lp_flags = 3 (dead)

During these three commands the old toast is then only read by the rewriting
backend itself, and I could not turn that into a failure. Sort path forced,
620 MB of toast, rows recently dead only because of a read-only REPEATABLE
READ holder, holder exits during "writing new heap" with three seconds of
strict read-out detoasting still to go: CLUSTER completes. The reason is in
GlobalVisTestShouldUpdate():

/*
* The current heuristic is that we update only if RecentXmin has changed
* since the last update. ...
*/
/* does the last snapshot built have a different xmin? */
return RecentXmin != ComputeXidHorizonsResultLastXmin;

The rewrite builds no snapshot after computing its cutoffs, so its pruning
horizon stays where it was at command start, and its own reads cannot prune
the tuples it decided to keep. That holds unless something mid-command
builds a fresh snapshot; I have not looked for such a path.

In short, I did not find a way to trigger the sort-path race: VACUUM is
locked out for the whole command, and the rewriting backend's own reads
prune with a horizon frozen at command start.

5) One cosmetic thing: v5-0002 adds a line of only tabs at
heapam_handler.c:1741.

Thanks,
Rui