Feedback-ID: ie0a040ee:Fastmail
Message-ID: <9609b975-622b-4846-981d-7a5896cd6b51@eisentraut.org>
Date: Tue, 26 Mar 2024 08:30:56 +0100
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: pgsql: pg_createsubscriber: creates a new logical replica from a
 standb
To: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Cc: pgsql-committers@lists.postgresql.org
References: <E1roio3-005ZLs-Q3@gemulon.postgresql.org>
 <20240326.140116.1116279856046587865.horikyota.ntt@gmail.com>
Content-Language: en-US
From: Peter Eisentraut <peter@eisentraut.org>
In-Reply-To: <20240326.140116.1116279856046587865.horikyota.ntt@gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Archived-At: 
 <https://www.postgresql.org/message-id/9609b975-622b-4846-981d-7a5896cd6b51%40eisentraut.org>
Precedence: bulk

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.
>