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

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;

Regards,
Vignesh