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

Hi Shihao, Ayush

On 2026/10/19 12:31, shihao zhong wrote:
> Hi,
>
> Here is a patch. rewrite_heap_dead_tuple() drops the held tuple when its
> successor is dead, but that is only right if the successor's inserter
> committed. If it aborted, the update never happened, so 0001 keeps the
> tuple and adds its logical rewrite mapping.

> Setting the hint in HeapTupleSatisfiesVacuum() is not enough. VACUUM
> FULL on a catalog can see a transaction that is still open, and this
> loses the pg_class row of a committed table:
I was actually also writing a code to fix the issue and have just
noticed the patch was already submitted. Attached a patch for reference.
I haven't considered into logical_rewrite_heap_tuple(). I need to check
this... Thank you.

> {
>+ /*
>+ * If the inserter of this tuple aborted, the update >never happened
>+ * and the earlier tuple is not dead. It will be >written at the end.
>+ */
>+ if (!HeapTupleHeaderXminCommitted(old_tuple->t_data) &&
>+ !>TransactionIdDidCommit(HeapTupleHeaderGetXmin(old_tuple->t_data)))
>+ return false;

I am thinking like the below since HeapTupleSatisfiesVacuum set the xmin
invalid hint bit for the dead tuple. So I believe we can just rely on
HeapTupleHeaderXminInvalid hint bit.

+ if (HeapTupleHeaderXminInvalid(old_tuple->t_data))
+ return false;

What do you think above?

>0002 is an isolation test for that case, it is optional. But I want it
>in tree to prevent
>future regression in that area.

+1. I also think this is optional but I believe the issue is easy to
oversight, so adding a new test would be helpful.

Regards,
Takeshi Ideriha