Hi Nisha, On Thu, Oct 1, 2026 at 3:46 PM Nisha Moond <[email protected]> wrote: > > On Thu, Oct 1, 2026 at 10:55 AM vignesh C <[email protected]> wrote: > > > > On Thu, 1 Oct 2026 at 10:26, shveta malik <[email protected]> wrote: > > > > > > On Thu, Oct 1, 2026 at 10:17 AM vignesh C <[email protected]> wrote: > > > > > > > > On Wed, 30 Sept 2026 at 18:53, 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. > > > > > > > > The (true) and (false) in this wording are a little unclear, as it is > > > > not obvious what they refer to. I'm not sure they are necessary here. > > > > If we do want to mention the Boolean values, it may be clearer to > > > > explicitly refer to the field, for example, replica_identity_full > > > > (true/false). > > > > > > Shall it be: > > > > > > "True if the conflicting local row was located using the full tuple, > > > rather than the replica identity key of the local table." > > > > Thanks, this looks good to me. > > > > Done. > > I’ve also removed the dead code as discussed in [1]. For the same > reason, also removed the part: “or when replica identity is not > applicable" from the replica_identity description.
* Thank you very much for the work. > > I’ve addressed these changes in the top-up 004 patch. Dilip, please > consider if it looks good to you. > Patches 001 to 003 are unchanged. * I had two more comments on v77. I sent the email with the additional comments as a follow-up to my first email. Can you please confirm you are able to see those comments too ? > > [1] > https://www.postgresql.org/message-id/CABdArM4H_ZVc3%3DpDe4PB-qk4XbWA%3DDLC0SF2_vunz1%2BodGPLPg%40mail.gmail.com > > -- > Thanks, > Nisha Thank you, Narayanan
