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

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.

>>
>>
>> > 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."

>>
>>
>> [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.

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