On Fri, Oct 16, 2026 at 8:11 PM Nisha Moond <[email protected]> wrote:
>
> On Thu, Oct 15, 2026 at 9:47 AM shveta malik <[email protected]> wrote:
> >
> > On Wed, Oct 14, 2026 at 8:16 PM Nisha Moond <[email protected]>
> > wrote:
> > >
> > >
> > > 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>.
> >
> > It could also be pg_<subid>_<relid>, for a tablesync worker. But
> > again, it is not very useful as both subid and relid can be figured
> > out from CLT table itself. So I too currently do not see any use of
> > this column, unless I am missing some case?
> >
>
> Not changed in v82. Removing the column touches several places: the
> CLT schema, the docs, the TAP tests and the regression output. Let's
> wait to hear what others think before making the change.
+1.
>
> > > 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?
> > >
>
> Added a comment explaining the reason.
>
> > > 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.
> > >
>
> Removed it from the replica_identity description.
>
> > > 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.
> > >
>
> Updated the docs to say it's NULL when the conflict occurs in a
> transaction prepared on the subscriber. Let me know if others think we
> should store the prepare time instead.
I think updating doc is good and sufficient. IMO, we should not update
remote_commit_ts with prepare_ts: a) it will be misleading, b) it
does not solve any purpose.
> > > 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.
> >
> > Doc looks okay to me.
> >
>
> Thanks for the review; I added it in v82-0003.
>
> --
> Thanks,
> Nisha
