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.
Best regards,
Hayato Kuroda
FUJITSU LIMITED
