Re: StringInfo fixes, v19 edition. Plus a few oddities - Mailing list pgsql-hackers

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 (???).

~~~

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.

======
Kind Regards,
Peter Smith.
Fujitsu Australia