On Thu, Oct 1, 2026 at 4:51 PM Narayanan Venkateswaran <[email protected]> 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 <[email protected]> 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. > 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
