Re: Proposal: Conflict log history table for Logical Replication

On Tue, Oct 13, 2026 at 3:34 PM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
>
> I merged v80-0001 and v80-0002 and adjusted the commit message and
> test accordingly; I also included Amit's and Hou-san's code
> refactoring. Rebased Nisha's V80_2-0003 on top of that as 0002, also
> merged Shveta's datestyle fix with some modification into the 0002
>

Thanks for the updated patches. A few comments on v81-0001:

1) Does the remote_origin column in CLT add any value?
+ if (replorigin_xact_state.origin != InvalidReplOriginId)
+ replorigin_by_oid(replorigin_xact_state.origin, true, &remote_origin);

IIUC, replorigin_xact_state.origin is the subscription's own origin,
set at worker start, so every row has pg_<subid>. A CLT only holds
conflicts of its own subscription, and its name already contains the
subid. Once the subscription is dropped, the CLT goes with it, and
even a saved copy can't be traced back to a publisher node. Should we
drop this column? Thoughts?

2) conflict.c: insert_conflict_log_tuple():
+ heap_insert(conflictlogrel, tuple,
+ GetCurrentCommandId(true), HEAP_INSERT_NO_LOGICAL, NULL);
Should we add a comment explaining why HEAP_INSERT_NO_LOGICAL is still
needed even though a CLT can't be added to a publication?

3) v78-0004 removed "or when replica identity is not applicable" from
the replica_identity description, since searchslot can never be NULL.
I think v80 added it back:
+ when replica identity is not applicable.</entry>
The code also has Assert(!TupIsNull(searchslot)), so replica_identity
is NULL only when replica_identity_full is true. Should remove this
part again.

4) The conflict below is from a 2PC txn hitting delete_missing while
the worker processes PREPARE TRANSACTION:

relname | conflict_type | remote_xid | remote_commit_lsn | remote_commit_ts
---------+----------------+------------+-------------------+------------------
t2pc | delete_missing | 669 | 0/017D0958 |

Using the prepare LSN as remote_commit_lsn for 2PC seems intentional.
But remote_commit_ts stays NULL, because the commit time isn't known
during PREPARE TRANSACTION. Could we use the prepare time here, as we
do with the prepare lsn? It may not help with resolution, though. If
not, we should document the NULL for 2PC too; currently the docs
mention it only for the parallel apply worker.

5) The text recorded for a key depends on the apply worker's output
settings. With bytea_output = escape or quote_all_identifiers = on set
on the subscriber, the same key is stored differently from the
default. For example, the two settings were toggled between two
conflicts on the same keys (tb has a bytea key, tr a regclass[] key):

postgres=# SELECT relname, conflict_type, replica_identity::text FROM
pg_conflict.pg_conflict_log_16398;
relname | conflict_type | replica_identity
---------+----------------+--------------------------------
-- bytea_output
tb | delete_missing | {"k":"\\x80ff"} -- hex case
tb | delete_missing | {"k":"\\200\\377"} -- escape case

--quote_all_identifiers
tr | delete_missing | {"k":"{pg_class}"} -- off case
tr | delete_missing | {"k":"{\"\\\"pg_class\\\"\"}"} -- on case

The values themselves are correct: casting back to the column type
gives the same key, e.g. (replica_identity->>'k')::bytea or
::regclass[]. But anyone searching or grouping conflicts by
replica_identity->>'k' as text gets split or missing results. The
current docs say the value is "the same text a SELECT of that column
would show", which only holds when the reader's settings match the
apply worker's. It would help to document this.
The attached diff adds a paragraph for this. Please consider if it
looks okay to you, and feel free to modify it as you see fit.

--
Thanks,
Nisha