From: Antonin Houska <ah@cybertec.at>
To: Manu <manuelreyesbravo@gmail.com>
cc: pgsql-hackers@lists.postgresql.org
Subject: Re: REPACK enhancements
In-reply-to: <179017302006.2626644.6788432141295311288@gmail.com>
References: <28303.1790158015@localhost>
 <179017302006.2626644.6788432141295311288@gmail.com>
Comments: In-reply-to Manu <manuelreyesbravo@gmail.com>
   message dated "Wed, 23 Sep 2026 11:17:00 -0300."
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <18679.1790247323.1@localhost>
Content-Transfer-Encoding: quoted-printable
Date: Thu, 24 Sep 2026 12:55:23 +0200
Message-ID: <18680.1790247323@localhost>
Archived-At: 
 <https://www.postgresql.org/message-id/18680.1790247323%40localhost>
Precedence: bulk

Manu <manuelreyesbravo@gmail.com> wrote:

> Antonin Houska <ah@cybertec.at> wrote:
> =

> > > 3. 0008: assertion failure in compute_new_xmax_infomask()
> >
> > I don't know at the moment when the XID could get assigned. I need to
> > do some investigation.
> =

> I measured it, so you don't have to.  gdb attached to the backend
> running REPACK, breakpoint on AssignTransactionId(), backtrace on every
> hit (script attached).  In a whole REPACK (CONCURRENTLY) run there is
> exactly one hit, and it is this one:
> =

>   #0  AssignTransactionId (xact.c:644)
>   #1  GetCurrentTransactionId (xact.c:461)
>   #2  LogAccessExclusiveLockPrepare (standby.c:1465)
>   #3  LockAcquireExtended (lock.c:1008)     lockmode=3D8
>   #4  LockRelationOid (lmgr.c:115)          relid of the new heap
>   #5  heap_create_with_catalog (heap.c:1294)  "pg_temp_16384"
>   #6  make_new_heap (repack.c:1709)
>   #7  make_new_heap_for_repack (repack.c:1389)
>   #8  rebuild_relation (repack.c:1230)

If this was the reason, the assertion would be pretty easy to reproduce.

> ...

> That makes the Assert unsatisfiable from that side: by the time the
> replay calls heap_update() for a change whose xmax belongs to another
> transaction, GetTopTransactionIdIfAny() is necessarily valid.  It is
> not that the XID is assigned too early by accident - there is nowhere
> later to move it, short of creating the new heap in a separate
> transaction.

This XID assignment is fine - make_new_heap_for_repack() eventually starts=
 a
new transaction, and the reason for it is exactly to get rid of the XID.

> > > 4. Progress reporting
> > ...
> > Do you mean that we should add variants of WRITE_NEW_HEAP and
> > REBUILD_INDEX specifically for the auxiliary table?
> =

> Not necessarily - I think there are two separate things in there, and
> only the second one is a design question.
> =

> The first is a plain reporting bug: build_new_index() sets the phase
> for indexes that are not the table's own - the identity index of the
> empty new heap

This one *is* the table's own. Admittedly, if the table is empty, it's not
really index rebuild. Although reporting rebuild for pretty short time is
probably not a serious problem (the user can hardly notice this waiting), =
we
might want to skip updating the REPACK progress in this case
altogether. Moreover, we can skip the actual build, as an existing comment=
 in
make_new_heap_for_repack() indicates:

    * XXX NewHeap is empty - should we pass INDEX_CREATE_SKIP_BUILD?

However, I don't know if REPACK_INDEX_REBUILD_COUNT should then be increme=
nted
or not. If we increment it, we still consider buidling an empty index a
"rebuild". If we don't, the final value of the counter will be wrong. Mayb=
e
increment the counter when the data copying is done, but that seems
unnecessarily complicated.

> and the indexes of the auxiliary table.

This might be worth a separate phase: if we report the phase
REPACK_PHASE_REBUILD_INDEX, we should probably also increment the
REPACK_INDEX_REBUILD_COUNT counter. And if indexes on the auxiliary table =
are
included in the counter, user might be confused to see more index builds t=
han
the number of indexes on his table.

> That alone is what makes "rebuilding index" appear before "seq scanning
> heap", and twice with USING INDEX.  Not setting the phase for those inte=
rnal
> builds fixes the order without touching the catalog or the docs.

I concur with [1] that order is not a problem.

> The second is what the auxiliary table's work should be reported as,
> and there I would rather not add new values.  Today, with v03 and
> USING INDEX, SORT_TUPLES and WRITE_NEW_HEAP are never reported at all,
> while the "REPACK Phases" table in monitoring.sgml still says
> "REPACK is currently sorting tuples" and "REPACK is currently writing
> the new heap".  A user watching pg_stat_progress_repack on v19 and on
> v20 would see two documented phases disappear.

WRITE_NEW_HEAP should be used in process_auxiliary_table() - I'll fix that=
,
thanks. Regarding SORT_TUPLES, the actual "sorting" is implemented as an i=
ndex
build and scan, so there's no need to report this phase. And no, the phase
will not disappear from the documentation because REPACK w/o CONCURRENTLY =
will
still use it.

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

> Your two phases map onto the two existing values, I think: (1) is
> where the tuplesort actually runs, so that is SORT_TUPLES, and (2) is
> WRITE_NEW_HEAP.  That is the room for SORT_TUPLES - the sort is the
> index build.  It keeps the documented set of phases intact and needs
> no catversion bump.

I see your point now, but not sure this is a good idea. If the user sees t=
hat
sorting takes too much time, and if he's familiar with PG internals, he's
likely to conclude that increased maintenance_work_mem will help. However,=
 in
this special case it will not because the tuplesort engine is not involved=
.

[1] https://www.postgresql.org/message-id/CAN12%2BYKo-vjvdPQts6QnHoB3ET5A2=
or137oN_FGMviQdtYLN6Q%40mail.gmail.com

-- =

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