Re: Proposal: Conflict log history table for Logical Replication - Mailing list pgsql-hackers

Hi Nisha,

Thank you once again for the guidance.

Please find replies inline, please note I have switched to plain text mode too.

On Wed, Sep 30, 2026 at 6:53 PM Nisha Moond <nisha.moond412@gmail.com> wrote:
>
> On Wed, Sep 30, 2026 at 12:28 PM Narayanan Venkateswaran
> <narayananvpostgres@gmail.com> wrote:
> >
> > Hi Nisha,
> >
> > Thank you very much for the guidance and the pointers to the older thread,
> >
> > Please find some replies inline,
> >
> > On Wed, Sep 30, 2026 at 11:05 AM Nisha Moond <nisha.moond412@gmail.com> wrote:
> >>
> >> On Tue, Sep 29, 2026 at 5:56 PM Narayanan Venkateswaran
> >> <narayananvpostgres@gmail.com> wrote:
> >> >
> >> > Thank you very much for the excellent work. I looked at the patch v77,
> >> >
> >>
> >> Hi Narayanan, thanks for reviewing it.
> >>
> >> > The code decides replica_identity_full using the following logic (in conflict.c),
> >> >
> >> > if (!TupIsNull(searchslot))
> >> > {
> >> > Oid replica_index = GetRelationIdentityOrPK(rel);
> >> >
> >> > /*
> >> > * If the table has a valid replica identity index, build the index
> >> > * JSON datum from key value. Otherwise, in REPLICA IDENTITY FULL
> >> > * cases, set replica_identity_full to true and leave replica_identity
> >> > * NULL to avoid serializing full tuples that could exceed memory
> >> > * allocation limits.
> >> > */
> >> > if (OidIsValid(replica_index))
> >> > {
> >> > values[attno++] = BoolGetDatum(false);
> >> > values[attno++] = build_index_key_json(rel,
> >> >   replica_index,
> >> >   searchslot,
> >> >   &omitted);
> >> > }
> >> > else
> >> > {
> >> > values[attno++] = BoolGetDatum(true);
> >> > nulls[attno++] = true;
> >> > }
> >> > }
> >> > else
> >> > {
> >> > nulls[attno++] = true;
> >> > nulls[attno++] = true;
> >> > }
> >> >
> >> > In PostgreSQL catalogs (pg_class.relreplident), a table's replica can be one of four values:
> >> >
> >> > 'd' = REPLICA_IDENTITY_DEFAULT: Use PK index if one exists. If the table has no PK, it has no index and is NOT
FULL.
> >> > 'n' = REPLICA_IDENTITY_NOTHING: No replica identity.
> >> > 'i' = REPLICA_IDENTITY_INDEX: Explicit unique index.
> >> > 'f' = REPLICA_IDENTITY_FULL: The entire tuple is the identity.
> >> >
> >> > If a subscriber relation has REPLICA IDENTITY DEFAULT without a primary key (or REPLICA IDENTITY NOTHING)
GetRelationIdentityOrPK()returns InvalidOid. In this case, the code sets replica_identity_full = true. 
> >> >
> >>
> >> I think there may be some misunderstanding about what the
> >> replica_identity_full column actually stores. This question was also
> >> raised earlier; please see [1] and the discussion that followed. This
> >> field indicates the subscriber’s actual search method.
> >
> >
> > Thank you for clarifying the intended design and also the pointer to the old thread.
> >
> > I understand now that the intention for `replica_identity_full` is to indicate whether the conflicting row was
locatedvia a specific replica key index (`false`) versus a full-tuple search (`true`), rather than reflecting the DDL
catalogproperty (pg_class.relreplident). 
> >
>
> I think the docs can be improved to avoid this confusion, as Vignesh
> also suggested earlier in [1]. How about updating it to:
> "Indicates whether the conflicting local row was located using the
> full tuple (true) or the replica identity key of the local table
> (false)."
>
> Let me know if this works for you.

This looks good, Thank you very much.

>
> >>
> >>
> >> > However, the SGML docs update in the patch states that replica_identity_full "is NULL when replica identity
informationis not applicable". 
> >> >
> >> > The only way replica_identity_full can ever be set to NULL is if the execution enters the outer else block: when
TupIsNull(searchslot)is true (i.e., searchslot is NULL or empty). However, it looks like this slot contains the
incomingrow data sent by the publisher. It is always populated and never null. 
> >> >
> >> > Because searchslot is never null, the outer else block is never executed. The code will never set
replica_identity_fullto NULL. 
> >> >
> >> > I think it is better to explicitly check for REPLICA_IDENTITY_FULL in an else if block, something like the
below,
> >> >
> >> > else if (rel->rd_rel->relreplident == REPLICA_IDENTITY_FULL)
> >> > {
> >> > values[attno++] = BoolGetDatum(true);
> >> > nulls[attno++] = true;
> >> > }
> >> >
> >>
> >> As per [1], replica_identity_full value is determined independently of
> >> relreplident, so I don't think we need this else-if branch.
> >
> >
> >
> > However, I have the following question related to the following doc entry,
> >
> > +       <row>
> > +        <entry><literal>replica_identity_full</literal></entry>
> > +        <entry><type>boolean</type></entry>
> > +        <entry>Indicates whether the conflicting relation uses <literal>REPLICA IDENTITY FULL</literal>
(<literal>true</literal>)or a replica identity index (<literal>false</literal>). This is <literal>NULL</literal> when
replicaidentity information is not applicable.</entry> 
> > +       </row>
> >
> > The doc states it is "NULL when replica identity information is not applicable". However, in
insert_conflict_log_tuple(),replica_identity_full is only set to NULL if TupIsNull(searchslot) is true. Since
searchslot(remoteslot) is always populated for all currently logged conflicts, the outer else block is never reached
andreplica_identity_full is never NULL. Should the documentation be updated to remove the reference to NULL, or is
therea case where searchslot can be empty ? 
> >
> >
>
> I agree. Since we decided not to record ERROR conflicts like
> insert_exist, the search slot can never be NULL, and hence
> replica_identity_full can never be NULL either. This also makes the
> below else branch in insert_conflict_log_tuple() dead code.
>
> + }
> + else
> + {
> + nulls[attno++] = true;
> + nulls[attno++] = true;
> + }
>
> We should remove this dead code and update the docs to remove: "This
> is NULL when replica identity information is not applicable."
>

+1, Thank you.

> >>
> >>
> >> [I would request you to please reply inline to keep the discussion
> >> relevant and easier to follow.]
> >
> >
> > Really sorry, my humble apologies for the inconvenience.
> >
>
> No worries at all, and thank you for understanding. You can also try
> using plain-text format.

Done !

>
> [1] https://www.postgresql.org/message-id/CALDaNm2s1jtqukoMzNr94MNALvwMTRY53axEtPXf1YVfmH3_bQ%40mail.gmail.com
> --
> Thanks,
> Nisha

Thank you,
Narayanan