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

Hi Thom,

Reproduced here on master (07c73f45063), Linux, following your recipe
exactly: the three rows read NEWNEWNEW/12000 before REPACK finishes and
oldoldold/9000 after it, no error anywhere.

One thing worth adding, because it changes how this reads: the
injection point is not needed.

It makes the race deterministic, but the window your analysis describes
- between repack_setup_logical_decoding() taking the toast relfilenode
and copy_table_data() locking the toast table - is wide enough on its
own, because get_initial_snapshot() sits in it waiting for running
XIDs. An ordinary open transaction is enough to hold it open. Without
any injection point:

session A BEGIN; SELECT pg_current_xact_id(); (sleeps 3s, commits)
session B REPACK (CONCURRENTLY) test; -- waits for A
session C VACUUM FULL pg_toast.pg_toast_<oid>; -- inside the gap
session C UPDATE test SET big = ... ; -- right after A commits

5 of 5 runs lost the update.

Script attached; it builds and drops the table on each iteration so the
result is not an artifact of one particular relfilenode.

So this does not need a debug build or a test-only feature to happen.
It needs a long-running transaction, a REPACK (CONCURRENTLY), and
someone rewriting that table's toast relation in the meantime - which
is an odd thing to do by hand, but it is allowed, the parent is only
held with ShareUpdateExclusiveLock as you say, and "run VACUUM FULL on
the biggest toast tables" is the kind of thing maintenance scripts do.

I have not looked for a fix yet. From your description the obvious
question is whether repack_setup_logical_decoding() should hold the
toast lock until copy_table_data() takes it, or whether the relfilenode
should be re-checked after the snapshot is built; the second sounds
cheaper but I have not read enough of that path to have an opinion
worth posting.

Regards,
Manu