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

On Thu, Oct 1, 2026 at 4:51 PM Narayanan Venkateswaran
<narayananvpostgres@gmail.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.

On Wed, Sep 30, 2026 at 2:19 AM Narayanan Venkateswaran
<narayananvpostgres@gmail.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
placetoo ? 
>
> /*
>  * 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.

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

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

--
Thanks,
Nisha