Re: Proposal: Conflict log history table for Logical Replication

Hi Nisha,

On Mon, Oct 5, 2026 at 10:56 AM Nisha Moond <nisha(dot)moond412(at)gmail(dot)com> wrote:
>
> On Thu, Oct 1, 2026 at 4:51 PM Narayanan Venkateswaran
> <narayananvpostgres(at)gmail(dot)com> wrote:
> >
> > Hi Nisha,
> >
> > * 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, Narayana, for pointing this out. I missed responding to that
> email earlier. Please find my responses inline to both comments below.

Thank you very much for taking time to acknowledge my email and replying.

>
> On Wed, Sep 30, 2026 at 2:19 AM Narayanan Venkateswaran
> <narayananvpostgres(at)gmail(dot)com> wrote:
> >
> > Thank you once again for the great work, please find some more comments on v77 series of patches below,
> >
> > 1. build_local_conflicts_json_array() still uses row_to_json(). Can we apply the same changes from v77-0002 in this place too ?
> >
> > /*
> > * Builds the local conflicts JSON array column from the list of
> > * ConflictTupleInfo objects.
> > *
> > * Example output structure:
> > * [ { "xid": "1001", "commit_ts": "...", "origin": "..." }, ... ]
> > */
> > static Datum
> > build_local_conflicts_json_array(List *conflicttuples)
> > {
> > .
> > .
> > .
> > .
> > /*
> > * Build the higher level JSON datum in format described in function
> > * header.
> > */
> > json_datum = DirectFunctionCall1(row_to_json, datum);
> > .
> > .
> > .
> > }
> >
>
> The concern in v77-0002 was a user-defined cast to json on a
> user-defined type, which row_to_json() would call. That can't happen
> in local_conflicts - its fields have fixed built-in types (xid,
> timestamptz, text), and each is small and bounded (the origin name is
> at most 512 bytes). So row_to_json() is safe here.

ok, I don't have a strong opinion on this one.

>
> > 2. Minor nit:
> >
> > The commit message in v77-0001 states:
> >
> > The JSON array uses the following structured format:
> > [ { "xid": "1001", "commit_ts": "2025-12-25 10:00:00+05:30", "origin": "node_1",
> > "tuple": {"id": 1, "val": "old_data"} }, ... ]
> >
> > In the code below however, there are only 3 fields: xid, commit_ts, and origin. There is no "tuple" attribute,
> >
> > /*
> > * Schema for the elements within the 'local_conflicts' JSON array.
> > */
> > static const ConflictLogColumnDef LocalConflictSchema[] =
> > {
> > {.attname = "xid", .atttypid = XIDOID},
> > {.attname = "commit_ts", .atttypid = TIMESTAMPTZOID},
> > {.attname = "origin", .atttypid = TEXTOID}
> > };
> >
> > Can we please consider changing the commit message in v77-0001 ?
> >
>
> Agree. 001 needs a commit message update since we opted for the
> option-2 design [1], and I missed updating it when posting v75. This
> can be addressed in the next version, rather than posting a new
> version just for this.

Sure, Thank you.

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

Thank you,
Narayanan