From 9106f71fe1fedc8c3b73e7df3e80343fbb2dc7b6 Mon Sep 17 00:00:00 2001
From: Hayato Kuroda <kuroda.hayato@fujitsu.com>
Date: Wed, 31 Jan 2024 09:20:54 +0000
Subject: [PATCH v13 4/5] 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   | 105 ++++++++++++------
 .../t/040_pg_createsubscriber.pl              |   8 --
 .../t/041_pg_createsubscriber_standby.pl      |   5 +-
 4 files changed, 71 insertions(+), 64 deletions(-)

diff --git a/doc/src/sgml/ref/pg_createsubscriber.sgml b/doc/src/sgml/ref/pg_createsubscriber.sgml
index 53b42e6161..0abe1f6f28 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>
@@ -83,16 +78,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>
@@ -304,7 +289,7 @@ PostgreSQL documentation
    To create a logical replica for databases <literal>hr</literal> and
    <literal>finance</literal> from a physical replica at <literal>foo</literal>:
 <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 9c16533458..02291ba505 100644
--- a/src/bin/pg_basebackup/pg_createsubscriber.c
+++ b/src/bin/pg_basebackup/pg_createsubscriber.c
@@ -28,6 +28,7 @@
 #include "fe_utils/recovery_gen.h"
 #include "fe_utils/simple_list.h"
 #include "getopt_long.h"
+#include "port.h"
 #include "utils/pidfile.h"
 
 #define	PGS_OUTPUT_DIR	"pg_createsubscriber_output.d"
@@ -51,10 +52,10 @@ typedef struct LogicalRepInfo
 
 static void cleanup_objects_atexit(void);
 static void usage();
-static char *get_base_conninfo(char *conninfo, char *dbname,
-							   const char *noderole);
+static char *get_base_conninfo(char *conninfo, char *dbname);
 static bool get_exec_path(const char *path);
 static bool check_data_directory(const char *datadir);
+static char *get_primary_conninfo(const char *base_conninfo);
 static char *concat_conninfo_dbname(const char *conninfo, const char *dbname);
 static LogicalRepInfo *store_pub_sub_info(const char *pub_base_conninfo, const char *sub_base_conninfo);
 static PGconn *connect_database(const char *conninfo);
@@ -88,7 +89,6 @@ static void enable_subscription(PGconn *conn, LogicalRepInfo *dbinfo);
 static const char *progname;
 
 static char *subscriber_dir = NULL;
-static char *pub_conninfo_str = NULL;
 static char *sub_conninfo_str = NULL;
 static SimpleStringList database_names = {NULL, NULL};
 static char *primary_slot_name = NULL;
@@ -167,7 +167,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                       stop before modifying anything\n"));
@@ -192,7 +191,7 @@ usage(void)
  * dbname.
  */
 static char *
-get_base_conninfo(char *conninfo, char *dbname, const char *noderole)
+get_base_conninfo(char *conninfo, char *dbname)
 {
 	PQExpBuffer buf = createPQExpBuffer();
 	PQconninfoOption *conn_opts = NULL;
@@ -201,7 +200,7 @@ get_base_conninfo(char *conninfo, char *dbname, const char *noderole)
 	char	   *ret;
 	int			i;
 
-	pg_log_info("validating connection string on %s", noderole);
+	pg_log_info("validating connection string on subscriber");
 
 	conn_opts = PQconninfoParse(conninfo, &errmsg);
 	if (conn_opts == NULL)
@@ -425,6 +424,58 @@ 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(const char *base_conninfo)
+{
+	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, we must directly get the first element of the
+	 * database list.
+	 */
+	conninfo = concat_conninfo_dbname(base_conninfo, database_names.head->val);
+
+	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 is empty");
+		pg_log_error_hint("Check whether the target server is really a physical 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.
@@ -1256,15 +1307,18 @@ 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);
 
@@ -1286,6 +1340,7 @@ create_subscription(PGconn *conn, LogicalRepInfo *dbinfo)
 	if (!dry_run)
 		PQclear(res);
 
+	pg_free(conninfo);
 	destroyPQExpBuffer(str);
 }
 
@@ -1452,7 +1507,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'},
@@ -1519,7 +1573,7 @@ main(int argc, char **argv)
 	}
 #endif
 
-	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)
@@ -1527,9 +1581,6 @@ main(int argc, char **argv)
 			case 'D':
 				subscriber_dir = pg_strdup(optarg);
 				break;
-			case 'P':
-				pub_conninfo_str = pg_strdup(optarg);
-				break;
 			case 'S':
 				sub_conninfo_str = pg_strdup(optarg);
 				break;
@@ -1581,34 +1632,13 @@ main(int argc, char **argv)
 		exit(1);
 	}
 
-	/*
-	 * Parse connection string. Build a base connection string that might be
-	 * reused by multiple databases.
-	 */
-	if (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);
-	}
-	pub_base_conninfo = get_base_conninfo(pub_conninfo_str, dbname_conninfo,
-										  "publisher");
-	if (pub_base_conninfo == NULL)
-		exit(1);
-
 	if (sub_conninfo_str == NULL)
 	{
 		pg_log_error("no subscriber connection string specified");
 		pg_log_error_hint("Try \"%s --help\" for more information.", progname);
 		exit(1);
 	}
-	sub_base_conninfo = get_base_conninfo(sub_conninfo_str, NULL, "subscriber");
+	sub_base_conninfo = get_base_conninfo(sub_conninfo_str, dbname_conninfo);
 	if (sub_base_conninfo == NULL)
 		exit(1);
 
@@ -1618,7 +1648,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)
@@ -1626,7 +1656,7 @@ main(int argc, char **argv)
 			simple_string_list_append(&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
@@ -1637,6 +1667,9 @@ main(int argc, char **argv)
 		}
 	}
 
+	/* Obtain a connection string from the target */
+	pub_base_conninfo = get_primary_conninfo(sub_base_conninfo);
+
 	/*
 	 * Get the absolute path of pg_ctl and pg_resetwal on the subscriber.
 	 */
diff --git a/src/bin/pg_basebackup/t/040_pg_createsubscriber.pl b/src/bin/pg_basebackup/t/040_pg_createsubscriber.pl
index 0f02b1bfac..5c240a5417 100644
--- a/src/bin/pg_basebackup/t/040_pg_createsubscriber.pl
+++ b/src/bin/pg_basebackup/t/040_pg_createsubscriber.pl
@@ -17,18 +17,11 @@ 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'
 	],
 	'no subscriber connection string specified');
 command_fails(
@@ -36,7 +29,6 @@ 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 534bc53a76..a9d03acc87 100644
--- a/src/bin/pg_basebackup/t/041_pg_createsubscriber_standby.pl
+++ b/src/bin/pg_basebackup/t/041_pg_createsubscriber_standby.pl
@@ -56,19 +56,17 @@ 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');
 
 # dry run mode on node S
 command_ok(
 	[
 		'pg_createsubscriber', '--verbose', '--dry-run',
 		'--pgdata', $node_s->data_dir,
-		'--publisher-server', $node_p->connstr('pg1'),
 		'--subscriber-server', $node_s->connstr('pg1'),
 		'--database', 'pg1',
 		'--database', 'pg2'
@@ -88,7 +86,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

