Re: Add the replication origin name and commit-LSN to logical replication worker errcontext

On Mon, Feb 28, 2022 at 5:46 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> I've attached two patches: the first one changes
> apply_error_callback() so that it uses complete sentences with if-else
> blocks in order to have a translation work,
>

This is an improvement over what we have now but I think this is still
not completely correct as per message translation rules:
+ else
+ errcontext("processing remote data during \"%s\" in transaction %u at %s",
+    logicalrep_message_type(errarg->command),
+    errarg->remote_xid,
+    (errarg->ts != 0) ? timestamptz_to_str(errarg->ts) : "(not-set)");

As per guidelines [1][2], we don't prefer to construct messages at
run-time aka we can do better for the following part: +    (errarg->ts
!= 0) ? timestamptz_to_str(errarg->ts) : "(not-set)". I think we need
to use if-else here to split it further. If you agree, then the same
needs to be dealt with in other parts of the patch as well.

[1] - https://www.postgresql.org/docs/devel/nls-programmer.html#NLS-GUIDELINES
[2] - Do not construct sentences at run-time, like:
printf("Files were %s.\n", flag ? "copied" : "removed");
The word order within the sentence might be different in other languages.

-- 
With Regards,
Amit Kapila.