Re: Simplify code building the LR conflict messages

On Mon, Jan 19, 2026 at 4:25 PM Hayato Kuroda (Fujitsu)
<kuroda(dot)hayato(at)fujitsu(dot)com> wrote:
>
> Dear Amit,
>
> > Few comments:
> > =============
> > 1.
> > + append_tuple_value_detail(&err_detail, NULL, NULL,
> > + remote_desc, search_desc);
> > +
> > + appendStringInfoString(&err_detail, _(".\n"));
> >
> > Adding full-stop along with a newline like this appears odd. I think
> > we did it like this so that the internal function
> > append_tuple_value_detail() doesn't decide whether the line ends. Is
> > there any better way or are we okay with this kind of coding? I see
> > that previously build_tuple_value_details() also appends dot when
> > required. See below code:
> > build_tuple_value_details()
> > {
> > ...
> > if (tuple_value.len == 0)
> > return NULL;
>
> After considering bit more, I found "." can be appended all the cases and
> \newline can be added for some cases. Added a parameter to control \newline,
> and removed some termination handlings from the errdetail... function.
>
> > 2.
> > + if (key_desc)
> > + pfree(key_desc);
> > + if (search_desc)
> > + pfree(search_desc);
> > + if (local_desc)
> > + pfree(local_desc);
> > + if (remote_desc)
> > + pfree(remote_desc);
> >
> > Do we need these retail pfrees and in one in obtain_tuple_values()?
> > Which context is being used to allocate this memory and won't it reset
> > after applying or logging this change?
>
> I checked and the context is ApplyMessageContext and it is cleared after each
> replication protocol message. All pfree() are not needed anymore.
>
> Fixed the rest and update the commit message. Please see attached.
>

--v6-001 has a trailing whitespace issue.

--FWIW, I like the v6001 style better than v6002, where the conflict
type appears at the end.

-- Regarding this in one of the previous emails:
> BTW, I found that in CT_UPDATE_ORIGIN_DIFFERS and CT_DELETE_ORIGIN_DIFFERS cases
> assume localts is available because it can happen only when the commit_timestamp
> is tracked. I think the assumption is OK, but can we add Assert(localts) for them?
> If acceptable I can add up more top-up patch.

I think it is a good idea to add that Assert in a separate patch.

Other than these, I do not have any comments on attached patches..

thanks
Shveta