Re: REPACK enhancements

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

> Hi Antonin,
>
> On Tue, Nov 10, 2026 at 11:54 AM Antonin Houska <ah(at)cybertec(dot)at> wrote:
> > For now I commented the assertion out and added a comment on possible
> > solutions.
>
> Leaving it out looks safe to me, and the check can stay. Every other
> caller passes GetCurrentTransactionId(), and in all the runs I made
> (UPDATE, DELETE, savepoints, TOAST, row lockers, 150 replayed calls)
> the replay reached the function with xmax still zero: the copy clears
> it and only REPACK writes to the new heap. So the attached assertion
> keeps the old check and states what REPACK relies on;

...

> diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
> index 19be1a3995b..9086b79bc0c 100644
> --- a/src/backend/access/heap/heapam.c
> +++ b/src/backend/access/heap/heapam.c
> @@ -5578,12 +5578,14 @@ compute_new_xmax_infomask(TransactionId xmax, uint16 old_infomask,
> new_infomask2;
>
> /*
> - * TODO Move this statement to callers? REPACK (CONCURRENTLY) uses
> - * add_to_xmax different from the current transaction when replaying data
> - * changes. Alternatively, we can add a boolean argument to the function
> - * indicating the special case, but that does not look nice.
> - */
> - //Assert(TransactionIdIsCurrentTransactionId(add_to_xmax));
> + * REPACK (CONCURRENTLY) replays data changes with the XID of the
> + * transaction that made them (TABLE_REUSE_XID), so add_to_xmax need not
> + * be ours. It only replays them on tuples of the new heap that nobody
> + * else can lock or modify and whose xmax it has not set, so xmax is
> + * still invalid here and our own transaction does not matter below.
> + */
> + Assert(TransactionIdIsCurrentTransactionId(add_to_xmax) ||
> + !TransactionIdIsValid(xmax));
>

I don't really understand why 'xmax' should be involved. The original
assertion checks exactly whether 'add_to_max' is the "current transaction" and
I tried to disable it exactly when the function is called by REPACK (to replay
data changes). However, 'xmax' can be invalid even in cases unrelated to
REPACK (i.e. when doing "regular" UPDATE / DELETE). Thus what you propose
makes the assertion a bit less useful.

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