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

On Tue, Oct 20, 2026 at 11:32:44AM +0900, Takeshi Ideriha wrote:
> Regarding logical_rewrite_heap_tuple() part, you are completely right. I
> tested by myself yesterday and hit the same error while decoding the
> vacuum full scenario. Thank you for pointing out. I was not aware of DDL
> case with logical replication.

@@ -574,6 +576,14 @@ rewrite_heap_dead_tuple(RewriteState state, HeapTuple old_tuple)

if (unresolved != NULL)
{
+ /*
+ * 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;

Having a recheck of the old tuple's state within
rewrite_heap_dead_tuple() feels very incorrect to me, based on the
bunch of comments at the top of rewriteheap.c. It sounds to me that
rewrite_heap_dead_tuple() should be called when we know that the tuple
is dead, no? Isn't heapam_relation_copy_for_cluster() the bit actually
incorrect here, marking the tuple with "isdead" to start with? Your
solution of a recheck to make sure that an old tuple is actually dead
in a code path where an old tuple is assumed to be absolutely and
definitely dead looks like an API contract violation regarding
rewriteheap.c.
--
Michael