Re: pgsql: pg_createsubscriber: creates a new logical replica from a standb

I have committed your patch to tidy this up.  Thanks.

On 26.03.24 06:01, Kyotaro Horiguchi wrote:
> Hello.
> 
> This commit added the following error message:
> 
> pg_createsubscriber.c: 375
>>             pg_fatal("could not access directory \"%s\": %s", datadir,
>>                      strerror(errno));
> 
> Although other messages use %s with PQresultErrorMessage(), regarding
> this specific message, shouldn't we use %m instead of %s + strerror()?
> I'm not sure if that would be better.
> 
> 
> pg_createsubscriber.c: 687
>>         pg_log_error("could not obtain database OID: got %d rows, expected %d rows",
>>                      PQntuples(res), 1);
> pg_createsubscriber.c: 1652
>>         pg_log_error("could not obtain subscription OID: got %d rows, expected %d rows",
> 
> In these messages, the second %d is always written as "1 rows",
> whereas a similar message correctly uses "1 row".
> 
> pg_createsubscriber.c: 561
>>         pg_log_error("could not get system identifier: got %d rows, expected %d row",
>>                      PQntuples(res), 1);
> 
> I think it would be better to change the former instances to "%d row",
> or to change both to "1 row". I'd like to go the second way but maybe
> we should take the first way following our convention.
> 
> 
> pg_createsubscriber.c: 923
>>         pg_log_error("publisher requires wal_level >= logical");
> 
> We discussed this message in relation to commit 801792e528, and
> decided to quote "logical" to clarify that it is a string literal. I'd
> like to follow the conclusion here, too.
> 
> regards.
>