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

Hi Shihao,

I tested v1-0001 and v1-0002 on master (cff329240ba), two builds from
the same commit with --enable-cassert --enable-injection-points: one
with only 0002 applied (control) and one with both. 0001 builds with
no new warnings.

1. The race

The script I posted earlier wins the race without an injection point,
so it is a fair before/after check. Same script, same parameters,
against both builds:

master, 0002 only: 5 of 5 runs lost the update
master + 0001: 0 of 5 runs lost the update

2. The test

Thom already reported that the test fails without the fix; this only
adds that it fails for the right reason and nothing else moved:

control: repack_toast FAILS (results differ from expected)
0001: repack_toast passes, 499 ms

The other tests of the injection_points module pass in both builds, so
0002 fails only for the reason it is meant to.

3. The window Thom asked about

> > Is there any opportunity for another rewrite to sneak in between
> > these two?
>
> Yes, but it doesn't matter. The old worker is thrown away and nothing
> has been copied yet. The new worker reads the relfilenode itself when
> it starts, so a rewrite before that is simply what it sees. A rewrite
> after that is caught by the next check, which is made under the lock
> again.

That is the claim I could put under load instead of taking it on
trust. A loop of VACUUM FULL on the toast relation runs for the whole
startup of REPACK, so it lands inside that window many times over,
while an open transaction keeps the worker waiting during setup and an
UPDATE of the TOASTed columns commits right after. With
log_min_messages=debug1 the patch's own DEBUG1 counts the restarts:

hammer 0s: REPACK ended in ~4s, 0 restarts, value correct
hammer 8s: REPACK ended in ~4s, 33 restarts, value correct
hammer 20s: REPACK ended in ~20s, 166 restarts, value correct
hammer 40s: REPACK ended in ~40s, 342 restarts, value correct
master, 20s: REPACK ended in ~5s, n/a, UPDATE LOST

So it holds up under continuous pressure: the value is never wrong, no
deadlock, and the run with no rewrites at all costs nothing (4s, zero
restarts), so the retry loop does not show up on the normal path.

4. One thing worth deciding, not a correctness issue

The numbers above also say that REPACK can be held up for as long as
the rewriting lasts. It is not a hard livelock - with the 8s hammer
it got through in 4s - but with the 20s and the 40s one it finished
only about when the hammer stopped, at roughly 8 restarts per second.
Whether it gets through is a matter of winning the window; the loop
has no cap and no backoff, and each turn starts a worker that waits
for all running transactions again.

I would not call this a bug: correct-but-waiting beats fast-and-wrong,
and a VACUUM FULL loop on a toast relation is not a real workload. But
it is unbounded, and the caller gets no hint of why nothing is
happening. Since 0001 already has the DEBUG1, would it be worth
raising it, or capping the retries and erroring out after N? Your
call - I mention it because the measurement was there.

Script attached (.txt, so the cfbot keeps testing your patches).

Regards,
Manu