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

Hi,

On Thu, Oct 22, 2026 at 10:22 AM Michael Paquier <michael(at)paquier(dot)xyz> wrote:
>
> On Wed, Oct 21, 2026 at 06:01:28PM +0800, Zhijie Hou wrote:
> > + 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.
>
> Perhaps also check for HEAP_XMAX_IS_MULTI to skip the multixacts and
> make sure that active members are not lost, then? I am not completely
> sure if this would be always right, though, these code paths are the
> hard-level definition of tricky.

Right, I think we can skip setting flag for HEAP_XMAX_IS_MULTI tuples,
since they're
handled correctly later in rewrite_heap_tuple() by HeapTupleHeaderIsOnlyLocked()
and won't be added to unresolved_tups. (I see Takeshi San also mentioned that).

>
> What I am really wondering is why do we actually need to fix *two*
> problems at once? The first SetHintBits() looks like a no-brainer to
> me, and links to the second permutation in the test "s1_plain
> s1_count" where we do a CLUSTER after an UPDATE rollbacked to a
> previous savepoint.

+1 to fix that part first. That part of the fix (patch) looks clean to me. It
also covers more common cases, which can happen for both user tables and
catalogs, while the other part (the complicated, performance-sensitive one) only
covers catalog cases IIUC, as user table DML holds locks until transaction end,
so concurrent CLUSTER can't touch it in that case.

> Then with 500k rows, here are some VACUUM average runtimes in ms from
> one of my laptops:
> HEAD Shihao's v2
> subxact_updates 1408 2123
> delete 686 1369
> insert_only 689 693
>
> The subxact_updates is an edge case, fine, still it regresses badly.
> The DELETE case is a much more common case. It regresses even more
> badly, and that does not look acceptable to me. That should be
> optimizable, of course, I don't really know yet how to do that. I
> know however that regressions like these ones are never good.

One idea to improve is to do these expensive checks only when xmin is not equal
to xmax. If xmin is still in progress, xmax cannot be aborted, so it's safe to
return early in that case. The DELETE test wouldn't regress after this change,
since xmin and xmax differ there. The subxact_update test would still regress,
but that's likely a rare case.

>
> So the first step to me would be a "Divide and Conquer" strategy,
> where we should look at each problem independently instead of trying
> to do too much at once.

Agreed.

Best Regards,
Zhijie Hou