Manu <[email protected]> wrote: > Hi Antonin, > > On Tue, Nov 10, 2026 at 11:54 AM Antonin Houska <[email protected]> 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
