Hi, On Fri, Nov 6, 2026 at 12:13 PM Zhijie Hou wrote:
Hi Andres,On Wed, Nov 4, 2026 at 09:33 PM Andres Freund wrote: > That surely is not the right fix, it breaks any sense of abstraction > across table AMs. There's afaict no guarantee that the target table > is heapam. And even if there currently is, we shouldn't bake that in > further. Thanks for pointing it out. I agree that conflict.c shouldn't be reaching into BufferHeapTupleTableSlot or HeapTupleHeaderGetXmin() directly. To avoid that, on master, it seems cleanest to handle this at the TupleTableSlotOps level (alongside getsysattr and is_current_xact_tuple in execTuples.c), for example, via an optional callback that returns the effective xmin (HeapTupleHeaderGetXmin() for heap/buffer-heap slots, and InvalidTransactionId when NULL for virtual/minimal slots or AMs that don't store tuple xmin). TransactionId (*get_effective_xmin) (TupleTableSlot *slot);
Following this, I am attaching two patches implementing the approaches discussed above for reference: v2-0001-Fix-logical-xx.patch adds an optional get_effective_xmin callback to TupleTableSlotOps (alongside getsysattr and is_current_xact_tuple). Heap and buffer-heap slots implement it via HeapTupleHeaderGetXmin(), while slots that do not provide this information leave the callback unset so slot_get_effective_xmin() returns InvalidTransactionId. nocfbot-v2-0001-Fix-logical-xx.patch avoids adding a new TupleTableSlotOps callback (which is also needed for back-patching to 18) by checking TTS_IS_BUFFERTUPLE(localslot) || TTS_IS_HEAPTUPLE(localslot) in GetTupleTransactionInfo() to use HeapTupleHeaderGetXmin(), and falling back to slot_getsysattr() for other slot types. What do you think ? -- Best Regards, Zhijie Hou
From 16c66de323e75c4240de8d240dc81a91fcf9f114 Mon Sep 17 00:00:00 2001 From: Zhijie Hou <[email protected]> Date: Mon, 9 Nov 2026 16:17:46 +0800 Subject: [PATCH v2] Fix logical replication conflict detection on frozen tuples. GetTupleTransactionInfo() obtained the local tuple's xmin through the xmin system attribute. For heap tuples, heap_getsysattr() returns the raw XID from the tuple header even after the tuple has been frozen. Such an XID cannot be used reliably for commit timestamp lookup: its entry may have been trimmed, or the XID may have wrapped around and been reassigned. This could cause spurious update_origin_differs or delete_origin_differs conflicts, and unique-conflict DETAIL messages could report bogus transaction ID, origin, and commit timestamp information. Fix this by using HeapTupleHeaderGetXmin() when the slot is a heap or buffer-heap tuple slot (which returns FrozenTransactionId for frozen tuples) and falling back to slot_getsysattr() for other slot types. GetTupleTransactionInfo() ignores non-normal XIDs and sets *xmin to InvalidTransactionId, and errdetail_apply_conflict() omits transaction details from unique-conflict messages when the local XID is invalid. Author: Zhijie Hou <[email protected]> Discussion: https://postgr.es/m/cafvd2n_s++xxftde208kbzudbdeh8oabcgvoi6udbdejmuf...@mail.gmail.com --- src/backend/replication/logical/conflict.c | 68 +++++++++++++++++----- src/test/subscription/t/035_conflicts.pl | 45 ++++++++++++++ 2 files changed, 100 insertions(+), 13 deletions(-) diff --git a/src/backend/replication/logical/conflict.c b/src/backend/replication/logical/conflict.c index 90431ffce98..7e1dea158de 100644 --- a/src/backend/replication/logical/conflict.c +++ b/src/backend/replication/logical/conflict.c @@ -16,6 +16,7 @@ #include "access/commit_ts.h" #include "access/genam.h" +#include "access/htup_details.h" #include "access/tableam.h" #include "catalog/heap.h" #include "catalog/pg_am.h" @@ -227,8 +228,11 @@ GetConflictLogDest(const char *dest) } /* - * Get the xmin and commit timestamp data (origin and timestamp) associated - * with the provided local row. + * Get the effective xmin and commit timestamp data (origin and timestamp) + * associated with the provided local row. + * + * If the tuple is frozen or does not have a normal XID, *xmin is set to + * InvalidTransactionId. * * Return true if the commit timestamp data was found, false otherwise. */ @@ -236,24 +240,53 @@ bool GetTupleTransactionInfo(TupleTableSlot *localslot, TransactionId *xmin, ReplOriginId *localorigin, TimestampTz *localts) { - Datum xminDatum; - bool isnull; + TransactionId tup_xmin; + + *xmin = InvalidTransactionId; + *localorigin = InvalidReplOriginId; + *localts = 0; + + /* + * For heap slots, heap_getsysattr() returns the raw xmin even when the + * tuple is frozen, so use HeapTupleHeaderGetXmin() to obtain the + * effective xmin. For other slot types, fall back to fetching the xmin + * system attribute. + */ + if (TTS_IS_BUFFERTUPLE(localslot) || TTS_IS_HEAPTUPLE(localslot)) + { + bool shouldFree; + HeapTuple tuple; - xminDatum = slot_getsysattr(localslot, MinTransactionIdAttributeNumber, - &isnull); - *xmin = DatumGetTransactionId(xminDatum); - Assert(!isnull); + tuple = ExecFetchSlotHeapTuple(localslot, false, &shouldFree); + Assert(!shouldFree); + tup_xmin = HeapTupleHeaderGetXmin(tuple->t_data); + } + else + { + Datum xminDatum; + bool isnull; + + xminDatum = slot_getsysattr(localslot, MinTransactionIdAttributeNumber, + &isnull); + tup_xmin = DatumGetTransactionId(xminDatum); + Assert(!isnull); + } + + /* + * Non-normal XIDs cannot be used to obtain meaningful transaction + * information. + */ + if (!TransactionIdIsNormal(tup_xmin)) + return false; + + *xmin = tup_xmin; /* * The commit timestamp data is not available if track_commit_timestamp is * disabled. */ if (!track_commit_timestamp) - { - *localorigin = InvalidReplOriginId; - *localts = 0; return false; - } return TransactionIdGetCommitTsData(*xmin, localts, localorigin); } @@ -499,7 +532,7 @@ errdetail_apply_conflict(EState *estate, ResultRelInfo *relinfo, localxmin, timestamptz_to_str(localts)); } } - else + else if (TransactionIdIsValid(localxmin)) { if (tuple_buf.len) appendStringInfo(&err_detail, _("Key already exists in unique index \"%s\", modified in transaction %u: %s."), @@ -509,6 +542,15 @@ errdetail_apply_conflict(EState *estate, ResultRelInfo *relinfo, appendStringInfo(&err_detail, _("Key already exists in unique index \"%s\", modified in transaction %u."), get_rel_name(indexoid), localxmin); } + else + { + if (tuple_buf.len) + appendStringInfo(&err_detail, _("Key already exists in unique index \"%s\": %s."), + get_rel_name(indexoid), tuple_buf.data); + else + appendStringInfo(&err_detail, _("Key already exists in unique index \"%s\"."), + get_rel_name(indexoid)); + } break; diff --git a/src/test/subscription/t/035_conflicts.pl b/src/test/subscription/t/035_conflicts.pl index 48ce95d9d13..34157935e2b 100644 --- a/src/test/subscription/t/035_conflicts.pl +++ b/src/test/subscription/t/035_conflicts.pl @@ -209,6 +209,51 @@ ok( $node_B->poll_query_until( ), "the xmin value of slot 'pg_conflict_detection' is valid on Node B"); +############################################################################### +# Check that frozen tuples do not use their raw xmin for conflict detection or +# reporting. Run this before enabling retain_dead_tuples on Node A so that +# VACUUM FREEZE is not held back by the conflict detection slot. +############################################################################### + +$node_A->safe_psql( + 'postgres', " + CREATE UNIQUE INDEX tab_b_key ON tab (b); + INSERT INTO tab VALUES (3, 1), (4, 2), (5, 3);"); +$node_A->wait_for_catchup($subname_BA); +$node_A->safe_psql('postgres', "VACUUM FREEZE tab;"); + +$log_offset = -s $node_A->logfile; + +$node_B->safe_psql( + 'postgres', " + UPDATE tab SET b = 10 WHERE a = 3; + DELETE FROM tab WHERE a = 4;"); +$node_B->wait_for_catchup($subname_AB); + +unlike( + slurp_file($node_A->logfile(), $log_offset), + qr/conflict detected on relation "public.tab": conflict=(update|delete)_origin_differs/, + 'frozen tuples do not trigger update_origin_differs or delete_origin_differs' +); + +$log_offset = -s $node_A->logfile; + +$node_B->safe_psql('postgres', "INSERT INTO tab VALUES (6, 3);"); + +$node_A->wait_for_log( + qr/conflict detected on relation "public.tab": conflict=insert_exists.* +.*Could not apply remote change: remote row \(6, 3\).* +.*Key already exists in unique index "tab_b_key": key \(b\)=\(3\), local row \(5, 3\)./, + $log_offset); + +pass('insert_exists on frozen tuple does not report transaction details'); + +# Clean up +$node_A->safe_psql('postgres', "DROP INDEX tab_b_key;"); +$node_B->wait_for_catchup($subname_AB); +$node_A->safe_psql('postgres', "TRUNCATE tab;"); +$node_A->wait_for_catchup($subname_BA); + ################################################## # Check that the retain_dead_tuples option can be enabled only for disabled # subscriptions. Validate the NOTICE message during the subscription DDL, and -- 2.54.0 (Apple Git-157)
From 21f8e7223fbbe8bb8ae9bbfa06a31a4d76689af4 Mon Sep 17 00:00:00 2001 From: Zhijie Hou <[email protected]> Date: Fri, 6 Nov 2026 11:08:50 +0800 Subject: [PATCH v2] Fix logical replication conflict detection on frozen tuples. GetTupleTransactionInfo() obtained the local tuple's xmin through the xmin system attribute. For heap tuples, heap_getsysattr() returns the raw XID from the tuple header even after the tuple has been frozen. Such an XID cannot be used reliably for commit timestamp lookup: its entry may have been trimmed, or the XID may have wrapped around and been reassigned. This could cause spurious update_origin_differs or delete_origin_differs conflicts, and unique-conflict DETAIL messages could report bogus transaction ID, origin, and commit timestamp information. Fix this by adding an optional get_effective_xmin callback to TupleTableSlotOps that returns the effective inserting XID of the tuple. Heap and buffer-heap slots implement it using HeapTupleHeaderGetXmin(), which returns FrozenTransactionId for frozen tuples, while slots that do not provide this information leave the callback unset so slot_get_effective_xmin() returns InvalidTransactionId. GetTupleTransactionInfo() ignores non-normal XIDs and sets *xmin to InvalidTransactionId, and errdetail_apply_conflict() omits transaction details from unique-conflict messages when the local XID is invalid. Author: Zhijie Hou <[email protected]> Discussion: https://postgr.es/m/cafvd2n_s++xxftde208kbzudbdeh8oabcgvoi6udbdejmuf...@mail.gmail.com --- src/backend/executor/execTuples.c | 42 ++++++++++++++++++++ src/backend/replication/logical/conflict.c | 43 ++++++++++++++------- src/include/executor/tuptable.h | 31 +++++++++++++++ src/test/subscription/t/035_conflicts.pl | 45 ++++++++++++++++++++++ 4 files changed, 148 insertions(+), 13 deletions(-) diff --git a/src/backend/executor/execTuples.c b/src/backend/executor/execTuples.c index d83322cc510..b633bc7d64f 100644 --- a/src/backend/executor/execTuples.c +++ b/src/backend/executor/execTuples.c @@ -372,6 +372,25 @@ tts_heap_getsysattr(TupleTableSlot *slot, int attnum, bool *isnull) slot->tts_tupleDescriptor, isnull); } +static TransactionId +tts_heap_get_effective_xmin(TupleTableSlot *slot) +{ + HeapTupleTableSlot *hslot = (HeapTupleTableSlot *) slot; + + Assert(!TTS_EMPTY(slot)); + + /* + * In some code paths it's possible to get here with a non-materialized + * slot, in which case we can't retrieve the tuple's xmin. + */ + if (!hslot->tuple) + ereport(ERROR, + (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), + errmsg("don't have a storage tuple in this context"))); + + return HeapTupleHeaderGetXmin(hslot->tuple->t_data); +} + static bool tts_heap_is_current_xact_tuple(TupleTableSlot *slot) { @@ -777,6 +796,25 @@ tts_buffer_heap_getsysattr(TupleTableSlot *slot, int attnum, bool *isnull) slot->tts_tupleDescriptor, isnull); } +static TransactionId +tts_buffer_heap_get_effective_xmin(TupleTableSlot *slot) +{ + BufferHeapTupleTableSlot *bslot = (BufferHeapTupleTableSlot *) slot; + + Assert(!TTS_EMPTY(slot)); + + /* + * In some code paths it's possible to get here with a non-materialized + * slot, in which case we can't retrieve the tuple's xmin. + */ + if (!bslot->base.tuple) + ereport(ERROR, + (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), + errmsg("don't have a storage tuple in this context"))); + + return HeapTupleHeaderGetXmin(bslot->base.tuple->t_data); +} + static bool tts_buffer_is_current_xact_tuple(TupleTableSlot *slot) { @@ -1280,6 +1318,7 @@ const TupleTableSlotOps TTSOpsVirtual = { .clear = tts_virtual_clear, .getsomeattrs = tts_virtual_getsomeattrs, .getsysattr = tts_virtual_getsysattr, + .get_effective_xmin = NULL, .materialize = tts_virtual_materialize, .is_current_xact_tuple = tts_virtual_is_current_xact_tuple, .copyslot = tts_virtual_copyslot, @@ -1301,6 +1340,7 @@ const TupleTableSlotOps TTSOpsHeapTuple = { .clear = tts_heap_clear, .getsomeattrs = tts_heap_getsomeattrs, .getsysattr = tts_heap_getsysattr, + .get_effective_xmin = tts_heap_get_effective_xmin, .is_current_xact_tuple = tts_heap_is_current_xact_tuple, .materialize = tts_heap_materialize, .copyslot = tts_heap_copyslot, @@ -1319,6 +1359,7 @@ const TupleTableSlotOps TTSOpsMinimalTuple = { .clear = tts_minimal_clear, .getsomeattrs = tts_minimal_getsomeattrs, .getsysattr = tts_minimal_getsysattr, + .get_effective_xmin = NULL, .is_current_xact_tuple = tts_minimal_is_current_xact_tuple, .materialize = tts_minimal_materialize, .copyslot = tts_minimal_copyslot, @@ -1337,6 +1378,7 @@ const TupleTableSlotOps TTSOpsBufferHeapTuple = { .clear = tts_buffer_heap_clear, .getsomeattrs = tts_buffer_heap_getsomeattrs, .getsysattr = tts_buffer_heap_getsysattr, + .get_effective_xmin = tts_buffer_heap_get_effective_xmin, .is_current_xact_tuple = tts_buffer_is_current_xact_tuple, .materialize = tts_buffer_heap_materialize, .copyslot = tts_buffer_heap_copyslot, diff --git a/src/backend/replication/logical/conflict.c b/src/backend/replication/logical/conflict.c index 90431ffce98..2da6cb10602 100644 --- a/src/backend/replication/logical/conflict.c +++ b/src/backend/replication/logical/conflict.c @@ -227,8 +227,11 @@ GetConflictLogDest(const char *dest) } /* - * Get the xmin and commit timestamp data (origin and timestamp) associated - * with the provided local row. + * Get the effective xmin and commit timestamp data (origin and timestamp) + * associated with the provided local row. + * + * If the tuple is frozen or the slot does not provide a normal XID, *xmin is + * set to InvalidTransactionId. * * Return true if the commit timestamp data was found, false otherwise. */ @@ -236,24 +239,29 @@ bool GetTupleTransactionInfo(TupleTableSlot *localslot, TransactionId *xmin, ReplOriginId *localorigin, TimestampTz *localts) { - Datum xminDatum; - bool isnull; + TransactionId tup_xmin; + + *xmin = InvalidTransactionId; + *localorigin = InvalidReplOriginId; + *localts = 0; + + tup_xmin = slot_get_effective_xmin(localslot); + + /* + * Non-normal XIDs cannot be used to obtain meaningful transaction + * information. + */ + if (!TransactionIdIsNormal(tup_xmin)) + return false; - xminDatum = slot_getsysattr(localslot, MinTransactionIdAttributeNumber, - &isnull); - *xmin = DatumGetTransactionId(xminDatum); - Assert(!isnull); + *xmin = tup_xmin; /* * The commit timestamp data is not available if track_commit_timestamp is * disabled. */ if (!track_commit_timestamp) - { - *localorigin = InvalidReplOriginId; - *localts = 0; return false; - } return TransactionIdGetCommitTsData(*xmin, localts, localorigin); } @@ -499,7 +507,7 @@ errdetail_apply_conflict(EState *estate, ResultRelInfo *relinfo, localxmin, timestamptz_to_str(localts)); } } - else + else if (TransactionIdIsValid(localxmin)) { if (tuple_buf.len) appendStringInfo(&err_detail, _("Key already exists in unique index \"%s\", modified in transaction %u: %s."), @@ -509,6 +517,15 @@ errdetail_apply_conflict(EState *estate, ResultRelInfo *relinfo, appendStringInfo(&err_detail, _("Key already exists in unique index \"%s\", modified in transaction %u."), get_rel_name(indexoid), localxmin); } + else + { + if (tuple_buf.len) + appendStringInfo(&err_detail, _("Key already exists in unique index \"%s\": %s."), + get_rel_name(indexoid), tuple_buf.data); + else + appendStringInfo(&err_detail, _("Key already exists in unique index \"%s\"."), + get_rel_name(indexoid)); + } break; diff --git a/src/include/executor/tuptable.h b/src/include/executor/tuptable.h index 3db6c9c9bd0..b98b9a44786 100644 --- a/src/include/executor/tuptable.h +++ b/src/include/executor/tuptable.h @@ -16,6 +16,7 @@ #include "access/htup.h" #include "access/sysattr.h" +#include "access/transam.h" #include "access/tupdesc.h" #include "storage/buf.h" @@ -181,6 +182,16 @@ struct TupleTableSlotOps */ Datum (*getsysattr) (TupleTableSlot *slot, int attnum, bool *isnull); + /* + * Return the effective transaction ID that inserted the tuple. Unlike + * getsysattr(MinTransactionIdAttributeNumber), this accounts for freezing + * and other cases where the raw XID no longer provides meaningful + * transaction information. Throws an error if the slot doesn't contain + * the storage tuple. If the slot type cannot provide this information, + * it should not implement this callback and should set it as NULL. + */ + TransactionId (*get_effective_xmin) (TupleTableSlot *slot); + /* * Check if the tuple is created by the current transaction. Throws an * error if the slot doesn't contain the storage tuple. @@ -454,6 +465,26 @@ slot_getsysattr(TupleTableSlot *slot, int attnum, bool *isnull) return slot->tts_ops->getsysattr(slot, attnum, isnull); } +/* + * slot_get_effective_xmin - fetch the effective XID that inserted the slot's + * current tuple. + * + * If the slot type does not provide an effective xmin, this returns + * InvalidTransactionId. If the slot type supports storage tuples, the + * caller must ensure that there is currently one inside the slot, otherwise + * an error will be thrown. + */ +static inline TransactionId +slot_get_effective_xmin(TupleTableSlot *slot) +{ + Assert(!TTS_EMPTY(slot)); + + if (slot->tts_ops->get_effective_xmin == NULL) + return InvalidTransactionId; + + return slot->tts_ops->get_effective_xmin(slot); +} + /* * slot_is_current_xact_tuple - check if the slot's current tuple is created * by the current transaction. diff --git a/src/test/subscription/t/035_conflicts.pl b/src/test/subscription/t/035_conflicts.pl index 48ce95d9d13..34157935e2b 100644 --- a/src/test/subscription/t/035_conflicts.pl +++ b/src/test/subscription/t/035_conflicts.pl @@ -209,6 +209,51 @@ ok( $node_B->poll_query_until( ), "the xmin value of slot 'pg_conflict_detection' is valid on Node B"); +############################################################################### +# Check that frozen tuples do not use their raw xmin for conflict detection or +# reporting. Run this before enabling retain_dead_tuples on Node A so that +# VACUUM FREEZE is not held back by the conflict detection slot. +############################################################################### + +$node_A->safe_psql( + 'postgres', " + CREATE UNIQUE INDEX tab_b_key ON tab (b); + INSERT INTO tab VALUES (3, 1), (4, 2), (5, 3);"); +$node_A->wait_for_catchup($subname_BA); +$node_A->safe_psql('postgres', "VACUUM FREEZE tab;"); + +$log_offset = -s $node_A->logfile; + +$node_B->safe_psql( + 'postgres', " + UPDATE tab SET b = 10 WHERE a = 3; + DELETE FROM tab WHERE a = 4;"); +$node_B->wait_for_catchup($subname_AB); + +unlike( + slurp_file($node_A->logfile(), $log_offset), + qr/conflict detected on relation "public.tab": conflict=(update|delete)_origin_differs/, + 'frozen tuples do not trigger update_origin_differs or delete_origin_differs' +); + +$log_offset = -s $node_A->logfile; + +$node_B->safe_psql('postgres', "INSERT INTO tab VALUES (6, 3);"); + +$node_A->wait_for_log( + qr/conflict detected on relation "public.tab": conflict=insert_exists.* +.*Could not apply remote change: remote row \(6, 3\).* +.*Key already exists in unique index "tab_b_key": key \(b\)=\(3\), local row \(5, 3\)./, + $log_offset); + +pass('insert_exists on frozen tuple does not report transaction details'); + +# Clean up +$node_A->safe_psql('postgres', "DROP INDEX tab_b_key;"); +$node_B->wait_for_catchup($subname_AB); +$node_A->safe_psql('postgres', "TRUNCATE tab;"); +$node_A->wait_for_catchup($subname_BA); + ################################################## # Check that the retain_dead_tuples option can be enabled only for disabled # subscriptions. Validate the NOTICE message during the subscription DDL, and -- 2.54.0 (Apple Git-157)
