From 1315038f73179760000137cd34575645dc8fe86a Mon Sep 17 00:00:00 2001 From: Hayato Kuroda Date: Wed, 31 Jan 2024 09:20:54 +0000 Subject: [PATCH v12 3/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 | 98 ++++++++++++------- .../t/040_pg_createsubscriber.pl | 8 -- .../t/041_pg_createsubscriber_standby.pl | 5 +- 4 files changed, 65 insertions(+), 63 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 datadir - - - - - connstr @@ -83,16 +78,6 @@ PostgreSQL documentation - - - - - - The connection string to the publisher. For details see . - - - - @@ -304,7 +289,7 @@ PostgreSQL documentation To create a logical replica for databases hr and finance from a physical replica at foo: -$ pg_createsubscriber -D /usr/local/pgsql/data -P "host=foo" -S "host=localhost" -d hr -d finance +$ pg_createsubscriber -D /usr/local/pgsql/data -S "host=localhost" -d hr -d finance diff --git a/src/bin/pg_basebackup/pg_createsubscriber.c b/src/bin/pg_basebackup/pg_createsubscriber.c index 47970b10d6..f0e9db7793 100644 --- a/src/bin/pg_basebackup/pg_createsubscriber.c +++ b/src/bin/pg_basebackup/pg_createsubscriber.c @@ -51,10 +51,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 +88,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 +166,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 +190,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 +199,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 +423,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. @@ -1452,7 +1502,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 +1568,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 +1576,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 +1627,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 +1643,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 +1651,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 +1662,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