On Fri, 24 Apr 2026 at 12:30, Peter Smith <[email protected]> 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
v3-0001-replication-fix-translation-issues-in-tuple-value.patch
Description: Binary data
