Re: Adding a '--two-phase' option to 'pg_createsubscriber' utility.

On Mon, Feb 24, 2025 at 10:57 AM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> On Tue, Feb 18, 2025 at 12:16 AM Shubham Khanna
> <khannashubham1197(at)gmail(dot)com> wrote:
> >
>
> -static void check_publisher(const struct LogicalRepInfo *dbinfo);
> -static char *setup_publisher(struct LogicalRepInfo *dbinfo);
> +static void check_publisher(const struct LogicalRepInfo *dbinfo, bool
> two_phase);
> +static char *setup_publisher(struct LogicalRepInfo *dbinfo, bool two_phase);
> static void check_subscriber(const struct LogicalRepInfo *dbinfo);
> static void setup_subscriber(struct LogicalRepInfo *dbinfo,
> - const char *consistent_lsn);
> + const char *consistent_lsn, bool two_phase);
> static void setup_recovery(const struct LogicalRepInfo *dbinfo, const
> char *datadir,
> const char *lsn);
> static void drop_primary_replication_slot(struct LogicalRepInfo *dbinfo,
> const char *slotname);
> static void drop_failover_replication_slots(struct LogicalRepInfo *dbinfo);
> static char *create_logical_replication_slot(PGconn *conn,
> - struct LogicalRepInfo *dbinfo);
> + struct LogicalRepInfo *dbinfo,
> + bool two_phase);
> static void drop_replication_slot(PGconn *conn, struct LogicalRepInfo *dbinfo,
> const char *slot_name);
> static void pg_ctl_status(const char *pg_ctl_cmd, int rc);
> @@ -98,7 +100,9 @@ static void wait_for_end_recovery(const char *conninfo,
> const struct CreateSubscriberOptions *opt);
> static void create_publication(PGconn *conn, struct LogicalRepInfo *dbinfo);
> static void drop_publication(PGconn *conn, struct LogicalRepInfo *dbinfo);
> -static void create_subscription(PGconn *conn, const struct
> LogicalRepInfo *dbinfo);
> +static void create_subscription(PGconn *conn,
> + const struct LogicalRepInfo *dbinfo,
> + bool two_phase);
>
> Specifying two_phase as a separate parameter in so many APIs looks
> odd. Wouldn't it be better to make it part of LogicalRepInfo as we do
> with other parameters of CreateSubscriberOptions?
>

I agree that passing two_phase as a separate parameter across multiple
function calls adds redundancy. It would be more consistent and
maintainable to include two_phase as part of LogicalRepInfo, similar
to how other options from CreateSubscriberOptions are handled.
I have created a new structure LogicalRepInfos to include a bool
two_phase field. This way, all functions that require two_phase can
access it directly from the LogicalRepInfos structure, reducing
unnecessary argument passing.

The attached patch contains the suggested changes.

Thanks and regards,
Shubham Khanna.