Re: REPACK (CONCURRENTLY) can silently lose updates when the toast table is rewritten

Hi Shihao,

> Agreed. v2 attached. REPACK now fails if the TOAST table was
> rewritten, and the user can run it again.

I ran the same checks as for v1 against v2, on cff329240ba with
--enable-cassert --enable-injection-points, next to a control build
with only v2-0002. Both patches apply cleanly and build with no
warnings.

1. The race without an injection point (VACUUM FULL of the TOAST
relation while the worker waits, UPDATE right after), 5 runs each:

control: REPACK succeeds, update lost in 5 of 5
v2: REPACK fails in 5 of 5, update kept in 5 of 5

ERROR: could not execute REPACK (CONCURRENTLY) on relation "test"
DETAIL: The TOAST relation was rewritten concurrently.
HINT: The transaction might succeed if retried.

2. VACUUM FULL of the TOAST relation in a loop for 20 s, over the
whole startup: v2 fails after 3.6 s with the same error, and the
update is kept. With v1 the same run took about 20 s and 166 worker
restarts, so the unbounded wait I mentioned for v1 is gone.

3. No rewrite at all: REPACK succeeds in 3 of 3, 2.5-2.6 s, the same
as the control.

4. Thom's deadlock case, where a transaction that already has an XID
locks the TOAST relation while the worker waits for it:

REINDEX TABLE of the TOAST relation (lock, no rewrite)
v2: REPACK succeeds, no deadlock, update kept
CLUSTER of the TOAST relation (lock and rewrite)
control: REPACK succeeds, update lost
v2: REPACK fails with the error above, update kept

So taking the lock after the worker's setup does what the commit
message says: no deadlock, and a clear error when the rewrite does
happen.

5. Tests: repack_toast fails on the control and passes with v2. With
v2 all injection_points tests pass (4 regress, 14 isolation), and so
do make check (239) and src/test/isolation (133).

The script is attached (.txt, so the cfbot keeps testing your
patches).

Regards,
Manu