Re: REPACK enhancements

Antonin Houska <ah(at)cybertec(dot)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=8
#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)

So it is not the replay that assigns the XID, and it is not a race: it
is REPACK itself, creating the transient heap, long before any change
is applied. heap_create_with_catalog() locks the new relation with
AccessExclusiveLock, and LockAcquireExtended() does

if (lockmode >= AccessExclusiveLock &&
locktag->locktag_type == LOCKTAG_RELATION &&
!RecoveryInProgress() &&
XLogStandbyInfoActive())
{
LogAccessExclusiveLockPrepare();
log_lock = true;
}

and LogAccessExclusiveLockPrepare() is a bare
GetCurrentTransactionId(), so that the standby can release the lock at
commit (its comment explains why).

The two thresholds line up exactly: XLogStandbyInfoActive() is
wal_level >= WAL_LEVEL_REPLICA, and repack.c:1012 already refuses to
run with anything below WAL_LEVEL_REPLICA. So the branch is taken on
every REPACK that is allowed to start at all, and the backend has a
top XID from the moment it creates the transient heap.

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. Whether the right answer is to relax the Assert for this
caller or to change where the XID comes from is your call; I did not
want to guess at it, but the measurement seemed worth having before
you spend time looking.

> > 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 and the indexes of the auxiliary table. That alone is
what makes "rebuilding index" appear before "seq scanning heap", and
twice with USING INDEX. Not setting the phase for those internal
builds fixes the order without touching the catalog or the docs.

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.

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

If you do prefer new values for the auxiliary table, that is fine too,
but then monitoring.sgml and system_views.sql have to move with it. I
have a test module for exactly that check (it cross-reads progress.h,
system_views.sql and monitoring.sgml and compares the documented
phases against the ones a real run reports) in [1]; I'm happy to run
it against your next version either way, before it becomes someone
else's bug report.

One more thing to keep an eye on: Sami Imseih is changing the same
phases in [2] (v2-0001 docs for v19, v2-0002 phases for v20). Worth
syncing so the two don't collide.

[1] https://commitfest.postgresql.org/patch/7331/
[2] https://commitfest.postgresql.org/patch/7330/

Regards,
Manu