On Mon, Jan 19, 2026 at 4:25 PM Hayato Kuroda (Fujitsu) <[email protected]> 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
