Date: Tue, 26 Mar 2024 14:01:16 +0900 (JST)
Message-Id: <20240326.140116.1116279856046587865.horikyota.ntt@gmail.com>
To: peter@eisentraut.org
Cc: pgsql-committers@lists.postgresql.org
Subject: Re: pgsql: pg_createsubscriber: creates a new logical replica from
 a standb
From: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
In-Reply-To: <E1roio3-005ZLs-Q3@gemulon.postgresql.org>
References: <E1roio3-005ZLs-Q3@gemulon.postgresql.org>
User-Agent: Mew version 6.8 on Emacs 27.2
Mime-Version: 1.0
Content-Type: Multipart/Mixed;
 boundary="--Next_Part(Tue_Mar_26_14_01_16_2024_374)--"
Content-Transfer-Encoding: 7bit
Archived-At: 
 <https://www.postgresql.org/message-id/20240326.140116.1116279856046587865.horikyota.ntt%40gmail.com>
Precedence: bulk

----Next_Part(Tue_Mar_26_14_01_16_2024_374)--
Content-Type: Text/Plain; charset=us-ascii
Content-Transfer-Encoding: 7bit

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.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

----Next_Part(Tue_Mar_26_14_01_16_2024_374)--
Content-Type: Text/X-Patch; charset=us-ascii
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
 filename="pg_createsubscriber_message_fixes_v1.patch"

diff --git a/src/bin/pg_basebackup/pg_createsubscriber.c b/src/bin/pg_basebackup/pg_createsubscriber.c
index b8f8269340..a6e0986670 100644
--- a/src/bin/pg_basebackup/pg_createsubscriber.c
+++ b/src/bin/pg_basebackup/pg_createsubscriber.c
@@ -372,8 +372,7 @@ check_data_directory(const char *datadir)
 		if (errno == ENOENT)
 			pg_fatal("data directory \"%s\" does not exist", datadir);
 		else
-			pg_fatal("could not access directory \"%s\": %s", datadir,
-					 strerror(errno));
+			pg_fatal("could not access directory \"%s\": %m", datadir);
 	}
 
 	snprintf(versionfile, MAXPGPATH, "%s/PG_VERSION", datadir);
@@ -684,7 +683,7 @@ generate_object_name(PGconn *conn)
 
 	if (PQntuples(res) != 1)
 	{
-		pg_log_error("could not obtain database OID: got %d rows, expected %d rows",
+		pg_log_error("could not obtain database OID: got %d rows, expected %d row",
 					 PQntuples(res), 1);
 		disconnect_database(conn, true);
 	}
@@ -920,7 +919,7 @@ check_publisher(const struct LogicalRepInfo *dbinfo)
 
 	if (strcmp(wal_level, "logical") != 0)
 	{
-		pg_log_error("publisher requires wal_level >= logical");
+		pg_log_error("publisher requires wal_level >= \"logical\"");
 		failed = true;
 	}
 
@@ -1649,7 +1648,7 @@ set_replication_progress(PGconn *conn, const struct LogicalRepInfo *dbinfo, cons
 
 	if (PQntuples(res) != 1 && !dry_run)
 	{
-		pg_log_error("could not obtain subscription OID: got %d rows, expected %d rows",
+		pg_log_error("could not obtain subscription OID: got %d rows, expected %d row",
 					 PQntuples(res), 1);
 		disconnect_database(conn, true);
 	}

----Next_Part(Tue_Mar_26_14_01_16_2024_374)----