Re: Consistent coding for the naming of LR workers

Поиск
Список
Период
Сортировка
От Peter Smith
Тема Re: Consistent coding for the naming of LR workers
Дата
Msg-id CAHut+PvwBcV=EZpuV6SRgg5D_OzcaRimr07Wbj-DMAEAwARDYg@mail.gmail.com
обсуждение исходный текст
Ответ на Re: Consistent coding for the naming of LR workers  (Alvaro Herrera <alvherre@alvh.no-ip.org>)
Ответы Re: Consistent coding for the naming of LR workers  (Alvaro Herrera <alvherre@alvh.no-ip.org>)
Список 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



В списке pgsql-hackers по дате отправления:

Предыдущее
От: Peter Geoghegan
Дата:
Сообщение: Re: Adding further hardening to nbtree page deletion
Следующее
От: John Naylor
Дата:
Сообщение: Re: remap the .text segment into huge pages at run time