Re: Consistent coding for the naming of LR workers

From: Peter Smith <smithpb2250(at)gmail(dot)com>
To: Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org>
Cc: PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org>
Subject: Re: Consistent coding for the naming of LR workers
Date: 2023-06-21 02:31:46
Message-ID: CAHut+PvwBcV=EZpuV6SRgg5D_OzcaRimr07Wbj-DMAEAwARDYg@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

Re: Alvaro's comment [1] "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] "... 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 -
https://www.postgresql.org/message-id/20230615103759.bkkv226czkcnuork%40alvherre.pgsql
[2] Horiguchi-san review comments -
https://www.postgresql.org/message-id/20230616.104327.1878440413098623268.horikyota.ntt%40gmail.com

Kind Regards,
Peter Smith.
Fujitsu Australia

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message John Naylor 2023-06-21 02:35:36 Re: remap the .text segment into huge pages at run time
Previous Message Peter Geoghegan 2023-06-21 01:28:58 Re: Adding further hardening to nbtree page deletion