Consistent coding for the naming of LR workers
Hi,
There are different types of Logical Replication workers -- e.g.
tablesync workers, apply workers, and parallel apply workers.
The logging and errors often name these worker types, but during a
recent code review, I noticed some inconsistency in the way this is
done:
a) there is a common function get_worker_name() to return the name for
the worker type, -- OR --
b) the worker name is just hardcoded in the message/error
I think it is not ideal to cut/paste the same hardwired strings over
and over. IMO it just introduces an unnecessary risk of subtle naming
differences creeping in.
~~
It is better to have a *single* point where these worker names are
defined, so then all output uses identical LR worker nomenclature.
PSA a small patch to modify the code accordingly. This is not intended
to be a functional change - just a code cleanup.
Thoughts?
------
Kind Regards,
Peter Smith.
Fujitsu Australia
Attachments:
v1-0001-Consistent-naming-of-LR-workers.patchapplication/octet-stream; name=v1-0001-Consistent-naming-of-LR-workers.patchDownload
From f314184765c5180b26cea8ffba2038941e4b199c Mon Sep 17 00:00:00 2001
From: Peter Smith <peter.b.smith@fujitsu.com>
Date: Thu, 15 Jun 2023 12:19:49 +1000
Subject: [PATCH v1] Consistent naming of LR workers
---
.../replication/logical/applyparallelworker.c | 34 +++++++++++++++-------
src/backend/replication/logical/launcher.c | 6 ++--
src/backend/replication/logical/tablesync.c | 8 +++--
src/backend/replication/logical/worker.c | 27 +++++++++++------
src/include/replication/worker_internal.h | 4 +++
5 files changed, 54 insertions(+), 25 deletions(-)
diff --git a/src/backend/replication/logical/applyparallelworker.c b/src/backend/replication/logical/applyparallelworker.c
index 82c1ddc..979b1d4 100644
--- a/src/backend/replication/logical/applyparallelworker.c
+++ b/src/backend/replication/logical/applyparallelworker.c
@@ -716,7 +716,9 @@ ProcessParallelApplyInterrupts(void)
if (ShutdownRequestPending)
{
ereport(LOG,
- (errmsg("logical replication parallel apply worker for subscription \"%s\" has finished",
+ /* translator: first %s is the name of logical replication worker */
+ (errmsg("%s for subscription \"%s\" has finished",
+ LR_WORKER_NAME_APPLY_PARALLEL,
MySubscription->name)));
proc_exit(0);
@@ -821,8 +823,9 @@ LogicalParallelApplyLoop(shm_mq_handle *mqh)
Assert(shmq_res == SHM_MQ_DETACHED);
ereport(ERROR,
+ /* translator: first %s is the name of logical replication worker */
(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
- errmsg("lost connection to the logical replication apply worker")));
+ errmsg("lost connection to the %s", LR_WORKER_NAME_APPLY)));
}
MemoryContextReset(ApplyMessageContext);
@@ -1024,9 +1027,9 @@ HandleParallelApplyMessage(StringInfo msg)
*/
if (edata.context)
edata.context = psprintf("%s\n%s", edata.context,
- _("logical replication parallel apply worker"));
+ LR_WORKER_NAME_APPLY_PARALLEL);
else
- edata.context = pstrdup(_("logical replication parallel apply worker"));
+ edata.context = pstrdup(LR_WORKER_NAME_APPLY_PARALLEL);
/*
* Context beyond that should use the error context callbacks
@@ -1040,7 +1043,8 @@ HandleParallelApplyMessage(StringInfo msg)
*/
ereport(ERROR,
(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
- errmsg("logical replication parallel apply worker exited due to error"),
+ /* translator: first %s is the name of logical replication worker */
+ errmsg("%s exited due to error", LR_WORKER_NAME_APPLY_PARALLEL),
errcontext("%s", edata.context)));
}
@@ -1054,7 +1058,9 @@ HandleParallelApplyMessage(StringInfo msg)
break;
default:
- elog(ERROR, "unrecognized message type received from logical replication parallel apply worker: %c (message length %d bytes)",
+ /* translator: first %s is the name of logical replication worker */
+ elog(ERROR, "unrecognized message type received %s: %c (message length %d bytes)",
+ LR_WORKER_NAME_APPLY_PARALLEL,
msgtype, msg->len);
}
}
@@ -1126,8 +1132,9 @@ HandleParallelApplyMessages(void)
}
else
ereport(ERROR,
+ /* translator: first %s is the name of logical replication worker */
(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
- errmsg("lost connection to the logical replication parallel apply worker")));
+ errmsg("lost connection to the %s", LR_WORKER_NAME_APPLY_PARALLEL)));
}
MemoryContextSwitchTo(oldcontext);
@@ -1215,7 +1222,9 @@ pa_switch_to_partial_serialize(ParallelApplyWorkerInfo *winfo,
bool stream_locked)
{
ereport(LOG,
- (errmsg("logical replication apply worker will serialize the remaining changes of remote transaction %u to a file",
+ /* translator: first %s is the name of logical replication worker */
+ (errmsg("%s will serialize the remaining changes of remote transaction %u to a file",
+ LR_WORKER_NAME_APPLY,
winfo->shared->xid)));
/*
@@ -1299,8 +1308,9 @@ pa_wait_for_xact_finish(ParallelApplyWorkerInfo *winfo)
*/
if (pa_get_xact_state(winfo->shared) != PARALLEL_TRANS_FINISHED)
ereport(ERROR,
+ /* translator: first %s is the name of logical replication worker */
(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
- errmsg("lost connection to the logical replication parallel apply worker")));
+ errmsg("lost connection to the %s", LR_WORKER_NAME_APPLY_PARALLEL)));
}
/*
@@ -1373,7 +1383,8 @@ pa_start_subtrans(TransactionId current_xid, TransactionId top_xid)
pa_savepoint_name(MySubscription->oid, current_xid,
spname, sizeof(spname));
- elog(DEBUG1, "defining savepoint %s in logical replication parallel apply worker", spname);
+ /* translator: second %s is the name of logical replication worker */
+ elog(DEBUG1, "defining savepoint %s in %s", LR_WORKER_NAME_APPLY_PARALLEL, spname);
/* We must be in transaction block to define the SAVEPOINT. */
if (!IsTransactionBlock())
@@ -1468,7 +1479,8 @@ pa_stream_abort(LogicalRepStreamAbortData *abort_data)
pa_savepoint_name(MySubscription->oid, subxid, spname, sizeof(spname));
- elog(DEBUG1, "rolling back to savepoint %s in logical replication parallel apply worker", spname);
+ /* translator: second %s is the name of logical replication worker */
+ elog(DEBUG1, "rolling back to savepoint %s in %s", LR_WORKER_NAME_APPLY_PARALLEL, spname);
/*
* Search the subxactlist, determine the offset tracked for the
diff --git a/src/backend/replication/logical/launcher.c b/src/backend/replication/logical/launcher.c
index 87b5593..ee47793 100644
--- a/src/backend/replication/logical/launcher.c
+++ b/src/backend/replication/logical/launcher.c
@@ -465,13 +465,13 @@ retry:
if (OidIsValid(relid))
snprintf(bgw.bgw_name, BGW_MAXLEN,
- "logical replication worker for subscription %u sync %u", subid, relid);
+ "%s for subscription %u sync %u", LR_WORKER_NAME_TABLESYNC, subid, relid);
else if (is_parallel_apply_worker)
snprintf(bgw.bgw_name, BGW_MAXLEN,
- "logical replication parallel apply worker for subscription %u", subid);
+ "%s for subscription %u", LR_WORKER_NAME_APPLY_PARALLEL, subid);
else
snprintf(bgw.bgw_name, BGW_MAXLEN,
- "logical replication apply worker for subscription %u", subid);
+ "%s for subscription %u", LR_WORKER_NAME_APPLY, subid);
if (is_parallel_apply_worker)
snprintf(bgw.bgw_type, BGW_MAXLEN, "logical replication parallel worker");
diff --git a/src/backend/replication/logical/tablesync.c b/src/backend/replication/logical/tablesync.c
index abae8d4..a410520 100644
--- a/src/backend/replication/logical/tablesync.c
+++ b/src/backend/replication/logical/tablesync.c
@@ -150,7 +150,9 @@ finish_sync_worker(void)
StartTransactionCommand();
ereport(LOG,
- (errmsg("logical replication table synchronization worker for subscription \"%s\", table \"%s\" has finished",
+ /* translator: first %s is the name of logical replication worker */
+ (errmsg("%s for subscription \"%s\", table \"%s\" has finished",
+ LR_WORKER_NAME_TABLESYNC,
MySubscription->name,
get_rel_name(MyLogicalRepWorker->relid))));
CommitTransactionCommand();
@@ -619,7 +621,9 @@ process_syncing_tables_for_apply(XLogRecPtr current_lsn)
if (AllTablesyncsReady())
{
ereport(LOG,
- (errmsg("logical replication apply worker for subscription \"%s\" will restart so that two_phase can be enabled",
+ /* translator: first %s is the name of logical replication worker */
+ (errmsg("%s for subscription \"%s\" will restart so that two_phase can be enabled",
+ LR_WORKER_NAME_APPLY,
MySubscription->name)));
should_exit = true;
}
diff --git a/src/backend/replication/logical/worker.c b/src/backend/replication/logical/worker.c
index 0ee764d..b8c9eba 100644
--- a/src/backend/replication/logical/worker.c
+++ b/src/backend/replication/logical/worker.c
@@ -442,11 +442,11 @@ static const char *
get_worker_name(void)
{
if (am_tablesync_worker())
- return _("logical replication table synchronization worker");
+ return LR_WORKER_NAME_TABLESYNC;
else if (am_parallel_apply_worker())
- return _("logical replication parallel apply worker");
+ return LR_WORKER_NAME_APPLY_PARALLEL;
else
- return _("logical replication apply worker");
+ return LR_WORKER_NAME_APPLY;
}
/*
@@ -509,7 +509,9 @@ should_apply_changes_for_rel(LogicalRepRelMapEntry *rel)
rel->state != SUBREL_STATE_UNKNOWN)
ereport(ERROR,
(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
- errmsg("logical replication parallel apply worker for subscription \"%s\" will stop",
+ /* translator: first %s is the name of logical replication worker */
+ errmsg("%s for subscription \"%s\" will stop",
+ LR_WORKER_NAME_APPLY_PARALLEL,
MySubscription->name),
errdetail("Cannot handle streamed replication transactions using parallel apply workers until all tables have been synchronized.")));
@@ -1071,7 +1073,8 @@ apply_handle_begin_prepare(StringInfo s)
if (am_tablesync_worker())
ereport(ERROR,
(errcode(ERRCODE_PROTOCOL_VIOLATION),
- errmsg_internal("tablesync worker received a BEGIN PREPARE message")));
+ errmsg_internal("%s received a BEGIN PREPARE message",
+ LR_WORKER_NAME_TABLESYNC)));
/* There must not be an active streaming transaction. */
Assert(!TransactionIdIsValid(stream_xid));
@@ -1310,7 +1313,8 @@ apply_handle_stream_prepare(StringInfo s)
if (am_tablesync_worker())
ereport(ERROR,
(errcode(ERRCODE_PROTOCOL_VIOLATION),
- errmsg_internal("tablesync worker received a STREAM PREPARE message")));
+ errmsg_internal("%s received a STREAM PREPARE message",
+ LR_WORKER_NAME_TABLESYNC)));
logicalrep_read_stream_prepare(s, &prepare_data);
set_apply_error_context_xact(prepare_data.xid, prepare_data.prepare_lsn);
@@ -3950,7 +3954,9 @@ maybe_reread_subscription(void)
{
if (am_parallel_apply_worker())
ereport(LOG,
- (errmsg("logical replication parallel apply worker for subscription \"%s\" will stop because of a parameter change",
+ /* translator: first %s is the name of logical replication worker */
+ (errmsg("%s for subscription \"%s\" will stop because of a parameter change",
+ LR_WORKER_NAME_APPLY_PARALLEL,
MySubscription->name)));
else
ereport(LOG,
@@ -4512,7 +4518,9 @@ InitializeApplyWorker(void)
if (am_tablesync_worker())
ereport(LOG,
- (errmsg("logical replication table synchronization worker for subscription \"%s\", table \"%s\" has started",
+ /* translator: first %s is the name of logical replication worker */
+ (errmsg("%s for subscription \"%s\", table \"%s\" has started",
+ LR_WORKER_NAME_TABLESYNC,
MySubscription->name,
get_rel_name(MyLogicalRepWorker->relid))));
else
@@ -4707,7 +4715,8 @@ ApplyWorkerMain(Datum main_arg)
}
ereport(DEBUG1,
- (errmsg_internal("logical replication apply worker for subscription \"%s\" two_phase is %s",
+ (errmsg_internal("%s for subscription \"%s\" two_phase is %s",
+ LR_WORKER_NAME_APPLY,
MySubscription->name,
MySubscription->twophasestate == LOGICALREP_TWOPHASE_STATE_DISABLED ? "DISABLED" :
MySubscription->twophasestate == LOGICALREP_TWOPHASE_STATE_PENDING ? "PENDING" :
diff --git a/src/include/replication/worker_internal.h b/src/include/replication/worker_internal.h
index 343e781..aeb26dd 100644
--- a/src/include/replication/worker_internal.h
+++ b/src/include/replication/worker_internal.h
@@ -26,6 +26,10 @@
#include "storage/shm_toc.h"
#include "storage/spin.h"
+/* Names for the different kinds of logical replication workers. */
+#define LR_WORKER_NAME_TABLESYNC _("logical replication table synchronization worker")
+#define LR_WORKER_NAME_APPLY _("logical replication apply worker")
+#define LR_WORKER_NAME_APPLY_PARALLEL _("logical replication parallel apply worker")
typedef struct LogicalRepWorker
{
--
1.8.3.1
On Thu, Jun 15, 2023 at 8:13 AM Peter Smith <smithpb2250@gmail.com> wrote:
There are different types of Logical Replication workers -- e.g.
tablesync workers, apply workers, and parallel apply workers.The logging and errors often name these worker types, but during a
recent code review, I noticed some inconsistency in the way this is
done:
a) there is a common function get_worker_name() to return the name for
the worker type, -- OR --
b) the worker name is just hardcoded in the message/errorI think it is not ideal to cut/paste the same hardwired strings over
and over. IMO it just introduces an unnecessary risk of subtle naming
differences creeping in.~~
It is better to have a *single* point where these worker names are
defined, so then all output uses identical LR worker nomenclature.
+1. I think makes error strings in the code look a bit shorter. I
think it is better to park the patch for the next CF to avoid
forgetting about it.
--
With Regards,
Amit Kapila.
On 2023-Jun-15, Peter Smith wrote:
PSA a small patch to modify the code accordingly. This is not intended
to be a functional change - just a code cleanup.
From a translation standpoint, this doesn't seem good. Consider this
proposed message:
"lost connection to the %s"
It's not possible to translate "to the" correctly to Spanish because it
depends on the grammatical gender of the %s. At present this is not an
actual problem because all bgworker types have the same gender, but it
goes counter translability good practices. Also, other languages may
have different considerations.
You could use a generic term and then add a colon-separated or a quoted
indicator for its type:
lost connection to logical replication worker of type "parallel apply"
lost connection to logical replication worker: "parallel apply worker"
lost connection to logical replication worker: type "parallel apply worker"
and then make the type string (and nothing else in that message) be a
%s. But I don't think this looks very good.
I'd leave this alone, except if there are any actual inconsistencies in
which case they should be fixed specifically.
--
Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/
Thou shalt check the array bounds of all strings (indeed, all arrays), for
surely where thou typest "foo" someone someday shall type
"supercalifragilisticexpialidocious" (5th Commandment for C programmers)
At Thu, 15 Jun 2023 12:42:33 +1000, Peter Smith <smithpb2250@gmail.com> wrote in
It is better to have a *single* point where these worker names are
defined, so then all output uses identical LR worker nomenclature.PSA a small patch to modify the code accordingly. This is not intended
to be a functional change - just a code cleanup.Thoughts?
I generally like this direction when it actually decreases the number
of translatable messages without making grepping on the tree
excessively difficult. However, in this case, the patch doesn't seems
to reduce the translatable messages; instead, it makes grepping
difficult.
Addition to that, I'm inclined to concur with Alvaro regarding the
gramattical aspect.
Consequently, I'd prefer to leave these alone.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
Re: Alvaro's comment [1]Alvaro review comments - /messages/by-id/20230615103759.bkkv226czkcnuork@alvherre.pgsql "From a translation standpoint, this doesn't
seem good".
Except, please note that there are already multiple message format
strings in the HEAD code that look like "%s for subscription ...",
that are using the get_worker_name() function for the name
substitution.
e.g.
- "%s for subscription \"%s\" will stop because the subscription was removed"
- "%s for subscription \"%s\" will stop because the subscription was disabled"
- "%s for subscription \"%s\" will restart because of a parameter change"
- "%s for subscription %u will not start because the subscription was
removed during startup"
- "%s for subscription \"%s\" will not start because the subscription
was disabled during startup"
- "%s for subscription \"%s\" has started"
And many of my patch changes will result in a format string which has
exactly that same pattern:
e.g.
- "%s for subscription \"%s\" has finished"
- "%s for subscription \"%s\", table \"%s\" has finished"
- "%s for subscription \"%s\" will restart so that two_phase can be
enabledworker"
- "%s for subscription \"%s\" will stop"
- "%s for subscription \"%s\" will stop because of a parameter change"
- "%s for subscription \"%s\", table \"%s\" has started"
So, I don't think it is fair to say that these format strings are OK
for the existing HEAD code, but not OK for the patch code, when they
are both the same.
~~
OTOH, you are correct there are some more problematic messages (see
below - one of these you cited) that are not using the same pattern:
e.g.
- "lost connection to the %s"
- "%s exited due to error"
- "unrecognized message type received %s: %c (message length %d bytes)"
- "lost connection to the %s"
- "%s will serialize the remaining changes of remote transaction %u to a file"
- "lost connection to the %s"
- "defining savepoint %s in %s"
- "rolling back to savepoint %s in %s"
IMO it will be an improvement for all-round consistency if those also
get changed to use the similar pattern:
e.g. "lost connection to the %s" --> "%s for subscription \"%s",
cannot be contacted"
e.g. "defining savepoint %s in %s" --> "%s for subscription \"%s",
defining savepoint %s"
e.g. "rolling back to savepoint %s in %s" --> "%s for subscription
\"%s", rolling back to savepoint %s"
etc.
Thoughts?
------
Re: Horiguchi-san's comment [2]Horiguchi-san review comments - /messages/by-id/20230616.104327.1878440413098623268.horikyota.ntt@gmail.com "... instead, it makes grepping difficult."
Sorry, I didn't really understand how this patch makes grepping more
difficult. e.g. If you are grepping for any message about "table
synchronization worker" then you are currently hoping and relying that
there there are no differences in the wording of all the existing
messages. If something instead says "tablesync worker" you will
accidentally overlook it.
OTOH, this patch eliminates the guesswork and luck. In the example,
grepping for LR_WORKER_NAME_TABLESYNC will give you all the messages
you were looking for.
------
[1]: Alvaro review comments - /messages/by-id/20230615103759.bkkv226czkcnuork@alvherre.pgsql
/messages/by-id/20230615103759.bkkv226czkcnuork@alvherre.pgsql
[2]: Horiguchi-san review comments - /messages/by-id/20230616.104327.1878440413098623268.horikyota.ntt@gmail.com
/messages/by-id/20230616.104327.1878440413098623268.horikyota.ntt@gmail.com
Kind Regards,
Peter Smith.
Fujitsu Australia
On 2023-Jun-21, Peter Smith wrote:
Except, please note that there are already multiple message format
strings in the HEAD code that look like "%s for subscription ...",
that are using the get_worker_name() function for the name
substitution.e.g.
- "%s for subscription \"%s\" will stop because the subscription was removed"
- "%s for subscription \"%s\" will stop because the subscription was disabled"
- "%s for subscription \"%s\" will restart because of a parameter change"
- "%s for subscription %u will not start because the subscription was
removed during startup"
- "%s for subscription \"%s\" will not start because the subscription
was disabled during startup"
- "%s for subscription \"%s\" has started"
That is a terrible pattern in relatively new code. Let's get rid of it
entirely rather than continue to propagate it.
So, I don't think it is fair to say that these format strings are OK
for the existing HEAD code, but not OK for the patch code, when they
are both the same.
Agreed. Let's remove them all.
BTW this is documented:
https://www.postgresql.org/docs/15/nls-programmer.html#NLS-GUIDELINES
--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
"I suspect most samba developers are already technically insane...
Of course, since many of them are Australians, you can't tell." (L. Torvalds)
On 21.06.23 09:18, Alvaro Herrera wrote:
That is a terrible pattern in relatively new code. Let's get rid of it
entirely rather than continue to propagate it.So, I don't think it is fair to say that these format strings are OK
for the existing HEAD code, but not OK for the patch code, when they
are both the same.Agreed. Let's remove them all.
This is an open issue for PG16 translation. I propose the attached
patch to fix this. Mostly, this just reverts to the previous wordings.
(I don't think for these messages the difference between "apply worker"
and "parallel apply worker" is all that interesting to explode the
number of messages. AFAICT, the table sync worker case wasn't even
used, since callers always handled it separately.)
Attachments:
0001-Fix-untranslatable-error-message-assembly.patchtext/plain; charset=UTF-8; name=0001-Fix-untranslatable-error-message-assembly.patchDownload
From 6a1558629f97d83c8b11f204b39aceffc94dee8f Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@eisentraut.org>
Date: Wed, 12 Jul 2023 13:31:08 +0200
Subject: [PATCH] Fix untranslatable error message assembly
Discussion: https://www.postgresql.org/message-id/flat/CAHut%2BPt1xwATviPGjjtJy5L631SGf3qjV9XUCmxLu16cHamfgg%40mail.gmail.com
---
src/backend/replication/logical/worker.c | 44 +++++++-----------------
1 file changed, 12 insertions(+), 32 deletions(-)
diff --git a/src/backend/replication/logical/worker.c b/src/backend/replication/logical/worker.c
index 0ee764d68f..95b36ced13 100644
--- a/src/backend/replication/logical/worker.c
+++ b/src/backend/replication/logical/worker.c
@@ -435,20 +435,6 @@ static inline void reset_apply_error_context_info(void);
static TransApplyAction get_transaction_apply_action(TransactionId xid,
ParallelApplyWorkerInfo **winfo);
-/*
- * Return the name of the logical replication worker.
- */
-static const char *
-get_worker_name(void)
-{
- if (am_tablesync_worker())
- return _("logical replication table synchronization worker");
- else if (am_parallel_apply_worker())
- return _("logical replication parallel apply worker");
- else
- return _("logical replication apply worker");
-}
-
/*
* Form the origin name for the subscription.
*
@@ -3904,9 +3890,8 @@ maybe_reread_subscription(void)
if (!newsub)
{
ereport(LOG,
- /* translator: first %s is the name of logical replication worker */
- (errmsg("%s for subscription \"%s\" will stop because the subscription was removed",
- get_worker_name(), MySubscription->name)));
+ (errmsg("logical replication apply worker for subscription \"%s\" will stop because the subscription was removed",
+ MySubscription->name)));
/* Ensure we remove no-longer-useful entry for worker's start time */
if (!am_tablesync_worker() && !am_parallel_apply_worker())
@@ -3918,9 +3903,8 @@ maybe_reread_subscription(void)
if (!newsub->enabled)
{
ereport(LOG,
- /* translator: first %s is the name of logical replication worker */
- (errmsg("%s for subscription \"%s\" will stop because the subscription was disabled",
- get_worker_name(), MySubscription->name)));
+ (errmsg("logical replication apply worker for subscription \"%s\" will stop because the subscription was disabled",
+ MySubscription->name)));
apply_worker_exit();
}
@@ -3954,9 +3938,8 @@ maybe_reread_subscription(void)
MySubscription->name)));
else
ereport(LOG,
- /* translator: first %s is the name of logical replication worker */
- (errmsg("%s for subscription \"%s\" will restart because of a parameter change",
- get_worker_name(), MySubscription->name)));
+ (errmsg("logical replication apply worker for subscription \"%s\" will restart because of a parameter change",
+ MySubscription->name)));
apply_worker_exit();
}
@@ -4478,9 +4461,8 @@ InitializeApplyWorker(void)
if (!MySubscription)
{
ereport(LOG,
- /* translator: %s is the name of logical replication worker */
- (errmsg("%s for subscription %u will not start because the subscription was removed during startup",
- get_worker_name(), MyLogicalRepWorker->subid)));
+ (errmsg("logical replication apply worker for subscription %u will not start because the subscription was removed during startup",
+ MyLogicalRepWorker->subid)));
/* Ensure we remove no-longer-useful entry for worker's start time */
if (!am_tablesync_worker() && !am_parallel_apply_worker())
@@ -4494,9 +4476,8 @@ InitializeApplyWorker(void)
if (!MySubscription->enabled)
{
ereport(LOG,
- /* translator: first %s is the name of logical replication worker */
- (errmsg("%s for subscription \"%s\" will not start because the subscription was disabled during startup",
- get_worker_name(), MySubscription->name)));
+ (errmsg("logical replication apply worker for subscription \"%s\" will not start because the subscription was disabled during startup",
+ MySubscription->name)));
apply_worker_exit();
}
@@ -4517,9 +4498,8 @@ InitializeApplyWorker(void)
get_rel_name(MyLogicalRepWorker->relid))));
else
ereport(LOG,
- /* translator: first %s is the name of logical replication worker */
- (errmsg("%s for subscription \"%s\" has started",
- get_worker_name(), MySubscription->name)));
+ (errmsg("logical replication apply worker for subscription \"%s\" has started",
+ MySubscription->name)));
CommitTransactionCommand();
}
--
2.41.0
On Wed, Jul 12, 2023 at 9:35 PM Peter Eisentraut <peter@eisentraut.org> wrote:
On 21.06.23 09:18, Alvaro Herrera wrote:
That is a terrible pattern in relatively new code. Let's get rid of it
entirely rather than continue to propagate it.So, I don't think it is fair to say that these format strings are OK
for the existing HEAD code, but not OK for the patch code, when they
are both the same.Agreed. Let's remove them all.
This is an open issue for PG16 translation. I propose the attached
patch to fix this. Mostly, this just reverts to the previous wordings.
(I don't think for these messages the difference between "apply worker"
and "parallel apply worker" is all that interesting to explode the
number of messages. AFAICT, the table sync worker case wasn't even
used, since callers always handled it separately.)
I thought the get_worker_name function was reachable by tablesync workers also.
Since ApplyWorkerMain is a common entry point for both leader apply
workers and tablesync workers, any logs in that code path could
potentially be from either kind of worker. At least, when the function
was first introduced (around patch v43-0001? [1]v43-0001 - /messages/by-id/OS0PR01MB5716E366874B253B58FC0A23943C9@OS0PR01MB5716.jpnprd01.prod.outlook.com) it was also
replacing some tablesync logs.
------
[1]: v43-0001 - /messages/by-id/OS0PR01MB5716E366874B253B58FC0A23943C9@OS0PR01MB5716.jpnprd01.prod.outlook.com
Kind Regards,
Peter Smith.
Fujitsu Australia
On 13.07.23 06:59, Peter Smith wrote:
On Wed, Jul 12, 2023 at 9:35 PM Peter Eisentraut <peter@eisentraut.org> wrote:
On 21.06.23 09:18, Alvaro Herrera wrote:
That is a terrible pattern in relatively new code. Let's get rid of it
entirely rather than continue to propagate it.So, I don't think it is fair to say that these format strings are OK
for the existing HEAD code, but not OK for the patch code, when they
are both the same.Agreed. Let's remove them all.
This is an open issue for PG16 translation. I propose the attached
patch to fix this. Mostly, this just reverts to the previous wordings.
(I don't think for these messages the difference between "apply worker"
and "parallel apply worker" is all that interesting to explode the
number of messages. AFAICT, the table sync worker case wasn't even
used, since callers always handled it separately.)I thought the get_worker_name function was reachable by tablesync workers also.
Since ApplyWorkerMain is a common entry point for both leader apply
workers and tablesync workers, any logs in that code path could
potentially be from either kind of worker. At least, when the function
was first introduced (around patch v43-0001? [1]) it was also
replacing some tablesync logs.
I suppose we could just say "logical replication worker" in all cases.
That should be enough precision for the purpose of these messages.
On Thu, Jul 13, 2023 at 4:07 PM Peter Eisentraut <peter@eisentraut.org> wrote:
On 13.07.23 06:59, Peter Smith wrote:
On Wed, Jul 12, 2023 at 9:35 PM Peter Eisentraut <peter@eisentraut.org> wrote:
On 21.06.23 09:18, Alvaro Herrera wrote:
That is a terrible pattern in relatively new code. Let's get rid of it
entirely rather than continue to propagate it.So, I don't think it is fair to say that these format strings are OK
for the existing HEAD code, but not OK for the patch code, when they
are both the same.Agreed. Let's remove them all.
This is an open issue for PG16 translation. I propose the attached
patch to fix this. Mostly, this just reverts to the previous wordings.
(I don't think for these messages the difference between "apply worker"
and "parallel apply worker" is all that interesting to explode the
number of messages. AFAICT, the table sync worker case wasn't even
used, since callers always handled it separately.)I thought the get_worker_name function was reachable by tablesync workers also.
Since ApplyWorkerMain is a common entry point for both leader apply
workers and tablesync workers, any logs in that code path could
potentially be from either kind of worker. At least, when the function
was first introduced (around patch v43-0001? [1]) it was also
replacing some tablesync logs.I suppose we could just say "logical replication worker" in all cases.
That should be enough precision for the purpose of these messages.
+1
Regards,
--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com
On 2023-Jul-13, Peter Eisentraut wrote:
I suppose we could just say "logical replication worker" in all cases. That
should be enough precision for the purpose of these messages.
Agreed. IMO the user doesn't care about specifics.
--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
On 13.07.23 11:29, Alvaro Herrera wrote:
On 2023-Jul-13, Peter Eisentraut wrote:
I suppose we could just say "logical replication worker" in all cases. That
should be enough precision for the purpose of these messages.Agreed. IMO the user doesn't care about specifics.
Ok, committed.