On Fri, 24 Apr 2026 at 12:30, Peter Smith <smithpb2250@gmail.com> wrote:
>
> Hi Vignesh.
>
> Some review comments for patch v2-0001.
>
> ======
>
> append_tuple_value_detail:
>
> 1.
> + if (first)
> + appendStringInfoString(buf, "tuple data not available (insufficient
> privileges)");
>
> The logic to use first==true to mean "insufficient privilege" seems
> strange. Presumably, the only way to get this message is when the
> `continue` of the prior loop happened at *every* iteration.
>
> Isn't it ambiguous? e.g. What if there are multiple tuple_values but
> only 1 tuple_value was NULL? Then the boolean `first` will not be
> true, so there was something unavailable due to insufficient
> privileges that has gone unreported (???).
I have removed this from this patch now. We can handle this separately
as it is not required for the current issue.
> ~~~
>
> errdetail_apply_conflict:
>
> 2.
> case CT_UPDATE_DELETED:
> - appendStringInfoString(&err_detail, _("Could not find the row to be
> updated"));
>
> - append_tuple_value_detail(&err_detail,
> - list_make2(remote_desc, search_desc),
> - true);
> + append_tuple_value_detail(&tuple_buf,
> + list_make2(remote_desc, search_desc));
> + appendStringInfo(&err_detail, _("Could not find the row to be
> updated: %s.\n"),
> + tuple_buf.data);
>
> Results in a whitespace line after the CT_UPDATE_DELETED: that was not
> there before.
Modified
The attached v3 version patch has the changes for the same. This patch
has also addressed Amit's comments from [1].
[1] - https://www.postgresql.org/message-id/CAA4eK1%2B_Mr1NEzEnGdyZ86r4Bp9a4DwGhnmuKtxwDFv9n5FATg%40mail.gmail.com
Regards,
Vignesh
