From: Kyotaro Horiguchi <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
Date: Tue, 26 Mar 2024 14:01:16 +0900 (JST)
Message-ID: <20240326.140116.1116279856046587865.horikyota.ntt@gmail.com> (raw)
In-Reply-To: <E1roio3-005ZLs-Q3@gemulon.postgresql.org>
References: <E1roio3-005ZLs-Q3@gemulon.postgresql.org>
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
Attachments:pg_createsubscriber_message_fixes_v1.patch (text/x-patch, 1.5K) downloaddiff --git a/src/bin/pg_basebackup/pg_createsubscriber.c b/src/bin/pg_basebackup/pg_createsubscriber.cindex 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);
}
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-committers@postgresql.org
Cc: horikyota.ntt@gmail.com, peter@eisentraut.org, pgsql-committers@lists.postgresql.org
Subject: Re: pgsql: pg_createsubscriber: creates a new logical replica from a standb
In-Reply-To: <20240326.140116.1116279856046587865.horikyota.ntt@gmail.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox