Re: REPACK enhancements

Manu <manuelreyesbravo(at)gmail(dot)com> wrote:

> 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.

When tuple is copied, xmax needs to be set to invalid. If the deleting
transaction commits, it'll set the xmax during replay. I could fix it locally,
will include the fix in the next patch version.

> 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.

Perhaps we need to use more specific name for CONCURRENTLY.

> 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

Interesting is that the pg_constraint catalog allows duplicate constraint
name, as long as the constraints are on different relations. Again, the
transient table obviously needs a different constraint name.

> 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.

I don't know at the moment when the XID could get assigned. I need to do some
investigation.

> 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,

Do you mean that we should add variants of WRITE_NEW_HEAP and REBUILD_INDEX
specifically for the auxiliary table?

> and SORT_TUPLES and WRITE_NEW_HEAP reported around the build and the scan of
> the auxiliary table's index.

With the auxiliary table, sorting IMO hapens in two phases: 1) build the
clustering index and 2) scan the index and insert the output into the new
heap. As long as each phase is reported on its own, I don't see room for
SORT_TUPLES.

> A small thing: your diff makes gcc warn that nblocks may be used
> uninitialized in heapam_handler.c.

I'll fix that.

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

Thanks for review, I'll reflect it in the next patch version.

--
Antonin Houska
Web: https://www.cybertec-postgresql.com