Subject: Re: Improve error reporting for few options in pg_createsubscriber

From: Amit Kapila <[email-masked]>
Date: Fri, 4 Apr 2025 09:36:43 +0530

On Sat, Mar 22, 2025 at 7:26 PM vignesh C <[email-masked]> 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.

---

From: vignesh C <[email-masked]>
Date: Fri, 4 Apr 2025 10:11:24 +0530

> database. I suggest to use both short and long form where applicable.

Fixed this.

> 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 <[email-masked]>
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