Re: Proposal: Conflict log history table for Logical Replication

On Thu, Oct 1, 2026 at 10:55 AM vignesh C <vignesh21(at)gmail(dot)com> wrote:
>
> On Thu, 1 Oct 2026 at 10:26, shveta malik <shveta(dot)malik(at)gmail(dot)com> wrote:
> >
> > On Thu, Oct 1, 2026 at 10:17 AM vignesh C <vignesh21(at)gmail(dot)com> wrote:
> > >
> > > On Wed, 30 Sept 2026 at 18:53, Nisha Moond <nisha(dot)moond412(at)gmail(dot)com> wrote:
> > > >
> > > > On Wed, Sep 30, 2026 at 12:28 PM Narayanan Venkateswaran
> > > > <narayananvpostgres(at)gmail(dot)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(dot)moond412(at)gmail(dot)com> wrote:
> > > > >>
> > > > >> On Tue, Sep 29, 2026 at 5:56 PM Narayanan Venkateswaran
> > > > >> <narayananvpostgres(at)gmail(dot)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 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.

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.

[1] https://www.postgresql.org/message-id/CABdArM4H_ZVc3%3DpDe4PB-qk4XbWA%3DDLC0SF2_vunz1%2BodGPLPg%40mail.gmail.com

--
Thanks,
Nisha