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

On Wed, Dec 11, 2024 at 10:59 AM vignesh C <vignesh21(at)gmail(dot)com> wrote:
>
> On Tue, 10 Dec 2024 at 17:17, Shubham Khanna
> <khannashubham1197(at)gmail(dot)com> wrote:
> >
> > On Mon, Dec 9, 2024 at 7:43 PM vignesh C <vignesh21(at)gmail(dot)com> wrote:
> >
> > The attached Patch contains the required changes.
>
> Few comments:
> 1) This is not correct, currently enabling two_phase option using
> alter subscription is supported:
> Notably, the replication for prepared transactions functions regardless of the
> initial two-phase setting on the replication slot. However, the user cannot
> change the setting after the subscription is created unless a future command,
> such as 'ALTER SUBSCRIPTION ... SET (two_phase = on)', is supported.
> This provides flexibility for future enhancements.
>
> 2) You can enable-two-phase on a non dry-run mode test case, as the
> verification will not be possible in dry-run mode:
> # pg_createsubscriber can run without --databases option
> @@ -352,7 +355,7 @@ command_ok(
> $node_p->connstr($db1), '--socketdir',
> $node_s->host, '--subscriber-port',
> $node_s->port, '--replication-slot',
> - 'replslot1'
> + 'replslot1', '--enable-two-phase'
>
> 3) I felt this line can be removed "Two-phase commit ensures atomicity
> in logical replication for prepared transactions." as this information
> is available at prepare transaction and create subscription page
> documentation:
> + <literal>off</literal>.
> + Two-phase commit ensures atomicity in logical replication for prepared
> + transactions. See the
> + <link linkend="sql-createsubscription-params-with-two-phase"><literal>two_phase</literal></link>
> + documentation for more details.
>
> 4) This change is not required as it is not needed for the patch:
> dbinfo[i].made_replslot = false;
> dbinfo[i].made_publication = false;
> +
> /* Fill subscriber attributes */
> conninfo = concat_conninfo_dbname(sub_base_conninfo, cell->val);
>
> 5) Similarly here too:
> @@ -341,6 +343,7 @@ command_ok(
> $node_s->start;
> is($node_s->safe_psql('postgres', 'SELECT pg_catalog.pg_is_in_recovery()'),
> 't', 'standby is in recovery');
> +
> $node_s->stop;
>

I have fixed the given comments. The v3 version patch attached at [1]
has the changes for the same.
[1] - https://www.postgresql.org/message-id/CAHv8RjJ8NuHLQUhkqE-fy5k%2B3nZUdX9QngrLaa7iS0TEJNicow%40mail.gmail.com

Thanks and Regards,
Shubham Khanna.