From fe7fb7a2144e3e70b318cdfefa4d181b40f107d6 Mon Sep 17 00:00:00 2001 From: Hayato Kuroda Date: Fri, 2 Feb 2024 09:31:44 +0000 Subject: [PATCH v18 5/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 | 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 f5238771b7..2ff31628ce 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 @@ -82,16 +77,6 @@ PostgreSQL documentation - - - - - - The connection string to the publisher. For details see . - - - - @@ -303,7 +288,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 09e746b85b..9549b889a8 100644 --- a/src/bin/pg_basebackup/pg_createsubscriber.c +++ b/src/bin/pg_basebackup/pg_createsubscriber.c @@ -38,7 +38,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? */ @@ -66,6 +65,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); @@ -178,7 +179,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")); @@ -415,6 +415,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); } @@ -1559,7 +1614,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'}, @@ -1615,7 +1669,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) { @@ -1640,7 +1693,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) @@ -1648,9 +1701,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; @@ -1703,28 +1753,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"); @@ -1732,7 +1760,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); @@ -1742,7 +1770,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) @@ -1750,7 +1778,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 @@ -1762,6 +1790,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