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
