Dear Vignesh,
Since the original author seems bit busy, I updated the patch set.
> 1) Cleanup function handler flag should be reset, i.e.
> dbinfo->made_replslot = false; should be there else there will be an
> error during drop replication slot cleanup in error flow:
> +static void
> +drop_replication_slot(PGconn *conn, LogicalRepInfo *dbinfo, const
> char *slot_name)
> +{
> + PQExpBuffer str = createPQExpBuffer();
> + PGresult *res;
> +
> + Assert(conn != NULL);
> +
> + pg_log_info("dropping the replication slot \"%s\" on database
> \"%s\"", slot_name, dbinfo->dbname);
> +
> + appendPQExpBuffer(str, "SELECT
> pg_drop_replication_slot('%s')", slot_name);
> +
> + pg_log_debug("command is: %s", str->data);
Fixed.
> 2) Cleanup function handler flag should be reset, i.e.
> dbinfo->made_publication = false; should be there else there will be
> an error during drop publication cleanup in error flow:
> +/*
> + * Remove publication if it couldn't finish all steps.
> + */
> +static void
> +drop_publication(PGconn *conn, LogicalRepInfo *dbinfo)
> +{
> + PQExpBuffer str = createPQExpBuffer();
> + PGresult *res;
> +
> + Assert(conn != NULL);
> +
> + pg_log_info("dropping publication \"%s\" on database \"%s\"",
> dbinfo->pubname, dbinfo->dbname);
> +
> + appendPQExpBuffer(str, "DROP PUBLICATION %s", dbinfo->pubname);
> +
> + pg_log_debug("command is: %s", str->data);
>
> 3) Cleanup function handler flag should be reset, i.e.
> dbinfo->made_subscription = false; should be there else there will be
> an error during drop publication cleanup in error flow:
> +/*
> + * Remove subscription if it couldn't finish all steps.
> + */
> +static void
> +drop_subscription(PGconn *conn, LogicalRepInfo *dbinfo)
> +{
> + PQExpBuffer str = createPQExpBuffer();
> + PGresult *res;
> +
> + Assert(conn != NULL);
> +
> + pg_log_info("dropping subscription \"%s\" on database \"%s\"",
> dbinfo->subname, dbinfo->dbname);
> +
> + appendPQExpBuffer(str, "DROP SUBSCRIPTION %s",
> dbinfo->subname);
> +
> + pg_log_debug("command is: %s", str->data);
Fixed.
> 4) I was not sure if drop_publication is required here, as we will not
> create any publication in subscriber node:
> + if (dbinfo[i].made_subscription)
> + {
> + conn = connect_database(dbinfo[i].subconninfo);
> + if (conn != NULL)
> + {
> + drop_subscription(conn, &dbinfo[i]);
> + if (dbinfo[i].made_publication)
> + drop_publication(conn, &dbinfo[i]);
> + disconnect_database(conn);
> + }
> + }
Removed. But I'm not sure the cleanup is really meaningful.
See [1].
> 5) The connection should be disconnected in case of error case:
> + /* secure search_path */
> + res = PQexec(conn, ALWAYS_SECURE_SEARCH_PATH_SQL);
> + if (PQresultStatus(res) != PGRES_TUPLES_OK)
> + {
> + pg_log_error("could not clear search_path: %s",
> PQresultErrorMessage(res));
> + return NULL;
> + }
> + PQclear(res);
PQfisnih() was added.
> 6) There should be a line break before postgres_fe inclusion, to keep
> it consistent:
> + *-------------------------------------------------------------------------
> + */
> +#include "postgres_fe.h"
> +
> +#include <signal.h>
Added.
> 7) These includes are not required:
> 7.a) #include <signal.h>
> 7.b) #include <sys/stat.h>
> 7.c) #include <sys/wait.h>
> 7.d) #include "access/xlogdefs.h"
> 7.e) #include "catalog/pg_control.h"
> 7.f) #include "common/file_utils.h"
> 7.g) #include "utils/pidfile.h"
Removed.
> + * src/bin/pg_basebackup/pg_createsubscriber.c
> + *
> + *-------------------------------------------------------------------------
> + */
> +#include "postgres_fe.h"
> +
> +#include <signal.h>
> +#include <sys/stat.h>
> +#include <sys/time.h>
> +#include <sys/wait.h>
> +#include <time.h>
> +
> +#include "access/xlogdefs.h"
> +#include "catalog/pg_authid_d.h"
> +#include "catalog/pg_control.h"
> +#include "common/connect.h"
> +#include "common/controldata_utils.h"
> +#include "common/file_perm.h"
> +#include "common/file_utils.h"
> +#include "common/logging.h"
> +#include "common/restricted_token.h"
> +#include "fe_utils/recovery_gen.h"
> +#include "fe_utils/simple_list.h"
> +#include "getopt_long.h"
> +#include "utils/pidfile.h"
>
> 8) POSTMASTER_STANDBY and POSTMASTER_FAILED are not being used, is it
> required or kept for future purpose:
> +enum WaitPMResult
> +{
> + POSTMASTER_READY,
> + POSTMASTER_STANDBY,
> + POSTMASTER_STILL_STARTING,
> + POSTMASTER_FAILED
> +};
I think they are here because WaitPMResult is just ported from pg_ctl.c.
I renamed the enumeration and removed non-necessary attributes.
> 9) pg_createsubscriber should be kept after pg_basebackup to maintain
> the consistent order:
> diff --git a/src/bin/pg_basebackup/.gitignore
> b/src/bin/pg_basebackup/.gitignore
> index 26048bdbd8..b3a6f5a2fe 100644
> --- a/src/bin/pg_basebackup/.gitignore
> +++ b/src/bin/pg_basebackup/.gitignore
> @@ -1,5 +1,6 @@
> /pg_basebackup
> /pg_receivewal
> /pg_recvlogical
> +/pg_createsubscriber
Addressed.
> 10) dry-run help message is not very clear, how about something
> similar to pg_upgrade's message like "check clusters only, don't
> change any data":
> + printf(_(" -d, --database=DBNAME database to
> create a subscription\n"));
> + printf(_(" -n, --dry-run stop before
> modifying anything\n"));
> + printf(_(" -t, --recovery-timeout=SECS seconds to wait
> for recovery to end\n"));
> + printf(_(" -r, --retain retain log file
> after success\n"));
> + printf(_(" -v, --verbose output verbose
> messages\n"));
> + printf(_(" -V, --version output version
> information, then exit\n"));
Changed.
New patch is available in [2].
[1]:
https://www.postgresql.org/message-id/TYCPR01MB1207713BEC5C379A05D65E342F54B2%40TYCPR01MB12077.jpnprd01.prod.outlook.com
[2]:
https://www.postgresql.org/message-id/TYCPR01MB12077A6BB424A025F04A8243DF54F2%40TYCPR01MB12077.jpnprd01.prod.outlook.com
Best Regards,
Hayato Kuroda
FUJITSU LIMITED
https://www.fujitsu.com/
