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 <[email protected]> wrote: > > On Wed, Sep 30, 2026 at 12:28 PM Narayanan Venkateswaran > <[email protected]> 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 <[email protected]> > > wrote: > >> > >> On Tue, Sep 29, 2026 at 5:56 PM Narayanan Venkateswaran > >> <[email protected]> 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 located via a specific replica key > > index (`false`) versus a full-tuple search (`true`), rather than reflecting > > the DDL catalog property (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 information is 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 incoming row 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_full to 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 replica identity 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 and replica_identity_full is never NULL. > > Should the documentation be updated to remove the reference to NULL, or is > > there a 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
