Re: Add the replication origin name and commit-LSN to logical replication worker errcontext - Mailing list pgsql-hackers

From Amit Kapila
Subject Re: Add the replication origin name and commit-LSN to logical replication worker errcontext
Date
Msg-id CAA4eK1LHd+vttOsemJ6kZF-Ys_LUZOi0WRHbVs+_C7oTbW6Vtw@mail.gmail.com
Whole thread Raw
In response to Add the replication origin name and commit-LSN to logical replication worker errcontext  (Masahiko Sawada <sawada.mshk@gmail.com>)
Responses Re: Add the replication origin name and commit-LSN to logical replication worker errcontext
List pgsql-hackers
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.



pgsql-hackers by date:

Previous
From: Peter Smith
Date:
Subject: Re: Add the replication origin name and commit-LSN to logical replication worker errcontext
Next
From: Andy Fan
Date:
Subject: Re: Condition pushdown: why (=) is pushed down into join, but BETWEEN or >= is not?