Re: REPACK enhancements

Hi Antonin, Shihao,

shihao zhong <zhong950419(at)gmail(dot)com> wrote:
> I am still on 0004, so the two fixes attached are against v03-0004 alone.

To stay out of Shihao's way, I looked at 0006 and 0008. v03 applies
cleanly on be00f041a33, the master it was made on (on today's master 0004
needs a rebase after 16735af3e39 and a62ff0829d6). Built with
--enable-cassert, plus "use_sort = false" (Shihao's #1), without which
make check crashes in VACUUM FULL. Controls are be00f041a33 without the
series and the series stopped at each patch.

1. 0008: "could not find target tuple" after a concurrent DELETE/UPDATE

REPACK (CONCURRENTLY) fails when a row is deleted or updated by a
transaction that is still in progress when the copy reads the row, and
that commits after the copy and before the changes are replayed. The
attached isolation spec does it on a 1000-row table (two ranges, the
DELETE in the second one) and fails every time; with UPDATE instead of
DELETE it is the same.

Since the failed run leaves the new heap behind (see 2), I could look at
it with pageinspect: it has exactly one tuple with xmax set, the copy of
that row, with the same xmin/xmax as in the old heap (666/668, 668 being
the DELETE). So I think the copy carries the xmax of the transaction
still in progress, and HeapTupleSatisfiesNewHeap() then takes any valid
xmax as committed, so find_target_tuple() skips the row when the DELETE
is replayed.

Without injection points it shows up too. REPACK (CONCURRENTLY) with
default settings on a 30000-row table while another session keeps
deleting rows, 8 runs per build: be00f041a33 8/8 fine, the series up to
0007 8/8 fine, the whole series 4 of 8 failed. (3 runs each with the
series stopped at 0004, 0005 and 0006 were fine as well.) With 0007,
the spec's scenario doesn't get that far: REPACK waits in
RepackWorkerExport while the DELETE's transaction is open. With 0008
it doesn't wait. Shihao's and your diffs don't change this.

2. 0006: the new heap left behind by a failed run blocks other rewrites

The commit message says a failed run leaves the new heap, and the next
REPACK (CONCURRENTLY) drops it. But that cleanup is only in
make_new_heap_for_repack(), and the leftover is pg_temp_<oid> in the
table's own schema, the same name make_new_heap() uses for everything
else. After one failed run (the attached SQL uses an 'error' injection
point), every other rewrite of the table fails until a REPACK
(CONCURRENTLY) succeeds:

VACUUM FULL t; -- all five:
CLUSTER t USING t_pkey; -- ERROR: relation
REPACK t; -- "pg_temp_16395"
ALTER TABLE t ALTER id TYPE bigint; -- already exists
ALTER TABLE t ADD COLUMN w float8 DEFAULT random();

The leftover is a regular table as far as pg_dump knows, and it comes
with a constraint also named t_pkey (on t_pkey_repacknew), so the dump
does not restore cleanly:

CREATE TABLE public.pg_temp_16395 (id integer, v text);
ALTER TABLE ONLY public.pg_temp_16395
ADD CONSTRAINT t_pkey PRIMARY KEY (id);
-- ERROR: relation "t_pkey" already exists

It is the same after a crash: I stopped the server with -m immediate
while REPACK waited at repack-concurrently-before-lock. After recovery
the leftover was there with a full copy of the data (5176 kB for a
4.6 MB table), VACUUM FULL failed, and pg_upgrade to a new cluster
stopped at the same statement:

pg_restore: error: could not execute query: ERROR: relation "t_pkey" already exists

be00f041a33 leaves nothing in the same test, and pg_upgrade succeeds.

I don't know which fix you'd prefer. Doing the lookup of find_new_heaps()
in make_new_heap() would cover all the rewrites, but not pg_dump or
pg_upgrade on a cluster where nobody rewrites the table afterwards.

3. 0008: assertion failure in compute_new_xmax_infomask()

TRAP: failed Assert("TransactionIdIsCurrentTransactionId(add_to_xmax) || !TransactionIdIsValid(GetTopTransactionIdIfAny())"), File: "heapam.c", Line: 5564

It fails in the replay after AccessExclusiveLock, called from
rebuild_relation_finish_concurrent(), in heap_update() of a replayed
UPDATE. So REPACK already has an XID of its own at that point. The
table had only the identity index, so there was no index build before.

A caveat: I only got it on builds with PROGRESS_DEBUG from [1] defined,
in 11 of 16 runs (REPACK (CONCURRENTLY) with repack_snapshot_after = 50
on a table that has been repacked once, with INSERT/UPDATE/DELETE
running at the same time). I got it 0 times in 8 runs without that
patch, 8 with log_min_messages = debug2, 6 with a table 10 times larger,
and 8 with the patch applied but PROGRESS_DEBUG not defined. With it,
every tuple the copy scans adds a log line, so my guess is timing, but I
can't rule out that the patch plays a part. Is REPACK expected to have
an XID at that point?

4. Progress reporting

With the trace from [1], these are the phases reported (a table with
only its primary key):

be00f041a33 v03
REPACK (CONCURRENTLY) t 1 7 5 6 8 7 1 5 6 8
... USING INDEX t_pkey 1 3 4 7 5 6 8 7 1 5 7 5 6 8
REPACK t [USING INDEX t_pkey] unchanged

build_new_index() sets PROGRESS_REPACK_PHASE_REBUILD_INDEX and now has
other callers: the identity index of the empty new heap, the one of the
auxiliary table, and the clustering index on the auxiliary table, which
is where the sort happens. So "rebuilding index" shows before "seq
scanning heap", and with USING INDEX "sorting tuples" and "writing new
heap" are never shown. Maybe the phase should only be set where the
table's own indexes are built, and SORT_TUPLES and WRITE_NEW_HEAP
reported around the build and the scan of the auxiliary table's index.

With your diff and Shihao's two (the latter adapted by hand to the whole
series, since they are against 0004 alone), plus use_sort: make check
(239 tests), injection_points (4 + 15) and test_decoding (20 + 14 + 6)
pass, and the trace shows no violations in 154,555 progress changes from
REPACK (CONCURRENTLY) in 50-page ranges, with DML, with synchronized scan
starting mid-table, and with USING INDEX. Without Shihao's syncscan
diff I saw his #3 too: that REPACK went on at about one block every 5 to
10 seconds (439 of 885 after a minute). A small thing: your diff makes
gcc warn that nblocks may be used uninitialized in heapam_handler.c.

[1] https://www.postgresql.org/message-id/CA%2BbCEdBKvmoOd%3DShLZA99FNHFOc5kjdPRzfOZLgSdcm07uy28g%40mail.gmail.com

Regards,
Manu