Manu <[email protected]> wrote:
> Antonin Houska <[email protected]> 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)
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 incremented
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. Maybe
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 than
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 internal
> 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 index
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 that
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-vjvdPQts6QnHoB3ET5A2or137oN_FGMviQdtYLN6Q%40mail.gmail.com
--
Antonin Houska
Web: https://www.cybertec-postgresql.com
