v19-0009-Remove-P-and-use-primary_conninfo-instead.patch

From 0c1bb96f5794518cc2c73b35e7238d2940925ee6 Mon Sep 17 00:00:00 2001
From: Hayato Kuroda <kuroda.hayato@fujitsu.com>
Date: Fri, 2 Feb 2024 09:31:44 +0000
Subject: [PATCH v19 9/9] Remove -P and use primary_conninfo instead

XXX: This may be a problematic when the OS user who started target instance is
not the current OS user and PGPASSWORD environment variable was used for
connecting to the primary server. In this case, the password would not be
written in the primary_conninfo and the PGPASSWORD variable might not be set.
This may lead an connection error. Is this a real issue? Note that using
PGPASSWORD is not recommended.
---
 doc/src/sgml/ref/pg_createsubscriber.sgml     |  17 +--
 src/bin/pg_basebackup/pg_createsubscriber.c   | 101 ++++++++++++------
 .../t/040_pg_createsubscriber.pl              |  11 +-
 .../t/041_pg_createsubscriber_standby.pl      |  10 +-
 4 files changed, 72 insertions(+), 67 deletions(-)

diff --git a/doc/src/sgml/ref/pg_createsubscriber.sgml b/doc/src/sgml/ref/pg_createsubscriber.sgml
index 275a3365da..e10270fc24 100644
--- a/doc/src/sgml/ref/pg_createsubscriber.sgml
+++ b/doc/src/sgml/ref/pg_createsubscriber.sgml
@@ -29,11 +29,6 @@ PostgreSQL documentation
      <arg choice="plain"><option>--pgdata</option></arg>
     </group>
     <replaceable>datadir</replaceable>
-    <group choice="req">
-     <arg choice="plain"><option>-P</option></arg>
-     <arg choice="plain"><option>--publisher-server</option></arg>
-    </group>
-    <replaceable>connstr</replaceable>
     <group choice="req">
      <arg choice="plain"><option>-S</option></arg>
      <arg choice="plain"><option>--subscriber-server</option></arg>
@@ -160,16 +155,6 @@ PostgreSQL documentation
       </listitem>
      </varlistentry>
 
-     <varlistentry>
-      <term><option>-P <replaceable class="parameter">connstr</replaceable></option></term>
-      <term><option>--publisher-server=<replaceable class="parameter">connstr</replaceable></option></term>
-      <listitem>
-       <para>
-        The connection string to the publisher. For details see <xref linkend="libpq-connstring"/>.
-       </para>
-      </listitem>
-     </varlistentry>
-
      <varlistentry>
       <term><option>-S <replaceable class="parameter">connstr</replaceable></option></term>
       <term><option>--subscriber-server=<replaceable class="parameter">connstr</replaceable></option></term>
@@ -380,7 +365,7 @@ PostgreSQL documentation
    create subscriptions for databases <literal>hr</literal> and
    <literal>finance</literal> from a physical standby:
 <screen>
-<prompt>$</prompt> <userinput>pg_createsubscriber -D /usr/local/pgsql/data -P "host=foo" -S "host=localhost" -d hr -d finance</userinput>
+<prompt>$</prompt> <userinput>pg_createsubscriber -D /usr/local/pgsql/data -S "host=localhost" -d hr -d finance</userinput>
 </screen>
   </para>
 
diff --git a/src/bin/pg_basebackup/pg_createsubscriber.c b/src/bin/pg_basebackup/pg_createsubscriber.c
index 28ea5835e9..cd72e77fef 100644
--- a/src/bin/pg_basebackup/pg_createsubscriber.c
+++ b/src/bin/pg_basebackup/pg_createsubscriber.c
@@ -32,7 +32,6 @@
 typedef struct CreateSubscriberOptions
 {
 	char	   *subscriber_dir; 		/* standby/subscriber data directory */
-	char	   *pub_conninfo_str;		/* publisher connection string */
 	char	   *sub_conninfo_str;		/* subscriber connection string */
 	SimpleStringList database_names;	/* list of database names */
 	bool		retain;					/* retain log file? */
@@ -60,6 +59,8 @@ static char *get_base_conninfo(char *conninfo, char **dbname);
 static char *get_bin_directory(const char *path);
 static bool check_data_directory(const char *datadir);
 static char *concat_conninfo_dbname(const char *conninfo, const char *dbname);
+static char *get_primary_conninfo_from_target(const char *base_conninfo,
+											  const char *dbname);
 static LogicalRepInfo *store_pub_sub_info(SimpleStringList dbnames,
 										  const char *pub_base_conninfo,
 										  const char *sub_base_conninfo);
@@ -168,7 +169,6 @@ usage(void)
 	printf(_("  %s [OPTION]...\n"), progname);
 	printf(_("\nOptions:\n"));
 	printf(_(" -D, --pgdata=DATADIR                location for the subscriber data directory\n"));
-	printf(_(" -P, --publisher-server=CONNSTR      publisher connection string\n"));
 	printf(_(" -S, --subscriber-server=CONNSTR     subscriber connection string\n"));
 	printf(_(" -d, --database=DBNAME               database to create a subscription\n"));
 	printf(_(" -n, --dry-run                       check clusters only, don't change target server\n"));
@@ -404,6 +404,57 @@ disconnect_database(PGconn *conn)
 	PQfinish(conn);
 }
 
+/*
+ * Obtain primary_conninfo from the target server. The value would be used for
+ * connecting from the pg_createsubscriber itself and logical replication apply
+ * worker.
+ */
+static char *
+get_primary_conninfo_from_target(const char *base_conninfo, const char *dbname)
+{
+	PGconn	   *conn;
+	PGresult   *res;
+	char	   *conninfo;
+	char	   *primaryconninfo;
+
+	pg_log_info("getting primary_conninfo from standby");
+
+	/*
+	 * Construct a connection string to the target instance. Since dbinfo has
+	 * not stored infomation yet, the name must be passed as an argument.
+	 */
+	conninfo = concat_conninfo_dbname(base_conninfo, dbname);
+
+	conn = connect_database(conninfo);
+	if (conn == NULL)
+		exit(1);
+
+	res = PQexec(conn, "SHOW primary_conninfo;");
+	if (PQresultStatus(res) != PGRES_TUPLES_OK)
+	{
+		pg_log_error("could not send command \"%s\": %s",
+					 "SHOW primary_conninfo;", PQresultErrorMessage(res));
+		PQclear(res);
+		disconnect_database(conn);
+		exit(1);
+	}
+
+	primaryconninfo = pg_strdup(PQgetvalue(res, 0, 0));
+
+	if (strlen(primaryconninfo) == 0)
+	{
+		pg_log_error("primary_conninfo was empty");
+		pg_log_error_hint("Check whether the target server is really a standby.");
+		exit(1);
+	}
+
+	pg_free(conninfo);
+	PQclear(res);
+	disconnect_database(conn);
+
+	return primaryconninfo;
+}
+
 /*
  * Obtain the system identifier using the provided connection. It will be used
  * to compare if a data directory is a clone of another one.
@@ -1358,17 +1409,20 @@ create_subscription(PGconn *conn, LogicalRepInfo *dbinfo)
 {
 	PQExpBuffer str = createPQExpBuffer();
 	PGresult   *res;
+	char	   *conninfo;
 
 	Assert(conn != NULL);
 
 	pg_log_info("creating subscription \"%s\" on database \"%s\"",
 				dbinfo->subname, dbinfo->dbname);
 
+	conninfo = escape_single_quotes_ascii(dbinfo->pubconninfo);
+
 	appendPQExpBuffer(str,
 					  "CREATE SUBSCRIPTION %s CONNECTION '%s' PUBLICATION %s "
 					  "WITH (create_slot = false, copy_data = false, "
 					  "      enabled = false)",
-					  dbinfo->subname, dbinfo->pubconninfo, dbinfo->pubname);
+					  dbinfo->subname, conninfo, dbinfo->pubname);
 
 	pg_log_debug("command is: %s", str->data);
 
@@ -1389,6 +1443,7 @@ create_subscription(PGconn *conn, LogicalRepInfo *dbinfo)
 	if (!dry_run)
 		PQclear(res);
 
+	pg_free(conninfo);
 	destroyPQExpBuffer(str);
 }
 
@@ -1562,7 +1617,6 @@ main(int argc, char **argv)
 		{"help", no_argument, NULL, '?'},
 		{"version", no_argument, NULL, 'V'},
 		{"pgdata", required_argument, NULL, 'D'},
-		{"publisher-server", required_argument, NULL, 'P'},
 		{"subscriber-server", required_argument, NULL, 'S'},
 		{"database", required_argument, NULL, 'd'},
 		{"dry-run", no_argument, NULL, 'n'},
@@ -1618,7 +1672,6 @@ main(int argc, char **argv)
 
 	/* Default settings */
 	opt.subscriber_dir = NULL;
-	opt.pub_conninfo_str = NULL;
 	opt.sub_conninfo_str = NULL;
 	opt.database_names = (SimpleStringList)
 	{
@@ -1643,7 +1696,7 @@ main(int argc, char **argv)
 
 	get_restricted_token();
 
-	while ((c = getopt_long(argc, argv, "D:P:S:d:nrt:v",
+	while ((c = getopt_long(argc, argv, "D:S:d:nrt:v",
 							long_options, &option_index)) != -1)
 	{
 		switch (c)
@@ -1651,9 +1704,6 @@ main(int argc, char **argv)
 			case 'D':
 				opt.subscriber_dir = pg_strdup(optarg);
 				break;
-			case 'P':
-				opt.pub_conninfo_str = pg_strdup(optarg);
-				break;
 			case 'S':
 				opt.sub_conninfo_str = pg_strdup(optarg);
 				break;
@@ -1706,28 +1756,6 @@ main(int argc, char **argv)
 		exit(1);
 	}
 
-	/*
-	 * Parse connection string. Build a base connection string that might be
-	 * reused by multiple databases.
-	 */
-	if (opt.pub_conninfo_str == NULL)
-	{
-		/*
-		 * TODO use primary_conninfo (if available) from subscriber and
-		 * extract publisher connection string. Assume that there are
-		 * identical entries for physical and logical replication. If there is
-		 * not, we would fail anyway.
-		 */
-		pg_log_error("no publisher connection string specified");
-		pg_log_error_hint("Try \"%s --help\" for more information.", progname);
-		exit(1);
-	}
-	pg_log_info("validating connection string on publisher");
-	pub_base_conninfo = get_base_conninfo(opt.pub_conninfo_str,
-										  &dbname_conninfo);
-	if (pub_base_conninfo == NULL)
-		exit(1);
-
 	if (opt.sub_conninfo_str == NULL)
 	{
 		pg_log_error("no subscriber connection string specified");
@@ -1735,7 +1763,7 @@ main(int argc, char **argv)
 		exit(1);
 	}
 	pg_log_info("validating connection string on subscriber");
-	sub_base_conninfo = get_base_conninfo(opt.sub_conninfo_str, NULL);
+	sub_base_conninfo = get_base_conninfo(opt.sub_conninfo_str, &dbname_conninfo);
 	if (sub_base_conninfo == NULL)
 		exit(1);
 
@@ -1745,7 +1773,7 @@ main(int argc, char **argv)
 
 		/*
 		 * If --database option is not provided, try to obtain the dbname from
-		 * the publisher conninfo. If dbname parameter is not available, error
+		 * the subscriber conninfo. If dbname parameter is not available, error
 		 * out.
 		 */
 		if (dbname_conninfo)
@@ -1753,7 +1781,7 @@ main(int argc, char **argv)
 			simple_string_list_append(&opt.database_names, dbname_conninfo);
 			num_dbs++;
 
-			pg_log_info("database \"%s\" was extracted from the publisher connection string",
+			pg_log_info("database \"%s\" was extracted from the subscriber connection string",
 						dbname_conninfo);
 		}
 		else
@@ -1765,6 +1793,11 @@ main(int argc, char **argv)
 		}
 	}
 
+	/* Obtain a connection string from the target */
+	pub_base_conninfo =
+				get_primary_conninfo_from_target(sub_base_conninfo,
+												 opt.database_names.head->val);
+
 	/* Get the absolute path of pg_ctl and pg_resetwal on the subscriber */
 	pg_bin_dir = get_bin_directory(argv[0]);
 
diff --git a/src/bin/pg_basebackup/t/040_pg_createsubscriber.pl b/src/bin/pg_basebackup/t/040_pg_createsubscriber.pl
index 95eb4e70ac..da8250d1b7 100644
--- a/src/bin/pg_basebackup/t/040_pg_createsubscriber.pl
+++ b/src/bin/pg_basebackup/t/040_pg_createsubscriber.pl
@@ -17,21 +17,12 @@ my $datadir = PostgreSQL::Test::Utils::tempdir;
 
 command_fails(['pg_createsubscriber'],
 	'no subscriber data directory specified');
-command_fails(
-	[ 'pg_createsubscriber', '--pgdata', $datadir ],
-	'no publisher connection string specified');
-command_fails(
-	[
-		'pg_createsubscriber', '--dry-run',
-		'--pgdata', $datadir,
-		'--publisher-server', 'dbname=postgres'
-	],
+command_fails([ 'pg_createsubscriber', '--dry-run', '--pgdata', $datadir, ],
 	'no subscriber connection string specified');
 command_fails(
 	[
 		'pg_createsubscriber', '--verbose',
 		'--pgdata', $datadir,
-		'--publisher-server', 'dbname=postgres',
 		'--subscriber-server', 'dbname=postgres'
 	],
 	'no database name specified');
diff --git a/src/bin/pg_basebackup/t/041_pg_createsubscriber_standby.pl b/src/bin/pg_basebackup/t/041_pg_createsubscriber_standby.pl
index d7567ef8e9..6b68276ce3 100644
--- a/src/bin/pg_basebackup/t/041_pg_createsubscriber_standby.pl
+++ b/src/bin/pg_basebackup/t/041_pg_createsubscriber_standby.pl
@@ -59,12 +59,11 @@ command_fails(
 	[
 		'pg_createsubscriber', '--verbose',
 		'--pgdata', $node_f->data_dir,
-		'--publisher-server', $node_p->connstr('pg1'),
 		'--subscriber-server', $node_f->connstr('pg1'),
 		'--database', 'pg1',
 		'--database', 'pg2'
 	],
-	'subscriber data directory is not a copy of the source database cluster');
+	'target database is not a physical standby');
 
 # Run pg_createsubscriber on the stopped node
 command_fails(
@@ -90,8 +89,7 @@ command_ok(
 	[
 		'pg_createsubscriber', '--verbose',
 		'--dry-run', '--pgdata',
-		$node_s->data_dir, '--publisher-server',
-		$node_p->connstr('pg1'), '--subscriber-server',
+		$node_s->data_dir, '--subscriber-server',
 		$node_s->connstr('pg1'), '--database',
 		'pg1', '--database',
 		'pg2'
@@ -107,8 +105,7 @@ command_ok(
 	[
 		'pg_createsubscriber', '--verbose',
 		'--dry-run', '--pgdata',
-		$node_s->data_dir, '--publisher-server',
-		$node_p->connstr('pg1'), '--subscriber-server',
+		$node_s->data_dir, '--subscriber-server',
 		$node_s->connstr('pg1')
 	],
 	'run pg_createsubscriber without --databases');
@@ -118,7 +115,6 @@ command_ok(
 	[
 		'pg_createsubscriber', '--verbose',
 		'--pgdata', $node_s->data_dir,
-		'--publisher-server', $node_p->connstr('pg1'),
 		'--subscriber-server', $node_s->connstr('pg1'),
 		'--database', 'pg1',
 		'--database', 'pg2'
-- 
2.43.0