From: Antonin Houska <ah@cybertec.at>
To: Manu <manuelreyesbravo@gmail.com>
cc: shihao zhong <zhong950419@gmail.com>,
    pgsql-hackers@lists.postgresql.org
Subject: Re: REPACK enhancements
In-reply-to: <179004809718.4166269.6878292164216131831@gmail.com>
References: 
 <CAGRkXqRX2baaDYHjHLK511vL3kuT8qmZRXbx+bgHCPcgyKsEjw@mail.gmail.com>
 <179004809718.4166269.6878292164216131831@gmail.com>
Comments: In-reply-to Manu <manuelreyesbravo@gmail.com>
   message dated "Tue, 22 Sep 2026 00:34:57 -0300."
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <28302.1790158015.1@localhost>
Content-Transfer-Encoding: quoted-printable
Date: Wed, 23 Sep 2026 12:06:55 +0200
Message-ID: <28303.1790158015@localhost>
Archived-At: 
 <https://www.postgresql.org/message-id/28303.1790158015%40localhost>
Precedence: bulk

Manu <manuelreyesbravo@gmail.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 loca=
lly,
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 s=
ome
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 tabl=
e's
> own indexes are built,

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

> and SORT_TUPLES and WRITE_NEW_HEAP reported around the build and the sca=
n 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%3DShLZA99FNHF=
Oc5kjdPRzfOZLgSdcm07uy28g%40mail.gmail.com

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

-- =

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