Improve error reporting for few options in pg_createsubscriber
Hi,
Currently, error reports for database, publication, subscription, and
replication slots do not include the option name. This has been
addressed by including the option name in the error messages, ensuring
consistency similar to remove option. Additionally, pg_log_error and
exit(1) have been replaced with a single pg_fatal statement which
means the same. The attached patch implements these changes.
Regards,
Vignesh
Attachments:
0001-Improve-error-reporting-consistency-in-pg_createsubs.patchtext/x-patch; charset=US-ASCII; name=0001-Improve-error-reporting-consistency-in-pg_createsubs.patchDownload
From 4d1c8ebcb5ad8c2f17b027d5bf23d063da419c9d Mon Sep 17 00:00:00 2001
From: Vignesh <vignesh21@gmail.com>
Date: Sat, 22 Mar 2025 18:56:15 +0530
Subject: [PATCH] Improve error reporting consistency in pg_createsubscriber
Ensure consistent error reporting in pg_createsubscriber
by including the option name in error messages. Additionally,
replace pg_log_error and exit calls with pg_fatal.
---
src/bin/pg_basebackup/pg_createsubscriber.c | 20 ++++----------------
1 file changed, 4 insertions(+), 16 deletions(-)
diff --git a/src/bin/pg_basebackup/pg_createsubscriber.c b/src/bin/pg_basebackup/pg_createsubscriber.c
index e2d6b7544bf..2b9ec24efc2 100644
--- a/src/bin/pg_basebackup/pg_createsubscriber.c
+++ b/src/bin/pg_basebackup/pg_createsubscriber.c
@@ -2063,10 +2063,7 @@ main(int argc, char **argv)
num_dbs++;
}
else
- {
- pg_log_error("database \"%s\" specified more than once", optarg);
- exit(1);
- }
+ pg_fatal("database \"%s\" specified more than once for --database", optarg);
break;
case 'D':
subscriber_dir = pg_strdup(optarg);
@@ -2113,10 +2110,7 @@ main(int argc, char **argv)
num_pubs++;
}
else
- {
- pg_log_error("publication \"%s\" specified more than once", optarg);
- exit(1);
- }
+ pg_fatal("publication \"%s\" specified more than once for --publication", optarg);
break;
case 3:
if (!simple_string_list_member(&opt.replslot_names, optarg))
@@ -2125,10 +2119,7 @@ main(int argc, char **argv)
num_replslots++;
}
else
- {
- pg_log_error("replication slot \"%s\" specified more than once", optarg);
- exit(1);
- }
+ pg_fatal("replication slot \"%s\" specified more than once for --replication-slot", optarg);
break;
case 4:
if (!simple_string_list_member(&opt.sub_names, optarg))
@@ -2137,10 +2128,7 @@ main(int argc, char **argv)
num_subs++;
}
else
- {
- pg_log_error("subscription \"%s\" specified more than once", optarg);
- exit(1);
- }
+ pg_fatal("subscription \"%s\" specified more than once for --subscription", optarg);
break;
default:
/* getopt_long already emitted a complaint */
--
2.43.0
Dear Vignesh,
Currently, error reports for database, publication, subscription, and
replication slots do not include the option name. This has been
addressed by including the option name in the error messages, ensuring
consistency similar to remove option.
Confirmed all error reporting in switch have its option name.
Additionally, pg_log_error and
exit(1) have been replaced with a single pg_fatal statement which
means the same. The attached patch implements these changes.
I grepped source and confirmed that exit(1) just after pg_log_error()
does not exist anymore. Several lines have below style, but IIUC it is
out-of-scope of the patch.
pg_log_error();
pg_log_error_hint();
exit(1);
Best regards,
Hayato Kuroda
FUJITSU LIMITED
On Sat, Mar 22, 2025 at 7:26 PM vignesh C <vignesh21@gmail.com> wrote:
Hi,
Currently, error reports for database, publication, subscription, and
replication slots do not include the option name. This has been
addressed by including the option name in the error messages, ensuring
consistency similar to remove option. Additionally, pg_log_error and
exit(1) have been replaced with a single pg_fatal statement which
means the same. The attached patch implements these changes.
A few comments:
*
pg_createsubscriber.exe -D ..\..\data_standby -d db1 -d db2 -d db1
--publication pub1 --publication pub1 -P "dbname=postgres port=5432"
-p 5444 --dry-run
pg_createsubscriber: error: database "db1" specified more than once
for --database
Even when user has used short form, we are displaying long form for
database. I suggest to use both short and long form where applicable.
*
"invalid object type \"%s\" specified for --remove". Isn't it better
to use the short form as well in this error message?
--
With Regards,
Amit Kapila.
On Fri, 4 Apr 2025 at 09:36, Amit Kapila <amit.kapila16@gmail.com> wrote:
A few comments:
*
pg_createsubscriber.exe -D ..\..\data_standby -d db1 -d db2 -d db1
--publication pub1 --publication pub1 -P "dbname=postgres port=5432"
-p 5444 --dry-run
pg_createsubscriber: error: database "db1" specified more than once
for --databaseEven when user has used short form, we are displaying long form for
database. I suggest to use both short and long form where applicable.
Fixed this.
*
"invalid object type \"%s\" specified for --remove". Isn't it better
to use the short form as well in this error message?
Fixed this.
The attached v2 version patch has the changes for the same.
Regards,
Vignesh
Attachments:
v2-0001-Improve-error-reporting-consistency-in-pg_creates.patchtext/x-patch; charset=US-ASCII; name=v2-0001-Improve-error-reporting-consistency-in-pg_creates.patchDownload
From 40ef017e6e5f22fb8650495935c186b5a4a56878 Mon Sep 17 00:00:00 2001
From: Vignesh <vignesh21@gmail.com>
Date: Sat, 22 Mar 2025 18:56:15 +0530
Subject: [PATCH v2] Improve error reporting consistency in pg_createsubscriber
Ensure consistent error reporting in pg_createsubscriber
by including the option name in error messages. Additionally,
replace pg_log_error and exit calls with pg_fatal.
---
src/bin/pg_basebackup/pg_createsubscriber.c | 26 +++++--------------
.../t/040_pg_createsubscriber.pl | 4 +--
2 files changed, 9 insertions(+), 21 deletions(-)
diff --git a/src/bin/pg_basebackup/pg_createsubscriber.c b/src/bin/pg_basebackup/pg_createsubscriber.c
index eed3793c816..1d2f39e3926 100644
--- a/src/bin/pg_basebackup/pg_createsubscriber.c
+++ b/src/bin/pg_basebackup/pg_createsubscriber.c
@@ -2124,10 +2124,7 @@ main(int argc, char **argv)
num_dbs++;
}
else
- {
- pg_log_error("database \"%s\" specified more than once", optarg);
- exit(1);
- }
+ pg_fatal("database \"%s\" specified more than once for -d/--database", optarg);
break;
case 'D':
subscriber_dir = pg_strdup(optarg);
@@ -2146,7 +2143,7 @@ main(int argc, char **argv)
if (!simple_string_list_member(&opt.objecttypes_to_remove, optarg))
simple_string_list_append(&opt.objecttypes_to_remove, optarg);
else
- pg_fatal("object type \"%s\" is specified more than once for --remove", optarg);
+ pg_fatal("object type \"%s\" is specified more than once for -R/--remove", optarg);
break;
case 's':
opt.socket_dir = pg_strdup(optarg);
@@ -2174,10 +2171,7 @@ main(int argc, char **argv)
num_pubs++;
}
else
- {
- pg_log_error("publication \"%s\" specified more than once", optarg);
- exit(1);
- }
+ pg_fatal("publication \"%s\" specified more than once for --publication", optarg);
break;
case 3:
if (!simple_string_list_member(&opt.replslot_names, optarg))
@@ -2186,10 +2180,7 @@ main(int argc, char **argv)
num_replslots++;
}
else
- {
- pg_log_error("replication slot \"%s\" specified more than once", optarg);
- exit(1);
- }
+ pg_fatal("replication slot \"%s\" specified more than once for --replication-slot", optarg);
break;
case 4:
if (!simple_string_list_member(&opt.sub_names, optarg))
@@ -2198,10 +2189,7 @@ main(int argc, char **argv)
num_subs++;
}
else
- {
- pg_log_error("subscription \"%s\" specified more than once", optarg);
- exit(1);
- }
+ pg_fatal("subscription \"%s\" specified more than once for --subscription", optarg);
break;
default:
/* getopt_long already emitted a complaint */
@@ -2226,7 +2214,7 @@ main(int argc, char **argv)
if (bad_switch)
{
- pg_log_error("%s cannot be used with --all", bad_switch);
+ pg_log_error("%s cannot be used with -a/--all", bad_switch);
pg_log_error_hint("Try \"%s --help\" for more information.", progname);
exit(1);
}
@@ -2352,7 +2340,7 @@ main(int argc, char **argv)
dbinfos.objecttypes_to_remove |= OBJECTTYPE_PUBLICATIONS;
else
{
- pg_log_error("invalid object type \"%s\" specified for --remove", cell->val);
+ pg_log_error("invalid object type \"%s\" specified for -R/--remove", cell->val);
pg_log_error_hint("The valid option is: \"publications\"");
exit(1);
}
diff --git a/src/bin/pg_basebackup/t/040_pg_createsubscriber.pl b/src/bin/pg_basebackup/t/040_pg_createsubscriber.pl
index 80153f7d77e..2d532fee567 100644
--- a/src/bin/pg_basebackup/t/040_pg_createsubscriber.pl
+++ b/src/bin/pg_basebackup/t/040_pg_createsubscriber.pl
@@ -399,7 +399,7 @@ command_fails_like(
'--database' => $db1,
'--all',
],
- qr/--database cannot be used with --all/,
+ qr/--database cannot be used with -a\/--all/,
'fail if --database is used with --all');
# run pg_createsubscriber with '--publication' and '--all' and verify
@@ -416,7 +416,7 @@ command_fails_like(
'--all',
'--publication' => 'pub1',
],
- qr/--publication cannot be used with --all/,
+ qr/--publication cannot be used with -a\/--all/,
'fail if --publication is used with --all');
# run pg_createsubscriber with '--all' option
--
2.43.0
On Fri, Apr 4, 2025 at 10:11 AM vignesh C <vignesh21@gmail.com> wrote:
On Fri, 4 Apr 2025 at 09:36, Amit Kapila <amit.kapila16@gmail.com> wrote:
The attached v2 version patch has the changes for the same.
Pushed.
--
With Regards,
Amit Kapila.