On Fri, 4 Apr 2025 at 09:36, Amit Kapila <amit.kapil...@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 --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.
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
From 40ef017e6e5f22fb8650495935c186b5a4a56878 Mon Sep 17 00:00:00 2001
From: Vignesh <vignes...@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
