Re: BUG #19760: CLUSTER loses a row the transaction inserted and then updated in a rolled-back subtransaction

Hi,

On Wed, Oct 21, 2026 at 3:13 PM Michael Paquier <michael(at)paquier(dot)xyz> wrote:
>
> On Tue, Oct 20, 2026 at 09:20:18PM -0400, shihao zhong wrote:
> > Agreed, and thanks for pointing at both places. v2 does just that and
> > leaves rewriteheap.c alone.
> >
> > With the hint set, the old version no longer waits in unresolved_tups,
> > so the logical mapping change from v1 is not needed anymore.
>
> I can see that you are testing both code paths, at least. The
> coverage part looks good, but I'd suggest to make the test more
> readable at least especially in the s1_alter and s1_plain that include
> a bunch of queries. There two are hard to parse.
>
> + if (!TransactionIdIsInProgress(xmax) &&
> + !TransactionIdDidCommit(xmax))
> + SetHintBits(tuple, buffer, HEAP_XMAX_INVALID,
> + InvalidTransactionId)
>
> It's also worth noting that these may make the code less efficient,
> with more SLRU traffic. What could be the class of worst case
> possible? A combination of an in-progress transaction with some
> VACUUM activity while the in-progress transaction is still opened?
> VACUUM would need to do more work with the patch in place, where I
> guess it could be slower if we have a bunch of tuples that need to
> have this hint bit set.
>
> Or perhaps this is too narrow to worry about? Anyway, we should be
> super careful with such changes. That can bite hard.
>
> If others have any ideas and/or comments, please feel free..

I haven't thought of a better idea yet, but I noticed a potential race condition
for MultiXacts in the current fix:

+ if (!(tuple->t_infomask & HEAP_XMAX_INVALID) &&
+ !HEAP_XMAX_IS_LOCKED_ONLY(tuple->t_infomask) &&
+ !HeapTupleHeaderIsOnlyLocked(tuple))
+ {
+ TransactionId xmax = HeapTupleHeaderGetUpdateXid(tuple);
+
+ if (!TransactionIdIsInProgress(xmax) &&
+ !TransactionIdDidCommit(xmax))
+ SetHintBits(tuple, buffer, HEAP_XMAX_INVALID,
+ InvalidTransactionId);

If a tuple's Xmax is a MultiXact and the updating subtransaction is still in
progress when HeapTupleHeaderIsOnlyLocked() is called, it returns false.
However, if that updating subtransaction aborts before the second
TransactionIdIsInProgress() check, the second check returns false and sets
HEAP_XMAX_INVALID on the tuple even though the MultiXact still has active
members, which I think violates the MultiXact hint-bit standard.

This can cause a problem if another transaction locked the tuple (e.g., FOR KEY
SHARE) beforehand. Once HEAP_XMAX_INVALID is set on the tuple, another backend
can lock the tuple FOR UPDATE even though the FOR KEY SHARE lock has not been
released.

I can reproduce this if a transaction updates a tuple, a second session locks
it FOR KEY SHARE, the first transaction updates it again in a subtransaction,
and a concurrent backend runs a SELECT that triggers on-access pruning
(heap_prune_satisfies_vacuum() -> HeapTupleSatisfiesVacuumHorizon()).

See the attached patch (which applies on top of the v2 patches and adds an
injection point) to reproduce this.

Best Regards,
Zhijie Hou