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

Hi all,

I am writing to propose the addition of the two_phase option in
pg_createsubscriber. As discussed in [1], supporting this feature
during the development of pg_createsubscriber was planned for this
version.
Currently, pg_createsubscriber creates subscriptions with the
two_phase option set to false. Enabling the two_phase option after a
subscription has been created is not straightforward, as it requires
the subscription to be disabled first. This patch aims to address this
limitation by incorporating the two_phase option into
pg_createsubscriber which will help create subscription with two_phase
option and make use of the advantages of creating subscription with
two_phase option.
The attached patch has the changes for the same.

[1] - https://www.postgresql.org/message-id/CAA4eK1%2B7gRxiK3xNEH03CY3mMKxqi7BVz2k3VxxVZp8fgjHZxg%40mail.gmail.com

Thanks and regards,
Shubham Khanna.

On Mon, 9 Dec 2024 at 16:25, Shubham Khanna <khannashubham1197(at)gmail(dot)com> wrote:
>
> Hi all,
>
> I am writing to propose the addition of the two_phase option in
> pg_createsubscriber. As discussed in [1], supporting this feature
> during the development of pg_createsubscriber was planned for this
> version.

Yes this will be useful.

> Currently, pg_createsubscriber creates subscriptions with the
> two_phase option set to false. Enabling the two_phase option after a
> subscription has been created is not straightforward, as it requires
> the subscription to be disabled first. This patch aims to address this
> limitation by incorporating the two_phase option into
> pg_createsubscriber which will help create subscription with two_phase
> option and make use of the advantages of creating subscription with
> two_phase option.
> The attached patch has the changes for the same.

Few comments:
1) You can keep it with no argument similar to how dry-run is handled:
@@ -1872,6 +1881,7 @@ main(int argc, char **argv)
{"publisher-server", required_argument, NULL, 'P'},
{"socketdir", required_argument, NULL, 's'},
{"recovery-timeout", required_argument, NULL, 't'},
+ {"two_phase", optional_argument, NULL, 'T'},
{"subscriber-username", required_argument, NULL, 'U'},
{"verbose", no_argument, NULL, 'v'},
{"version", no_argument, NULL, 'V'},

2) After making it to no argument option, if this option is set, just
set the value, strcmp's are not required:
+ case 'T':
+ if (optarg != NULL)
+ {
+ if (strcmp(optarg, "on") == 0)
+ opt.two_phase = true;
+ else if (strcmp(optarg, "off") == 0)
+ opt.two_phase = false;
+ else
+ pg_fatal("invalid
value for --two-phase: must be 'on' or 'off'");
+ }
+ else
+ opt.two_phase = true; /*
Default to true if no argument
+
* is provided */
+ break;

3) You can add a link to create subscription documentation page of
two_phase option here:
+ Enables or disables two-phase commit for the subscription.
+ When the option is provided without a value, it defaults to
+ <literal>on</literal>. Specify <literal>on</literal> to enable or
+ <literal>off</literal> to disable.
+ Two-phase commit ensures atomicity in logical replication for prepared
+ transactions. By default, this option is enabled unless explicitly set
+ to <literal>off</literal>.

4) Instead of adding new tests, can we include the prepare transaction
and prepare transaction verification to the existing tests itself?
+# Set up node A as primary
+my $node_a = PostgreSQL::Test::Cluster->new('node_a');
+my $aconnstr = $node_a->connstr;
+$node_a->init(allows_streaming => 'logical');
+$node_a->append_conf(
+ 'postgresql.conf', qq[
+ autovacuum = off
+ wal_level = logical
+ max_wal_senders = 10
+ max_worker_processes = 8
+ max_prepared_transactions = 10
+]);
+
+$node_a->start;

Regards,
Vignesh

Dear Shubham,

Thanks for the proposal!

> I am writing to propose the addition of the two_phase option in
> pg_createsubscriber. As discussed in [1], supporting this feature
> during the development of pg_createsubscriber was planned for this
> version.

Yes, that was the pending item.

> The attached patch has the changes for the same.

There are API related comments.

01.
I think the two-phase should be off by default. Because default value for CREATE
SUBSCRIPTION command is off, and your patch breaks pre-existing style.

02.
API style should be changed: no need to require the argument. I think it is enough
to provide "enable-twophase" and "T" option which specify the two_phase to true.

Best regards,
Hayato Kuroda
FUJITSU LIMITED

Hi Shubham,

Here are some review comments for the patch v1-0001.

Note -- I think Kuroda-san's idea to use a new switch like
'--enable-two-phase' would eliminate lots of my review comments below
about the inconsistencies with CREATE SUBSCRIBER, and the current
confusion about defaults and argument values etc.

======
Commit message

1.
By default, two-phase commit is enabled if the option is provided without
arguments. Users can explicitly set it to 'on' or 'off' using '--two-phase=on'
or '--two-phase=off'.

~

But you don't say what is the default if the option is omitted entirely.

~~~

2.
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.

~

Typo in ALTER example with the option name /two-phase/two_phase/

~~~

3.
Documentation has been updated to reflect the new option, and test cases have
been added to validate various scenarios, including proper validation of the
'--two-phase' flag and its combinations with other options.

/flag/option/ ??

======
doc/src/sgml/ref/pg_createsubscriber.sgml

4.
+ <varlistentry>
+ <term><option>-T</option></term>
+ <term><option>--two_phase</option></term>
+ <listitem>
+ <para>
+ Enables or disables two-phase commit for the subscription.
+ When the option is provided without a value, it defaults to
+ <literal>on</literal>. Specify <literal>on</literal> to enable or
+ <literal>off</literal> to disable.
+ Two-phase commit ensures atomicity in logical replication for prepared
+ transactions. By default, this option is enabled unless explicitly set
+ to <literal>off</literal>.
+ </para>
+ </listitem>
+ </varlistentry>
+

4a.
Typo -- the option you made is 'two-phase', not 'two_phase'

~

4b.
When you say "By default, this option is enabled unless explicitly set
to off." I take that as meaning it is default *enabled* even when the
option is entirely omitted.

But, that would be contrary to the behaviour of a normal CREATE
SUBSCRIPTION 'two_phase' option, so I wonder why should
pg_createsubscriber have a different default. Is this intentional? IMO
if no switch is specified then I thought it should be the same as the
CREATE SUBSCRIPTION default (i.e. false).

~

4c.
This "-T" option should be moved to be adjacent (below) "-t" to keep
everything consistently in A-Z order.

======
src/bin/pg_basebackup/pg_createsubscriber.c

5.
bool made_replslot; /* replication slot was created */
bool made_publication; /* publication was created */
+ bool two_phase; /* two-phase option was created */

I am not sure what "option was created" even means. Cut/paste error?

Also, perhaps this field belongs with the 1st group of fields in this
struct, instead of with those made_xxx fields.

~~~

store_pub_sub_info:

6.
+ /* Store two-phase option */
+ dbinfo[i].two_phase = opt->two_phase;
+

The comment says the same as what the code is doing which seems
unhelpful/redundant. And if you rearrange the struct fields as
previously suggested, this assignment should also move.

~~~

7.
pg_log_debug("subscriber(%d): subscription: %s ; connection string: %s", i,
dbinfo[i].subname ? dbinfo[i].subname : "(auto)",
dbinfo[i].subconninfo);
+ pg_log_debug("publisher(%d): two-phase: %s", i,
+ dbinfo[i].two_phase ? "true" : "false");

"two_phase" is a subscription option. So shouldn't this added
pg_log_debug info be part of the preceding pg_log_debug about the
subscription?

~~~

main:

8.
{"recovery-timeout", required_argument, NULL, 't'},
+ {"two_phase", optional_argument, NULL, 'T'},
{"subscriber-username", required_argument, NULL, 'U'},

AFAIK this option was supposed to be 'two-phase' (dash instead of
underscore is consistent with the other pg_createsubscriber options)

~~~

9.
opt.sub_username = NULL;
+ opt.two_phase = true;

As previously mentioned this default is not the same as the CREATE
SUBSCRIPTION default for 'two_phase', and I find that inconsistency to
be confusing.

~~~

10.
+ case 'T':
+ if (optarg != NULL)
+ {
+ if (strcmp(optarg, "on") == 0)
+ opt.two_phase = true;
+ else if (strcmp(optarg, "off") == 0)
+ opt.two_phase = false;
+ else
+ pg_fatal("invalid value for --two-phase: must be 'on' or 'off'");
+ }
+ else
+ opt.two_phase = true; /* Default to true if no argument
+ * is provided */
+ break;

I wonder if users familiar with the CREATE SUBSCRIPTION 'two_phase'
might find it strange that the only values accepted here are 'on' and
'off' but now all the other boolean variants.

======
.../t/040_pg_createsubscriber.pl

11.
+# Run pg_createsubscriber on a promoted server with two_phase=on
+command_ok(
+ [
+ 'pg_createsubscriber', '--verbose',
+ '--recovery-timeout', "$PostgreSQL::Test::Utils::timeout_default",
+ '--pgdata', $node_b->data_dir,
+ '--publisher-server', $node_a->connstr($db10),
+ '--subscriber-port', $node_b->port,
+ '--database', $db10,
+ '--two_phase=on'
+ ],
+ 'created subscription with two-phase commit enabled');

Hmm. I expect this should have been specified as '--two-phase=on'
(dash instead of underscore for consistency with all other switches of
pg_createsubscriber) but previous typos (e.g. #8 above) have
compounded the confusion.

======
Kind Regards,
Peter Smith.
Fujitsu Australia

On Mon, Dec 9, 2024 at 7:43 PM vignesh C <vignesh21(at)gmail(dot)com> wrote:
>
> On Mon, 9 Dec 2024 at 16:25, Shubham Khanna <khannashubham1197(at)gmail(dot)com> wrote:
> >
> > Hi all,
> >
> > I am writing to propose the addition of the two_phase option in
> > pg_createsubscriber. As discussed in [1], supporting this feature
> > during the development of pg_createsubscriber was planned for this
> > version.
>
> Yes this will be useful.
>
> > Currently, pg_createsubscriber creates subscriptions with the
> > two_phase option set to false. Enabling the two_phase option after a
> > subscription has been created is not straightforward, as it requires
> > the subscription to be disabled first. This patch aims to address this
> > limitation by incorporating the two_phase option into
> > pg_createsubscriber which will help create subscription with two_phase
> > option and make use of the advantages of creating subscription with
> > two_phase option.
> > The attached patch has the changes for the same.
>
> Few comments:
> 1) You can keep it with no argument similar to how dry-run is handled:
> @@ -1872,6 +1881,7 @@ main(int argc, char **argv)
> {"publisher-server", required_argument, NULL, 'P'},
> {"socketdir", required_argument, NULL, 's'},
> {"recovery-timeout", required_argument, NULL, 't'},
> + {"two_phase", optional_argument, NULL, 'T'},
> {"subscriber-username", required_argument, NULL, 'U'},
> {"verbose", no_argument, NULL, 'v'},
> {"version", no_argument, NULL, 'V'},
>

Changed the argument to 'no_argument'.

> 2) After making it to no argument option, if this option is set, just
> set the value, strcmp's are not required:
> + case 'T':
> + if (optarg != NULL)
> + {
> + if (strcmp(optarg, "on") == 0)
> + opt.two_phase = true;
> + else if (strcmp(optarg, "off") == 0)
> + opt.two_phase = false;
> + else
> + pg_fatal("invalid
> value for --two-phase: must be 'on' or 'off'");
> + }
> + else
> + opt.two_phase = true; /*
> Default to true if no argument
> +
> * is provided */
> + break;
>

Updated the switch case according to the modified argument.

> 3) You can add a link to create subscription documentation page of
> two_phase option here:
> + Enables or disables two-phase commit for the subscription.
> + When the option is provided without a value, it defaults to
> + <literal>on</literal>. Specify <literal>on</literal> to enable or
> + <literal>off</literal> to disable.
> + Two-phase commit ensures atomicity in logical replication for prepared
> + transactions. By default, this option is enabled unless explicitly set
> + to <literal>off</literal>.
>

Added the link to create subscription in the documentation of
pg_createsubscriber.

> 4) Instead of adding new tests, can we include the prepare transaction
> and prepare transaction verification to the existing tests itself?
> +# Set up node A as primary
> +my $node_a = PostgreSQL::Test::Cluster->new('node_a');
> +my $aconnstr = $node_a->connstr;
> +$node_a->init(allows_streaming => 'logical');
> +$node_a->append_conf(
> + 'postgresql.conf', qq[
> + autovacuum = off
> + wal_level = logical
> + max_wal_senders = 10
> + max_worker_processes = 8
> + max_prepared_transactions = 10
> +]);
> +
> +$node_a->start;
>

Removed the new test case and added it to the existing test cases.

The attached Patch contains the required changes.

Thanks and regards,
Shubham Khanna.

On Tue, Dec 10, 2024 at 7:42 AM Hayato Kuroda (Fujitsu)
<kuroda(dot)hayato(at)fujitsu(dot)com> wrote:
>
> Dear Shubham,
>
> Thanks for the proposal!
>
> > I am writing to propose the addition of the two_phase option in
> > pg_createsubscriber. As discussed in [1], supporting this feature
> > during the development of pg_createsubscriber was planned for this
> > version.
>
> Yes, that was the pending item.
>
> > The attached patch has the changes for the same.
>
> There are API related comments.
>
> 01.
> I think the two-phase should be off by default. Because default value for CREATE
> SUBSCRIPTION command is off, and your patch breaks pre-existing style.
>
> 02.
> API style should be changed: no need to require the argument. I think it is enough
> to provide "enable-twophase" and "T" option which specify the two_phase to true.
>

I have fixed the given comments. The v2 version patch attached at [1]
has the changes for the same.
[1] - https://www.postgresql.org/message-id/CAHv8RjLcdmz%3D_RMwveuDdr8i7r%3D09TAwtOnFmXeaia_v2RmnYA%40mail.gmail.com

Thanks and Regards,
Shubham Khanna.

On Tue, Dec 10, 2024 at 8:05 AM Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
>
> Hi Shubham,
>
> Here are some review comments for the patch v1-0001.
>
> Note -- I think Kuroda-san's idea to use a new switch like
> '--enable-two-phase' would eliminate lots of my review comments below
> about the inconsistencies with CREATE SUBSCRIBER, and the current
> confusion about defaults and argument values etc.
>
> ======
> Commit message
>
> 1.
> By default, two-phase commit is enabled if the option is provided without
> arguments. Users can explicitly set it to 'on' or 'off' using '--two-phase=on'
> or '--two-phase=off'.
>
> ~
>
> But you don't say what is the default if the option is omitted entirely.
>
>
> ~~~
>
> 2.
> 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.
>
> ~
>
> Typo in ALTER example with the option name /two-phase/two_phase/
>
> ~~~
>
> 3.
> Documentation has been updated to reflect the new option, and test cases have
> been added to validate various scenarios, including proper validation of the
> '--two-phase' flag and its combinations with other options.
>
> /flag/option/ ??
>

Modified the commit message according to the suggested changes.

> ======
> doc/src/sgml/ref/pg_createsubscriber.sgml
>
> 4.
> + <varlistentry>
> + <term><option>-T</option></term>
> + <term><option>--two_phase</option></term>
> + <listitem>
> + <para>
> + Enables or disables two-phase commit for the subscription.
> + When the option is provided without a value, it defaults to
> + <literal>on</literal>. Specify <literal>on</literal> to enable or
> + <literal>off</literal> to disable.
> + Two-phase commit ensures atomicity in logical replication for prepared
> + transactions. By default, this option is enabled unless explicitly set
> + to <literal>off</literal>.
> + </para>
> + </listitem>
> + </varlistentry>
> +
>
> 4a.
> Typo -- the option you made is 'two-phase', not 'two_phase'
>
> ~

Fixed.

> 4b.
> When you say "By default, this option is enabled unless explicitly set
> to off." I take that as meaning it is default *enabled* even when the
> option is entirely omitted.
>
> But, that would be contrary to the behaviour of a normal CREATE
> SUBSCRIPTION 'two_phase' option, so I wonder why should
> pg_createsubscriber have a different default. Is this intentional? IMO
> if no switch is specified then I thought it should be the same as the
> CREATE SUBSCRIPTION default (i.e. false).
>
> ~

Modified the documentation on the basis of the latest changes added to
the patch.

> 4c.
> This "-T" option should be moved to be adjacent (below) "-t" to keep
> everything consistently in A-Z order.
>

Fixed.

> ======
> src/bin/pg_basebackup/pg_createsubscriber.c
>
> 5.
> bool made_replslot; /* replication slot was created */
> bool made_publication; /* publication was created */
> + bool two_phase; /* two-phase option was created */
>
> I am not sure what "option was created" even means. Cut/paste error?
>
> Also, perhaps this field belongs with the 1st group of fields in this
> struct, instead of with those made_xxx fields.
>
> ~~~
>
> store_pub_sub_info:
>
> 6.
> + /* Store two-phase option */
> + dbinfo[i].two_phase = opt->two_phase;
> +
>
> The comment says the same as what the code is doing which seems
> unhelpful/redundant. And if you rearrange the struct fields as
> previously suggested, this assignment should also move.
>

Fixed.

>
> 7.
> pg_log_debug("subscriber(%d): subscription: %s ; connection string: %s", i,
> dbinfo[i].subname ? dbinfo[i].subname : "(auto)",
> dbinfo[i].subconninfo);
> + pg_log_debug("publisher(%d): two-phase: %s", i,
> + dbinfo[i].two_phase ? "true" : "false");
>
> "two_phase" is a subscription option. So shouldn't this added
> pg_log_debug info be part of the preceding pg_log_debug about the
> subscription?
>

Fixed.

>
> main:
>
> 8.
> {"recovery-timeout", required_argument, NULL, 't'},
> + {"two_phase", optional_argument, NULL, 'T'},
> {"subscriber-username", required_argument, NULL, 'U'},
>
> AFAIK this option was supposed to be 'two-phase' (dash instead of
> underscore is consistent with the other pg_createsubscriber options)
>

Fixed.

>
> 9.
> opt.sub_username = NULL;
> + opt.two_phase = true;
>
> As previously mentioned this default is not the same as the CREATE
> SUBSCRIPTION default for 'two_phase', and I find that inconsistency to
> be confusing.
>

Changed the default option to false as suggested.

>
> 10.
> + case 'T':
> + if (optarg != NULL)
> + {
> + if (strcmp(optarg, "on") == 0)
> + opt.two_phase = true;
> + else if (strcmp(optarg, "off") == 0)
> + opt.two_phase = false;
> + else
> + pg_fatal("invalid value for --two-phase: must be 'on' or 'off'");
> + }
> + else
> + opt.two_phase = true; /* Default to true if no argument
> + * is provided */
> + break;
>
> I wonder if users familiar with the CREATE SUBSCRIPTION 'two_phase'
> might find it strange that the only values accepted here are 'on' and
> 'off' but now all the other boolean variants.
>

Fixed.

> ======
> .../t/040_pg_createsubscriber.pl
>
> 11.
> +# Run pg_createsubscriber on a promoted server with two_phase=on
> +command_ok(
> + [
> + 'pg_createsubscriber', '--verbose',
> + '--recovery-timeout', "$PostgreSQL::Test::Utils::timeout_default",
> + '--pgdata', $node_b->data_dir,
> + '--publisher-server', $node_a->connstr($db10),
> + '--subscriber-port', $node_b->port,
> + '--database', $db10,
> + '--two_phase=on'
> + ],
> + 'created subscription with two-phase commit enabled');
>
> Hmm. I expect this should have been specified as '--two-phase=on'
> (dash instead of underscore for consistency with all other switches of
> pg_createsubscriber) but previous typos (e.g. #8 above) have
> compounded the confusion.
>

Fixed.

The v2 version patch attached at [1] has the changes for the same.
[1] - https://www.postgresql.org/message-id/CAHv8RjLcdmz%3D_RMwveuDdr8i7r%3D09TAwtOnFmXeaia_v2RmnYA%40mail.gmail.com

Thanks and Regards,
Shubham Khanna.

Hi Shubham,

Here are some review comments for patch v2-0001.

======
GENERAL - the new option name

1.
I am not sure if this new switch needed to be called
'--enable-two-phase'; probably just calling it '--two-phase' would
mean the same because specifying the switch already implies "enabling"
it to me.

Perhaps either name is fine. What do others think?

======
Commit message

2.
This patch introduces the '--enable-two-phase' option to the
'pg_createsubscriber' utility, allowing users to enable or disable two-phase
commit for subscriptions during their creation.

~

It seems a bit strange IMO to say "enable or disable", because this is
only for "enable", and the default is disable as described in the
following sentence.

~~~

3.
By default, two-phase commit is disabled if the option is provided without
arguments.

~

But, this option now has no arguments so the part "if the option is
provided without arguments" is not applicable anymore and should be
removed. Or, if you want to say something you could say "if the option
is not provided."

======
doc/src/sgml/ref/pg_createsubscriber.sgml

4.
+ <varlistentry>
+ <term><option>-T</option></term>
+ <term><option>--enable-two-phase</option></term>
+ <listitem>
+ <para>
+ Enables two-phase commit for the subscription. When the option is
+ provided, it is explicitly enabled. By default, two-phase commit is
+ <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.
+ </para>
+ </listitem>
+ </varlistentry>
+

I felt this was more verbose than necessary. IMO you only needed to
say something very simple; the user can chase the link to learn more
about two_phase if they want to.

SUGGESTION
Enables <link linkend="sql-createsubscription-params-with-two-phase"><literal>two_phase</literal></link>
commit for the subscription. The default is <literal>false</literal>.

======
src/bin/pg_basebackup/pg_createsubscriber.c

usage:

5.
printf(_(" -t, --recovery-timeout=SECS seconds to wait for
recovery to end\n"));
+ printf(_(" -T, --enable-two-phase enable two-phase commit for the
subscription\n"));
printf(_(" -U, --subscriber-username=NAME user name for subscriber
connection\n"));

AFAICT the patch is wrongly using tabs here instead of spaces.

~~~

store_pub_sub_info:

6.
+ dbinfo[i].two_phase = opt->two_phase; /* Set two-phase option */

I still think this comment only states the obvious so it is not
helpful. Can remove it.

~~~

7.
dbinfo[i].made_publication = false;
+
/* Fill subscriber attributes */
This whitespace change is unrelated to this patch.

~~~

8.
- pg_log_debug("subscriber(%d): subscription: %s ; connection string: %s", i,
+ pg_log_debug("subscriber(%d): subscription: %s ; connection string:
%s, --enable-two-phase: %s", i,
dbinfo[i].subname ? dbinfo[i].subname : "(auto)",
- dbinfo[i].subconninfo);
+ dbinfo[i].subconninfo,
+ dbinfo[i].two_phase ? "true" : "false");

IMO say "two_phase" here, not "--enable-two-phase".

======
.../t/040_pg_createsubscriber.pl

9.
max_worker_processes = 8
+max_prepared_transactions = 10
});

AFAICT the comment for this test code said:

# Restore default settings here but only apply it after testing standby. Some
# standby settings should not be a lower setting than on the primary.

But max_prepared_transactions = 10 is not the default setting for this GUC.

~~~

10.
is($node_s->safe_psql('postgres', 'SELECT pg_catalog.pg_is_in_recovery()'),
't', 'standby is in recovery');
+
$node_s->stop;

This whitespace change has nothing to do with the patch.

~~~

11.
- 'replslot1'
+ 'replslot1', '--enable-two-phase'

The comment for this test only says
# pg_createsubscriber can run without --databases option

Now it is doing more, so maybe it should give more details like "In
passing, also test the --enable-two-phase option."

~~~

12.
+# Verify that the prepared transaction is replicated to the subscriber
+my $count_prepared_s =
+ $node_s->safe_psql($db1, "SELECT count(*) FROM pg_prepared_xacts;");
+
+is($count_prepared_s, qq(0), 'Prepared transaction replicated to subscriber');
+

Is this test OK? It says to verify it is replicated. How does checking
subscriber has zero pg_prepared_xacts ensure replication occurred?

======
Please see the attached NITPICKS diffs which includes some (not all)
of the above suggestions.

======
Kind Regards,
Peter Smith.
Fujitsu Australia

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

On Wed, Dec 11, 2024 at 4:21 AM Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
>
> Hi Shubham,
>
> Here are some review comments for patch v2-0001.
>
> ======
> GENERAL - the new option name
>
> 1.
> I am not sure if this new switch needed to be called
> '--enable-two-phase'; probably just calling it '--two-phase' would
> mean the same because specifying the switch already implies "enabling"
> it to me.
>
> Perhaps either name is fine. What do others think?
>
> ======
> Commit message
>
> 2.
> This patch introduces the '--enable-two-phase' option to the
> 'pg_createsubscriber' utility, allowing users to enable or disable two-phase
> commit for subscriptions during their creation.
>
> ~
>
> It seems a bit strange IMO to say "enable or disable", because this is
> only for "enable", and the default is disable as described in the
> following sentence.
>
> ~~~
>
> 3.
> By default, two-phase commit is disabled if the option is provided without
> arguments.
>
> ~
>
> But, this option now has no arguments so the part "if the option is
> provided without arguments" is not applicable anymore and should be
> removed. Or, if you want to say something you could say "if the option
> is not provided."
>
> ======
> doc/src/sgml/ref/pg_createsubscriber.sgml
>
> 4.
> + <varlistentry>
> + <term><option>-T</option></term>
> + <term><option>--enable-two-phase</option></term>
> + <listitem>
> + <para>
> + Enables two-phase commit for the subscription. When the option is
> + provided, it is explicitly enabled. By default, two-phase commit is
> + <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.
> + </para>
> + </listitem>
> + </varlistentry>
> +
>
> I felt this was more verbose than necessary. IMO you only needed to
> say something very simple; the user can chase the link to learn more
> about two_phase if they want to.
>
> SUGGESTION
> Enables <link linkend="sql-createsubscription-params-with-two-phase"><literal>two_phase</literal></link>
> commit for the subscription. The default is <literal>false</literal>.
>
> ======
> src/bin/pg_basebackup/pg_createsubscriber.c
>
> usage:
>
> 5.
> printf(_(" -t, --recovery-timeout=SECS seconds to wait for
> recovery to end\n"));
> + printf(_(" -T, --enable-two-phase enable two-phase commit for the
> subscription\n"));
> printf(_(" -U, --subscriber-username=NAME user name for subscriber
> connection\n"));
>
> AFAICT the patch is wrongly using tabs here instead of spaces.
>
> ~~~
>
> store_pub_sub_info:
>
> 6.
> + dbinfo[i].two_phase = opt->two_phase; /* Set two-phase option */
>
> I still think this comment only states the obvious so it is not
> helpful. Can remove it.
>
> ~~~
>
> 7.
> dbinfo[i].made_publication = false;
> +
> /* Fill subscriber attributes */
> This whitespace change is unrelated to this patch.
>
> ~~~
>
> 8.
> - pg_log_debug("subscriber(%d): subscription: %s ; connection string: %s", i,
> + pg_log_debug("subscriber(%d): subscription: %s ; connection string:
> %s, --enable-two-phase: %s", i,
> dbinfo[i].subname ? dbinfo[i].subname : "(auto)",
> - dbinfo[i].subconninfo);
> + dbinfo[i].subconninfo,
> + dbinfo[i].two_phase ? "true" : "false");
>
> IMO say "two_phase" here, not "--enable-two-phase".
>
> ======
> .../t/040_pg_createsubscriber.pl
>
> 9.
> max_worker_processes = 8
> +max_prepared_transactions = 10
> });
>
> AFAICT the comment for this test code said:
>
> # Restore default settings here but only apply it after testing standby. Some
> # standby settings should not be a lower setting than on the primary.
>
> But max_prepared_transactions = 10 is not the default setting for this GUC.
>
> ~~~
>
> 10.
> is($node_s->safe_psql('postgres', 'SELECT pg_catalog.pg_is_in_recovery()'),
> 't', 'standby is in recovery');
> +
> $node_s->stop;
>
> This whitespace change has nothing to do with the patch.
>
> ~~~
>
> 11.
> - 'replslot1'
> + 'replslot1', '--enable-two-phase'
>
> The comment for this test only says
> # pg_createsubscriber can run without --databases option
>
> Now it is doing more, so maybe it should give more details like "In
> passing, also test the --enable-two-phase option."
>
> ~~~
>
> 12.
> +# Verify that the prepared transaction is replicated to the subscriber
> +my $count_prepared_s =
> + $node_s->safe_psql($db1, "SELECT count(*) FROM pg_prepared_xacts;");
> +
> +is($count_prepared_s, qq(0), 'Prepared transaction replicated to subscriber');
> +
>
> Is this test OK? It says to verify it is replicated. How does checking
> subscriber has zero pg_prepared_xacts ensure replication occurred?
>
> ======
> Please see the attached NITPICKS diffs which includes some (not all)
> of the above suggestions.
>
> ======

I have fixed the given comments. The attached patch contains the
suggested changes.

Thanks and regards,
Shubham Khanna.

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.

On Wed, 11 Dec 2024 at 11:21, Shubham Khanna
<khannashubham1197(at)gmail(dot)com> wrote:
>
> On Wed, Dec 11, 2024 at 4:21 AM Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
>
> I have fixed the given comments. The attached patch contains the
> suggested changes.

Since all the subscriptions are created based on the two_phase option
provided, there is no need to store this for each database:
@@ -53,6 +54,7 @@ struct LogicalRepInfo
char *pubname; /* publication name */
char *subname; /* subscription name */
char *replslotname; /* replication slot name */
+ bool two_phase; /* two-phase enabled
for the subscription */

dbinfo[i].dbname = cell->val;
+ dbinfo[i].two_phase = opt->two_phase;
if (num_pubs > 0)

How about we handle something like in the attached changes.

Regards,
Vignesh

On Wed, Dec 11, 2024 at 2:08 PM vignesh C <vignesh21(at)gmail(dot)com> wrote:
>
> On Wed, 11 Dec 2024 at 11:21, Shubham Khanna
> <khannashubham1197(at)gmail(dot)com> wrote:
> >
> > On Wed, Dec 11, 2024 at 4:21 AM Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
> >
> > I have fixed the given comments. The attached patch contains the
> > suggested changes.
>
> Since all the subscriptions are created based on the two_phase option
> provided, there is no need to store this for each database:
> @@ -53,6 +54,7 @@ struct LogicalRepInfo
> char *pubname; /* publication name */
> char *subname; /* subscription name */
> char *replslotname; /* replication slot name */
> + bool two_phase; /* two-phase enabled
> for the subscription */
>
> dbinfo[i].dbname = cell->val;
> + dbinfo[i].two_phase = opt->two_phase;
> if (num_pubs > 0)
>
> How about we handle something like in the attached changes.
>

Thank you for pointing this out and for suggesting the changes. I
agree with your approach.
Also, I found a mistake in getopt_long and fixed it in this version of
the patch.
The attached patch contains the suggested changes.

Thanks and regards,
Shubham Khanna.

Hi Shubham,

Here are some review comments for the patch v4-0001.

======
GENERAL.

1.
After reading Vignesh's last review and then the pg_createsubscriber
documentation I see there can be multiple databases simultaneously
specified (by writing multiple -d switches) and in that case each one
gets its own:
--publication
--replication-slot
--subscription

OTOH, this new '--enable-two-phase' switch is just a blanket two_phase
enablement across all subscriptions. There is no way for the user to
say if they want it enabled for some subscriptions (on some databases)
but not for others. I suppose this was intentional and OK (right?),
but this point needs to be clarified in the docs.

======
doc/src/sgml/ref/pg_createsubscriber.sgml

2.
+ <para>
+ Enables <link
linkend="sql-createsubscription-params-with-two-phase"><literal>two_phase</literal></link>
+ commit for the subscription. The default is <literal>false</literal>.
+ </para>

Following on from the previous review comment. Since this switch is
applied to all database subscriptions I think the help needs to say
that. Something like.

"If there are multiple subscriptions specified, this option applies to
all of them."

~~~

3.
In the "Prerequisites" sections of the docs, it gives requirements for
various GUC settings on the source server and the target server. So,
should there be another note here advising to configure the
'max_prepared_transactions' GUC when the '--enable-two-phase' is
specified?

~~~

4. "Warnings" section includes the following:

pg_createsubscriber sets up logical replication with two-phase commit
disabled. This means that any prepared transactions will be replicated
at the time of COMMIT PREPARED, without advance preparation. Once
setup is complete, you can manually drop and re-create the
subscription(s) with the two_phase option enabled.

~

The above text is from the "Warnings" section, but it seems stale
information that needs to be updated due to the introduction of this
new '--enable-two-phase' option.

======
src/bin/pg_basebackup/pg_createsubscriber.c

usage:
5.
printf(_(" -t, --recovery-timeout=SECS seconds to wait for
recovery to end\n"));
+ printf(_(" -T, --enable-two-phase enable two-phase commit
for the subscription\n"));

Given the previous review comments (#1/#2 etc), I was wondering if it
might be better to say more like "enable two-phase commit for all
subscriptions".

======
.../t/040_pg_createsubscriber.pl

6.
+is($count_prepared_s, qq(1), 'Prepared transaction replicated to subscriber');

Should there also have been an earlier check (way back before the
PREPARE) just to make sure this count was initially 0?

======
Kind Regards,
Peter Smith.
Fujitsu Australia

Dear Shubham,

> Thank you for pointing this out and for suggesting the changes. I
> agree with your approach.
> Also, I found a mistake in getopt_long and fixed it in this version of
> the patch.
> The attached patch contains the suggested changes.

Thanks for updating the patch. I think the patch looks mostly OK.

Regarding the test code - I think we should directly refer the pg_subscription catalog,
and confirm that subtwophase is 'p'. IIUC, we can wait until
all subscriptions. Subtwophasestate are 'e', by using poll_query_until() and [1].

[1]: SELECT count(1) = 0 FROM pg_subscription WHERE subtwophasestate NOT IN ('e');

Best regards,
Hayato Kuroda
FUJITSU LIMITED

On Thu, 12 Dec 2024 at 08:14, Hayato Kuroda (Fujitsu)
<kuroda(dot)hayato(at)fujitsu(dot)com> wrote:
>
> Dear Shubham,
>
> > Thank you for pointing this out and for suggesting the changes. I
> > agree with your approach.
> > Also, I found a mistake in getopt_long and fixed it in this version of
> > the patch.
> > The attached patch contains the suggested changes.
>
> Thanks for updating the patch. I think the patch looks mostly OK.
>
> Regarding the test code - I think we should directly refer the pg_subscription catalog,
> and confirm that subtwophase is 'p'. IIUC, we can wait until
> all subscriptions. Subtwophasestate are 'e', by using poll_query_until() and [1].
>
> [1]: SELECT count(1) = 0 FROM pg_subscription WHERE subtwophasestate NOT IN ('e');

Yes, that approach is better. Also this is not required after checking
using pg_subscription:
+# Prepare a transaction on the publisher
+$node_p->safe_psql(
+ $db1, qq[
+ BEGIN;
+ INSERT INTO tbl1 SELECT generate_series(1, 10);
+ PREPARE TRANSACTION 'test_prepare';
+]);
+
# Start subscriber
$node_s->start;

+# Verify that the prepared transaction is replicated to the subscriber
+my $count_prepared_s =
+ $node_s->safe_psql($db1, "SELECT count(*) FROM pg_prepared_xacts;");
+
+is($count_prepared_s, qq(1), 'Prepared transaction replicated to subscriber');

Regards,
Vignesh

On Thu, Dec 12, 2024 at 6:04 AM Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
>
> Hi Shubham,
>
> Here are some review comments for the patch v4-0001.
>
> ======
> GENERAL.
>
> 1.
> After reading Vignesh's last review and then the pg_createsubscriber
> documentation I see there can be multiple databases simultaneously
> specified (by writing multiple -d switches) and in that case each one
> gets its own:
> --publication
> --replication-slot
> --subscription
>
> OTOH, this new '--enable-two-phase' switch is just a blanket two_phase
> enablement across all subscriptions. There is no way for the user to
> say if they want it enabled for some subscriptions (on some databases)
> but not for others. I suppose this was intentional and OK (right?),
> but this point needs to be clarified in the docs.
>

Fixed.

> ======
> doc/src/sgml/ref/pg_createsubscriber.sgml
>
> 2.
> + <para>
> + Enables <link
> linkend="sql-createsubscription-params-with-two-phase"><literal>two_phase</literal></link>
> + commit for the subscription. The default is <literal>false</literal>.
> + </para>
>
> Following on from the previous review comment. Since this switch is
> applied to all database subscriptions I think the help needs to say
> that. Something like.
>
> "If there are multiple subscriptions specified, this option applies to
> all of them."
>
> ~~~
>

Fixed.

> 3.
> In the "Prerequisites" sections of the docs, it gives requirements for
> various GUC settings on the source server and the target server. So,
> should there be another note here advising to configure the
> 'max_prepared_transactions' GUC when the '--enable-two-phase' is
> specified?
>
> ~~~
>

Fixed.

> 4. "Warnings" section includes the following:
>
> pg_createsubscriber sets up logical replication with two-phase commit
> disabled. This means that any prepared transactions will be replicated
> at the time of COMMIT PREPARED, without advance preparation. Once
> setup is complete, you can manually drop and re-create the
> subscription(s) with the two_phase option enabled.
>
> ~
>
> The above text is from the "Warnings" section, but it seems stale
> information that needs to be updated due to the introduction of this
> new '--enable-two-phase' option.
>

Fixed.

> ======
> src/bin/pg_basebackup/pg_createsubscriber.c
>
> usage:
> 5.
> printf(_(" -t, --recovery-timeout=SECS seconds to wait for
> recovery to end\n"));
> + printf(_(" -T, --enable-two-phase enable two-phase commit
> for the subscription\n"));
>
> Given the previous review comments (#1/#2 etc), I was wondering if it
> might be better to say more like "enable two-phase commit for all
> subscriptions".
>

Fixed.

> ======
> .../t/040_pg_createsubscriber.pl
>
> 6.
> +is($count_prepared_s, qq(1), 'Prepared transaction replicated to subscriber');
>
> Should there also have been an earlier check (way back before the
> PREPARE) just to make sure this count was initially 0?
>

Removed this and added a new test case instead of this.

The attached patch contains the suggested changes.

Thanks and regards,
Shubham Khanna.

On Thu, Dec 12, 2024 at 8:14 AM Hayato Kuroda (Fujitsu)
<kuroda(dot)hayato(at)fujitsu(dot)com> wrote:
>
> Dear Shubham,
>
> > Thank you for pointing this out and for suggesting the changes. I
> > agree with your approach.
> > Also, I found a mistake in getopt_long and fixed it in this version of
> > the patch.
> > The attached patch contains the suggested changes.
>
> Thanks for updating the patch. I think the patch looks mostly OK.
>
> Regarding the test code - I think we should directly refer the pg_subscription catalog,
> and confirm that subtwophase is 'p'. IIUC, we can wait until
> all subscriptions. Subtwophasestate are 'e', by using poll_query_until() and [1].
>
> [1]: SELECT count(1) = 0 FROM pg_subscription WHERE subtwophasestate NOT IN ('e');
>

I have fixed the given comment. The v5 version patch attached at [1]
has the changes for the same.
[1] - https://www.postgresql.org/message-id/CAHv8Rj%2Bhd2MTNRs4AsK6%3DhRqvV6JC9g2tTAJwGjrNfXg6vhD8g%40mail.gmail.com

Thanks and Regards,
Shubham Khanna.

On Thu, Dec 12, 2024 at 9:34 AM vignesh C <vignesh21(at)gmail(dot)com> wrote:
>
> On Thu, 12 Dec 2024 at 08:14, Hayato Kuroda (Fujitsu)
> <kuroda(dot)hayato(at)fujitsu(dot)com> wrote:
> >
> > Dear Shubham,
> >
> > > Thank you for pointing this out and for suggesting the changes. I
> > > agree with your approach.
> > > Also, I found a mistake in getopt_long and fixed it in this version of
> > > the patch.
> > > The attached patch contains the suggested changes.
> >
> > Thanks for updating the patch. I think the patch looks mostly OK.
> >
> > Regarding the test code - I think we should directly refer the pg_subscription catalog,
> > and confirm that subtwophase is 'p'. IIUC, we can wait until
> > all subscriptions. Subtwophasestate are 'e', by using poll_query_until() and [1].
> >
> > [1]: SELECT count(1) = 0 FROM pg_subscription WHERE subtwophasestate NOT IN ('e');
>
> Yes, that approach is better. Also this is not required after checking
> using pg_subscription:
> +# Prepare a transaction on the publisher
> +$node_p->safe_psql(
> + $db1, qq[
> + BEGIN;
> + INSERT INTO tbl1 SELECT generate_series(1, 10);
> + PREPARE TRANSACTION 'test_prepare';
> +]);
> +
> # Start subscriber
> $node_s->start;
>
> +# Verify that the prepared transaction is replicated to the subscriber
> +my $count_prepared_s =
> + $node_s->safe_psql($db1, "SELECT count(*) FROM pg_prepared_xacts;");
> +
> +is($count_prepared_s, qq(1), 'Prepared transaction replicated to subscriber');
>

I have fixed the given comment. The v5 version patch attached at [1]
has the changes for the same.
[1] - https://www.postgresql.org/message-id/CAHv8Rj%2Bhd2MTNRs4AsK6%3DhRqvV6JC9g2tTAJwGjrNfXg6vhD8g%40mail.gmail.com

Thanks and Regards,
Shubham Khanna.

Hi Shubham,

Here are my review comments for the patch v5-0001.

======
doc/src/sgml/ref/pg_createsubscriber.sgml

1.
- must accept local connections.
+ must accept local connections. If you are planning to use the
+ --enable-two-phase switch then you will also need to set the
+ <xref linkend="guc-max-prepared-transactions"/> appropriately.

Should use sgml <option> markup for "--enable-two-phase".

~~~

2.
- <application>pg_createsubscriber</application> sets up logical
- replication with two-phase commit disabled. This means that any
- prepared transactions will be replicated at the time
- of <command>COMMIT PREPARED</command>, without advance preparation.
- Once setup is complete, you can manually drop and re-create the
- subscription(s) with
+ If --enable-two-phase switch is not specified, the
+ <application>pg_createsubscriber</application> sets up logical replication
+ with two-phase commit disabled. This means that any prepared transactions
+ will be replicated at the time of <command>COMMIT PREPARED</command>,
+ without advance preparation. Once setup is complete, you can manually drop
+ and re-create the subscription(s) with

Should use sgml <option> markup for "--enable-two-phase".

======
.../t/040_pg_createsubscriber.pl

3.
+# Verify that the subtwophase is 'p' in the pg_subscription catalog
+my $poll_query_until = $node_s->safe_psql('postgres',
+ "SELECT count(1) = 0 FROM pg_subscription WHERE subtwophasestate NOT
IN ('e');"
+);
+
+is($poll_query_until, qq(t),
+ 'Timed out while waiting for subscriber to enable twophase');

3a.
Hmm. Does this code match the comment? The comment says verify
subtwophase is 'p' (aka "pending") but AFAICT the code is actually
waiting until every subtwophase is 'e' (aka "enabled").

~

3b.
Also, if you are going to name char-codes (like 'p') in comments, it
might be helpful to include the equivalent words, saving readers from
having to search the documentation to find the meaning.

======
Kind Regards,
Peter Smith.
Fujitsu Australia

On Fri, Dec 13, 2024 at 4:42 AM Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
>
> Hi Shubham,
>
> Here are my review comments for the patch v5-0001.
>
> ======
> doc/src/sgml/ref/pg_createsubscriber.sgml
>
> 1.
> - must accept local connections.
> + must accept local connections. If you are planning to use the
> + --enable-two-phase switch then you will also need to set the
> + <xref linkend="guc-max-prepared-transactions"/> appropriately.
>
> Should use sgml <option> markup for "--enable-two-phase".
>
> ~~~
>
> 2.
> - <application>pg_createsubscriber</application> sets up logical
> - replication with two-phase commit disabled. This means that any
> - prepared transactions will be replicated at the time
> - of <command>COMMIT PREPARED</command>, without advance preparation.
> - Once setup is complete, you can manually drop and re-create the
> - subscription(s) with
> + If --enable-two-phase switch is not specified, the
> + <application>pg_createsubscriber</application> sets up logical replication
> + with two-phase commit disabled. This means that any prepared transactions
> + will be replicated at the time of <command>COMMIT PREPARED</command>,
> + without advance preparation. Once setup is complete, you can manually drop
> + and re-create the subscription(s) with
>
> Should use sgml <option> markup for "--enable-two-phase".
>
> ======
> .../t/040_pg_createsubscriber.pl
>
> 3.
> +# Verify that the subtwophase is 'p' in the pg_subscription catalog
> +my $poll_query_until = $node_s->safe_psql('postgres',
> + "SELECT count(1) = 0 FROM pg_subscription WHERE subtwophasestate NOT
> IN ('e');"
> +);
> +
> +is($poll_query_until, qq(t),
> + 'Timed out while waiting for subscriber to enable twophase');
>
> 3a.
> Hmm. Does this code match the comment? The comment says verify
> subtwophase is 'p' (aka "pending") but AFAICT the code is actually
> waiting until every subtwophase is 'e' (aka "enabled").
>
> ~
>
> 3b.
> Also, if you are going to name char-codes (like 'p') in comments, it
> might be helpful to include the equivalent words, saving readers from
> having to search the documentation to find the meaning.
>

I have fixed the given comments. The attached patch contains the
suggested changes.

Thanks and regards,
Shubham Khanna.

Hi Shubham.

Here are my review comments for v6-0001.

======
1.
+# Verify that the subtwophase is enabled ('e') in the pg_subscription catalog
+$node_s->poll_query_until('postgres',
+ "SELECT count(1) = 2 FROM pg_subscription WHERE subtwophasestate = 'e';")
+ or die "Timed out while waiting for subscriber to enable twophase";
+

This form of the SQL is probably OK but it's a bit tricky; Probably it
should have been explained in the comment about where that count "2"
has come from.

~~

I think it was OK as before (v5) to be querying until nothing was NOT
'e'. In other words, until everything was enabled 'e'.
SELECT count(1) = 0 FROM pg_subscription WHERE subtwophasestate NOT IN ('e');

~~

OTOH, to save execution time we really would be satisfied with both
'p' and 'e' states here. (we don't strictly need to wait for the
transition from 'p' to 'e' to occur).

So, SQL like the one below might be the best:

# Verify that all subtwophase states are pending or enabled,
# e.g. there are no subscriptions where subtwophase is disabled ('d').
SELECT count(1) = 0 FROM pg_subscription WHERE subtwophasestate IN ('d')

======
Kind Regards,
Peter Smith.
Fujitsu Australia

On Fri, Dec 13, 2024 at 12:20 PM Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
>
> Hi Shubham.
>
> Here are my review comments for v6-0001.
>
> ======
> 1.
> +# Verify that the subtwophase is enabled ('e') in the pg_subscription catalog
> +$node_s->poll_query_until('postgres',
> + "SELECT count(1) = 2 FROM pg_subscription WHERE subtwophasestate = 'e';")
> + or die "Timed out while waiting for subscriber to enable twophase";
> +
>
> This form of the SQL is probably OK but it's a bit tricky; Probably it
> should have been explained in the comment about where that count "2"
> has come from.
>
> ~~
>
> I think it was OK as before (v5) to be querying until nothing was NOT
> 'e'. In other words, until everything was enabled 'e'.
> SELECT count(1) = 0 FROM pg_subscription WHERE subtwophasestate NOT IN ('e');
>
> ~~
>
> OTOH, to save execution time we really would be satisfied with both
> 'p' and 'e' states here. (we don't strictly need to wait for the
> transition from 'p' to 'e' to occur).
>
> So, SQL like the one below might be the best:
>
> # Verify that all subtwophase states are pending or enabled,
> # e.g. there are no subscriptions where subtwophase is disabled ('d').
> SELECT count(1) = 0 FROM pg_subscription WHERE subtwophasestate IN ('d')
>

I have fixed the given comment. Since prepared transactions are not
being used anymore, I have removed it from the test file.
The attached patch contains the suggested changes.

Thanks and regards,
Shubham Khanna.

On Fri, 13 Dec 2024 at 13:01, Shubham Khanna
<khannashubham1197(at)gmail(dot)com> wrote:
>
> On Fri, Dec 13, 2024 at 12:20 PM Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
> >
> > Hi Shubham.
> >
> > Here are my review comments for v6-0001.
> >
> > ======
> > 1.
> > +# Verify that the subtwophase is enabled ('e') in the pg_subscription catalog
> > +$node_s->poll_query_until('postgres',
> > + "SELECT count(1) = 2 FROM pg_subscription WHERE subtwophasestate = 'e';")
> > + or die "Timed out while waiting for subscriber to enable twophase";
> > +
> >
> > This form of the SQL is probably OK but it's a bit tricky; Probably it
> > should have been explained in the comment about where that count "2"
> > has come from.
> >
> > ~~
> >
> > I think it was OK as before (v5) to be querying until nothing was NOT
> > 'e'. In other words, until everything was enabled 'e'.
> > SELECT count(1) = 0 FROM pg_subscription WHERE subtwophasestate NOT IN ('e');
> >
> > ~~
> >
> > OTOH, to save execution time we really would be satisfied with both
> > 'p' and 'e' states here. (we don't strictly need to wait for the
> > transition from 'p' to 'e' to occur).
> >
> > So, SQL like the one below might be the best:
> >
> > # Verify that all subtwophase states are pending or enabled,
> > # e.g. there are no subscriptions where subtwophase is disabled ('d').
> > SELECT count(1) = 0 FROM pg_subscription WHERE subtwophasestate IN ('d')
> >
>
> I have fixed the given comment. Since prepared transactions are not
> being used anymore, I have removed it from the test file.
> The attached patch contains the suggested changes.

Few comments:
1) Since we are providing --enable-two-phase option now and user can
create subscriptions with two-phase option this warning can be removed
now:
<para>
- <application>pg_createsubscriber</application> sets up logical
- replication with two-phase commit disabled. This means that any
- prepared transactions will be replicated at the time
- of <command>COMMIT PREPARED</command>, without advance preparation.
- Once setup is complete, you can manually drop and re-create the
- subscription(s) with
+ If <option>--enable-two-phase</option> is not specified, the
+ <application>pg_createsubscriber</application> sets up logical replication
+ with two-phase commit disabled. This means that any prepared transactions
+ will be replicated at the time of <command>COMMIT PREPARED</command>,
+ without advance preparation. Once setup is complete, you can manually drop
+ and re-create the subscription(s) with
the <link linkend="sql-createsubscription-params-with-two-phase"><literal>two_phase</literal></link>
- option enabled.

2) Since we are not going to wait till the subscriptions are enabled,
we can use safe_psql instead of poll_query_until, something like:
is($node_s->safe_psql('postgres',
"SELECT count(1) = 0 FROM pg_subscription WHERE subtwophasestate = 'd'"),
't', 'subscriptions are created with the two-phase option enabled');

instead of:
+$node_s->poll_query_until('postgres',
+ "SELECT count(1) = 0 FROM pg_subscription WHERE
subtwophasestate IN ('d');"
+);

3) This can be removed from the commit message:
Documentation has been updated to reflect the new option, and test cases have
been added to validate various scenarios, including proper validation of the
'--enable-two-phase' option and its combinations with other options.

4) Should" two-phase option" be "enable-two-phase option" here:
const char *sub_username; /* subscriber username */
+ bool two_phase; /* two-phase option */
SimpleStringList database_names; /* list of database names */

Regards,
Vignesh

On Fri, Dec 13, 2024 at 2:39 PM vignesh C <vignesh21(at)gmail(dot)com> wrote:
>
> On Fri, 13 Dec 2024 at 13:01, Shubham Khanna
> <khannashubham1197(at)gmail(dot)com> wrote:
> >
> > On Fri, Dec 13, 2024 at 12:20 PM Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
> > >
> > > Hi Shubham.
> > >
> > > Here are my review comments for v6-0001.
> > >
> > > ======
> > > 1.
> > > +# Verify that the subtwophase is enabled ('e') in the pg_subscription catalog
> > > +$node_s->poll_query_until('postgres',
> > > + "SELECT count(1) = 2 FROM pg_subscription WHERE subtwophasestate = 'e';")
> > > + or die "Timed out while waiting for subscriber to enable twophase";
> > > +
> > >
> > > This form of the SQL is probably OK but it's a bit tricky; Probably it
> > > should have been explained in the comment about where that count "2"
> > > has come from.
> > >
> > > ~~
> > >
> > > I think it was OK as before (v5) to be querying until nothing was NOT
> > > 'e'. In other words, until everything was enabled 'e'.
> > > SELECT count(1) = 0 FROM pg_subscription WHERE subtwophasestate NOT IN ('e');
> > >
> > > ~~
> > >
> > > OTOH, to save execution time we really would be satisfied with both
> > > 'p' and 'e' states here. (we don't strictly need to wait for the
> > > transition from 'p' to 'e' to occur).
> > >
> > > So, SQL like the one below might be the best:
> > >
> > > # Verify that all subtwophase states are pending or enabled,
> > > # e.g. there are no subscriptions where subtwophase is disabled ('d').
> > > SELECT count(1) = 0 FROM pg_subscription WHERE subtwophasestate IN ('d')
> > >
> >
> > I have fixed the given comment. Since prepared transactions are not
> > being used anymore, I have removed it from the test file.
> > The attached patch contains the suggested changes.
>
> Few comments:
> 1) Since we are providing --enable-two-phase option now and user can
> create subscriptions with two-phase option this warning can be removed
> now:
> <para>
> - <application>pg_createsubscriber</application> sets up logical
> - replication with two-phase commit disabled. This means that any
> - prepared transactions will be replicated at the time
> - of <command>COMMIT PREPARED</command>, without advance preparation.
> - Once setup is complete, you can manually drop and re-create the
> - subscription(s) with
> + If <option>--enable-two-phase</option> is not specified, the
> + <application>pg_createsubscriber</application> sets up logical replication
> + with two-phase commit disabled. This means that any prepared transactions
> + will be replicated at the time of <command>COMMIT PREPARED</command>,
> + without advance preparation. Once setup is complete, you can manually drop
> + and re-create the subscription(s) with
> the <link linkend="sql-createsubscription-params-with-two-phase"><literal>two_phase</literal></link>
> - option enabled.
>
> 2) Since we are not going to wait till the subscriptions are enabled,
> we can use safe_psql instead of poll_query_until, something like:
> is($node_s->safe_psql('postgres',
> "SELECT count(1) = 0 FROM pg_subscription WHERE subtwophasestate = 'd'"),
> 't', 'subscriptions are created with the two-phase option enabled');
>
> instead of:
> +$node_s->poll_query_until('postgres',
> + "SELECT count(1) = 0 FROM pg_subscription WHERE
> subtwophasestate IN ('d');"
> +);
>
> 3) This can be removed from the commit message:
> Documentation has been updated to reflect the new option, and test cases have
> been added to validate various scenarios, including proper validation of the
> '--enable-two-phase' option and its combinations with other options.
>
> 4) Should" two-phase option" be "enable-two-phase option" here:
> const char *sub_username; /* subscriber username */
> + bool two_phase; /* two-phase option */
> SimpleStringList database_names; /* list of database names */
>

I have fixed the given comments. The attached patch contains the
suggested changes.

Thanks and regards,
Shubham Khanna.

Hi Shubham.

The patch v8-0001 looks good to me.

FYI, there are a few pending patches [1][2][3] that might have some
tests/docs overlap with this one, so be on the lookout because if
those get pushed first your patch may require rebasing.

======
[1] https://www.postgresql.org/message-id/flat/CAHut%2BPsVMbDEkbGZxpb1vVPB8uKmhFgD3ow6PXoSs5ktzBXLbQ%40mail.gmail.com#df7b92b25077554439d9bb931103dd8a
[2] https://www.postgresql.org/message-id/flat/CAHut%2BPtnA4DB_pcv4TDr4NjUSM1%3DP2N_cuZx5DX09k7LVmaqUA%40mail.gmail.com
[3] https://www.postgresql.org/message-id/flat/CAHut%2BPv%2B96CykSY6-uLKZWaa6to9x1DurmyJh8Jmu1_1P7hp4Q%40mail.gmail.com

Kind Regards,
Peter Smith.
Fujitsu Australia

On Fri, 13 Dec 2024 at 15:33, Shubham Khanna
<khannashubham1197(at)gmail(dot)com> wrote:
>
> On Fri, Dec 13, 2024 at 2:39 PM vignesh C <vignesh21(at)gmail(dot)com> wrote:
> >
> > On Fri, 13 Dec 2024 at 13:01, Shubham Khanna
> > <khannashubham1197(at)gmail(dot)com> wrote:
> > >
> > > On Fri, Dec 13, 2024 at 12:20 PM Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
> > > >
> > > > Hi Shubham.
> > > >
> > > > Here are my review comments for v6-0001.
> > > >
> > > > ======
> > > > 1.
> > > > +# Verify that the subtwophase is enabled ('e') in the pg_subscription catalog
> > > > +$node_s->poll_query_until('postgres',
> > > > + "SELECT count(1) = 2 FROM pg_subscription WHERE subtwophasestate = 'e';")
> > > > + or die "Timed out while waiting for subscriber to enable twophase";
> > > > +
> > > >
> > > > This form of the SQL is probably OK but it's a bit tricky; Probably it
> > > > should have been explained in the comment about where that count "2"
> > > > has come from.
> > > >
> > > > ~~
> > > >
> > > > I think it was OK as before (v5) to be querying until nothing was NOT
> > > > 'e'. In other words, until everything was enabled 'e'.
> > > > SELECT count(1) = 0 FROM pg_subscription WHERE subtwophasestate NOT IN ('e');
> > > >
> > > > ~~
> > > >
> > > > OTOH, to save execution time we really would be satisfied with both
> > > > 'p' and 'e' states here. (we don't strictly need to wait for the
> > > > transition from 'p' to 'e' to occur).
> > > >
> > > > So, SQL like the one below might be the best:
> > > >
> > > > # Verify that all subtwophase states are pending or enabled,
> > > > # e.g. there are no subscriptions where subtwophase is disabled ('d').
> > > > SELECT count(1) = 0 FROM pg_subscription WHERE subtwophasestate IN ('d')
> > > >
> > >
> > > I have fixed the given comment. Since prepared transactions are not
> > > being used anymore, I have removed it from the test file.
> > > The attached patch contains the suggested changes.
> >
> > Few comments:
> > 1) Since we are providing --enable-two-phase option now and user can
> > create subscriptions with two-phase option this warning can be removed
> > now:
> > <para>
> > - <application>pg_createsubscriber</application> sets up logical
> > - replication with two-phase commit disabled. This means that any
> > - prepared transactions will be replicated at the time
> > - of <command>COMMIT PREPARED</command>, without advance preparation.
> > - Once setup is complete, you can manually drop and re-create the
> > - subscription(s) with
> > + If <option>--enable-two-phase</option> is not specified, the
> > + <application>pg_createsubscriber</application> sets up logical replication
> > + with two-phase commit disabled. This means that any prepared transactions
> > + will be replicated at the time of <command>COMMIT PREPARED</command>,
> > + without advance preparation. Once setup is complete, you can manually drop
> > + and re-create the subscription(s) with
> > the <link linkend="sql-createsubscription-params-with-two-phase"><literal>two_phase</literal></link>
> > - option enabled.
> >
> > 2) Since we are not going to wait till the subscriptions are enabled,
> > we can use safe_psql instead of poll_query_until, something like:
> > is($node_s->safe_psql('postgres',
> > "SELECT count(1) = 0 FROM pg_subscription WHERE subtwophasestate = 'd'"),
> > 't', 'subscriptions are created with the two-phase option enabled');
> >
> > instead of:
> > +$node_s->poll_query_until('postgres',
> > + "SELECT count(1) = 0 FROM pg_subscription WHERE
> > subtwophasestate IN ('d');"
> > +);
> >
> > 3) This can be removed from the commit message:
> > Documentation has been updated to reflect the new option, and test cases have
> > been added to validate various scenarios, including proper validation of the
> > '--enable-two-phase' option and its combinations with other options.
> >
> > 4) Should" two-phase option" be "enable-two-phase option" here:
> > const char *sub_username; /* subscriber username */
> > + bool two_phase; /* two-phase option */
> > SimpleStringList database_names; /* list of database names */
> >
>
> I have fixed the given comments. The attached patch contains the
> suggested changes.

The documentation requires a minor update: instead of specifying
subscriptions, the user will specify multiple databases, and the
subscription will be created on the specified databases. Documentation
should be updated accordingly:
+ <para>
+ Enables <link
linkend="sql-createsubscription-params-with-two-phase"><literal>two_phase</literal></link>
+ commit for the subscription. If there are multiple subscriptions
+ specified, this option applies to all of them.
+ The default is <literal>false</literal>.

Regards,
Vignesh

On Fri, Dec 27, 2024 at 11:30 AM vignesh C <vignesh21(at)gmail(dot)com> wrote:
> > > > >
> The documentation requires a minor update: instead of specifying
> subscriptions, the user will specify multiple databases, and the
> subscription will be created on the specified databases. Documentation
> should be updated accordingly:
> + <para>
> + Enables <link
> linkend="sql-createsubscription-params-with-two-phase"><literal>two_phase</literal></link>
> + commit for the subscription. If there are multiple subscriptions
> + specified, this option applies to all of them.
> + The default is <literal>false</literal>.
>

I have fixed the given comment. The attached patch contains the
suggested changes.

Thanks and regards,
Shubham Khanna.

On Fri, Dec 27, 2024 at 5:36 PM Shubham Khanna
<khannashubham1197(at)gmail(dot)com> wrote:
>
The patch no longer applies on HEAD. Please do rebase.

regards,
Ajin Cherian
Fujitsu Australia

On Fri, Jan 10, 2025 at 9:08 PM Ajin Cherian <itsajin(at)gmail(dot)com> wrote:
>
> On Fri, Dec 27, 2024 at 5:36 PM Shubham Khanna
> <khannashubham1197(at)gmail(dot)com> wrote:
> >
> The patch no longer applies on HEAD. Please do rebase.
>
Sorry, I was mistaken. Ignore this. The patch does apply on HEAD.

regards,
Ajin Cherian

On Fri, 27 Dec 2024 at 12:06, Shubham Khanna
<khannashubham1197(at)gmail(dot)com> wrote:
>
> On Fri, Dec 27, 2024 at 11:30 AM vignesh C <vignesh21(at)gmail(dot)com> wrote:
> > > > > >
> > The documentation requires a minor update: instead of specifying
> > subscriptions, the user will specify multiple databases, and the
> > subscription will be created on the specified databases. Documentation
> > should be updated accordingly:
> > + <para>
> > + Enables <link
> > linkend="sql-createsubscription-params-with-two-phase"><literal>two_phase</literal></link>
> > + commit for the subscription. If there are multiple subscriptions
> > + specified, this option applies to all of them.
> > + The default is <literal>false</literal>.
> >
>
> I have fixed the given comment. The attached patch contains the
> suggested changes.
>

Hi Shubham,

I have reviewed the v9 patch. It looks fine to me. I have tested it
and it is working as expected.
I have few comments:

1.
- pg_log_debug("subscriber(%d): subscription: %s ; connection
string: %s", i,
+ pg_log_debug("subscriber(%d): subscription: %s ; connection
string: %s, two_phase: %s", i,
dbinfo[i].subname ? dbinfo[i].subname : "(auto)",
- dbinfo[i].subconninfo);
+ dbinfo[i].subconninfo,
+ opt->two_phase ? "true" : "false");
Here the value of 'opt->two_phase' will be the same for all
subscriptions. So is it good to log it here? Thoughts?

2. In documentation pg_createsubscriber.sgml. Under Warning section,
we have following:

<application>pg_createsubscriber</application> sets up logical
replication with two-phase commit disabled. This means that any
prepared transactions will be replicated at the time
of <command>COMMIT PREPARED</command>, without advance preparation.
Once setup is complete, you can manually drop and re-create the
subscription(s) with
the <link linkend="sql-createsubscription-params-with-two-phase"><literal>two_phase</literal></link>
option enabled.

I think we should update the documentation accordingly.

Thanks and Regards,
Shlok Kyal

On Tue, Jan 14, 2025 at 4:53 PM Shlok Kyal <shlok(dot)kyal(dot)oss(at)gmail(dot)com> wrote:
>
> On Fri, 27 Dec 2024 at 12:06, Shubham Khanna
> <khannashubham1197(at)gmail(dot)com> wrote:
> >
> > On Fri, Dec 27, 2024 at 11:30 AM vignesh C <vignesh21(at)gmail(dot)com> wrote:
> > > > > > >
> > > The documentation requires a minor update: instead of specifying
> > > subscriptions, the user will specify multiple databases, and the
> > > subscription will be created on the specified databases. Documentation
> > > should be updated accordingly:
> > > + <para>
> > > + Enables <link
> > > linkend="sql-createsubscription-params-with-two-phase"><literal>two_phase</literal></link>
> > > + commit for the subscription. If there are multiple subscriptions
> > > + specified, this option applies to all of them.
> > > + The default is <literal>false</literal>.
> > >
> >
> > I have fixed the given comment. The attached patch contains the
> > suggested changes.
> >
>
> Hi Shubham,
>
> I have reviewed the v9 patch. It looks fine to me. I have tested it
> and it is working as expected.
> I have few comments:
>
> 1.
> - pg_log_debug("subscriber(%d): subscription: %s ; connection
> string: %s", i,
> + pg_log_debug("subscriber(%d): subscription: %s ; connection
> string: %s, two_phase: %s", i,
> dbinfo[i].subname ? dbinfo[i].subname : "(auto)",
> - dbinfo[i].subconninfo);
> + dbinfo[i].subconninfo,
> + opt->two_phase ? "true" : "false");
> Here the value of 'opt->two_phase' will be the same for all
> subscriptions. So is it good to log it here? Thoughts?
>

Here, the 'two-phase option' is a global setting, so it makes sense to
log it consistently within the debug output. While it is true that
'opt->two_phase' will have the same value for all subscriptions,
including it in the debug log provides clarity about the replication
setup at the time of execution.
This information can be particularly useful for debugging purposes, as
it gives immediate insight into whether a 'two-phase' commit was
enabled or disabled during the setup. Therefore, logging the
'two-phase' status, even though it's consistent across subscriptions,
adds valuable context to the debug output.

> 2. In documentation pg_createsubscriber.sgml. Under Warning section,
> we have following:
>
> <application>pg_createsubscriber</application> sets up logical
> replication with two-phase commit disabled. This means that any
> prepared transactions will be replicated at the time
> of <command>COMMIT PREPARED</command>, without advance preparation.
> Once setup is complete, you can manually drop and re-create the
> subscription(s) with
> the <link linkend="sql-createsubscription-params-with-two-phase"><literal>two_phase</literal></link>
> option enabled.
>
> I think we should update the documentation accordingly.
>

Previously, the warning was necessary because the 'two-phase' option
was not available, and users needed to be informed about the default
behavior regarding 'two-phase' commit. However, with the recent
addition of the 'two-phase' option, users can now directly configure
this behavior during the setup process.
Given this enhancement, the warning message is no longer relevant and
should be removed from the documentation to reflect the latest changes
accurately. Updating the documentation will help ensure that it aligns
with the current functionality and avoids any potential confusion for
users.

The attached patch contains the required changes.

Thanks and regards,
Shubham Khanna.

On Wed, Jan 15, 2025 at 5:33 PM Shubham Khanna <khannashubham1197(at)gmail(dot)com>
wrote:
> Previously, the warning was necessary because the 'two-phase' option
> was not available, and users needed to be informed about the default
> behavior regarding 'two-phase' commit. However, with the recent
> addition of the 'two-phase' option, users can now directly configure
> this behavior during the setup process.
> Given this enhancement, the warning message is no longer relevant and
> should be removed from the documentation to reflect the latest changes
> accurately. Updating the documentation will help ensure that it aligns
> with the current functionality and avoids any potential confusion for
> users.

Hi Shubham,

Even though the documentation is updated, the actual code still gives a
warning, when you try and create pg_createsubscriber with the
--enable-two-phase option:

pg_createsubscriber: warning: two_phase option will not be enabled for
replication slots
pg_createsubscriber: detail: Subscriptions will be created with the
two_phase option disabled. Prepared transactions will be replicated at
COMMIT PREPARED.

This is coming from code in check_publisher()

if (max_prepared_transactions != 0)
{
pg_log_warning("two_phase option will not be enabled for
replication slots");
pg_log_warning_detail("Subscriptions will be created with the
two_phase option disabled. "
"Prepared transactions will be replicated at
COMMIT PREPARED.");
}

I think this code needs to be updated as well.

regards,
Ajin Cherian
Fujitsu Australia

On Wed, Jan 15, 2025 at 9:24 PM Ajin Cherian <itsajin(at)gmail(dot)com> wrote:
>
>
>
> On Wed, Jan 15, 2025 at 5:33 PM Shubham Khanna <khannashubham1197(at)gmail(dot)com> wrote:
> > Previously, the warning was necessary because the 'two-phase' option
> > was not available, and users needed to be informed about the default
> > behavior regarding 'two-phase' commit. However, with the recent
> > addition of the 'two-phase' option, users can now directly configure
> > this behavior during the setup process.
> > Given this enhancement, the warning message is no longer relevant and
> > should be removed from the documentation to reflect the latest changes
> > accurately. Updating the documentation will help ensure that it aligns
> > with the current functionality and avoids any potential confusion for
> > users.
>
> Hi Shubham,
>
> Even though the documentation is updated, the actual code still gives a warning, when you try and create pg_createsubscriber with the --enable-two-phase option:
>
> pg_createsubscriber: warning: two_phase option will not be enabled for replication slots
> pg_createsubscriber: detail: Subscriptions will be created with the two_phase option disabled. Prepared transactions will be replicated at COMMIT PREPARED.
>
> This is coming from code in check_publisher()
>
> if (max_prepared_transactions != 0)
> {
> pg_log_warning("two_phase option will not be enabled for replication slots");
> pg_log_warning_detail("Subscriptions will be created with the two_phase option disabled. "
> "Prepared transactions will be replicated at COMMIT PREPARED.");
> }
>
> I think this code needs to be updated as well.
>

Hi Shubham.

I'm not so sure about the documentation change of v10.

Sure, we can remove the warnings in the docs and just assume/expect
the user to be aware that there is a new '--enable-two-phase' option
that they should be using. That is what v10 is now doing.

OTOH, if the user (accidentally?) omits the new '--enable-two-phase'
switch then all this two-phase documentation still seems relevant.

~

In other words, we could've left all this documented information
intact, but just qualify the paragraph. For example:

CURRENTLY (v9)
pg_createsubscriber sets up logical replication with two-phase commit
disabled. This means that any prepared transactions will be replicated
at the time of COMMIT PREPARED, without advance preparation. Once
setup is complete, you can manually drop and re-create the
subscription(s) with the two_phase option enabled.

SUGGESTION
Unless the '--enable-two-phase' switch is specified,
pg_createsubscriber sets up ...

~

Similarly, to help (accident prone?) users we could leave this code
warning [1] intact, but just change the condition slightly, and add a
helpful hint on how to avoid the problem.

CURRENTLY (v10)
if (max_prepared_transactions != 0)
{
pg_log_warning("two_phase option will not be enabled for
replication slots");
pg_log_warning_detail("Subscriptions will be created with the
two_phase option disabled. "
"Prepared transactions will be
replicated at COMMIT PREPARED.");
}

SUGGESTION
if (max_prepared_transactions != 0 && !opt->two_phase)
{
pg_log_warning("two_phase option will not be enabled for
replication slots");
pg_log_warning_detail("Subscriptions will be created with the
two_phase option disabled. "
"Prepared transactions will be
replicated at COMMIT PREPARED.");
pg_log_warning_hint("You can use '--enable-two_phase' switch
to enable two_phase.");
}

~~~

But, check what other people think just in case I am in the minority
by thinking these warnings in docs/code are still potentially useful.

======
[1] https://www.postgresql.org/message-id/CAFPTHDbdXa4wh01B98L8VssJnyH%3DuK6Qfi%3DsKKVXRq-9_YwXsg%40mail.gmail.com

Kind Regards,
Peter Smith.
Fujitsu Australia

On Wed, 15 Jan 2025 at 12:03, Shubham Khanna
<khannashubham1197(at)gmail(dot)com> wrote:
>
> On Tue, Jan 14, 2025 at 4:53 PM Shlok Kyal <shlok(dot)kyal(dot)oss(at)gmail(dot)com> wrote:
> >
> > On Fri, 27 Dec 2024 at 12:06, Shubham Khanna
> > <khannashubham1197(at)gmail(dot)com> wrote:
> > >
> > > On Fri, Dec 27, 2024 at 11:30 AM vignesh C <vignesh21(at)gmail(dot)com> wrote:
> > > > > > > >
> > > > The documentation requires a minor update: instead of specifying
> > > > subscriptions, the user will specify multiple databases, and the
> > > > subscription will be created on the specified databases. Documentation
> > > > should be updated accordingly:
> > > > + <para>
> > > > + Enables <link
> > > > linkend="sql-createsubscription-params-with-two-phase"><literal>two_phase</literal></link>
> > > > + commit for the subscription. If there are multiple subscriptions
> > > > + specified, this option applies to all of them.
> > > > + The default is <literal>false</literal>.
> > > >
> > >
> > > I have fixed the given comment. The attached patch contains the
> > > suggested changes.
> > >
> >
> > Hi Shubham,
> >
> > I have reviewed the v9 patch. It looks fine to me. I have tested it
> > and it is working as expected.
> > I have few comments:
> >
> > 1.
> > - pg_log_debug("subscriber(%d): subscription: %s ; connection
> > string: %s", i,
> > + pg_log_debug("subscriber(%d): subscription: %s ; connection
> > string: %s, two_phase: %s", i,
> > dbinfo[i].subname ? dbinfo[i].subname : "(auto)",
> > - dbinfo[i].subconninfo);
> > + dbinfo[i].subconninfo,
> > + opt->two_phase ? "true" : "false");
> > Here the value of 'opt->two_phase' will be the same for all
> > subscriptions. So is it good to log it here? Thoughts?
> >
>
> Here, the 'two-phase option' is a global setting, so it makes sense to
> log it consistently within the debug output. While it is true that
> 'opt->two_phase' will have the same value for all subscriptions,
> including it in the debug log provides clarity about the replication
> setup at the time of execution.
> This information can be particularly useful for debugging purposes, as
> it gives immediate insight into whether a 'two-phase' commit was
> enabled or disabled during the setup. Therefore, logging the
> 'two-phase' status, even though it's consistent across subscriptions,
> adds valuable context to the debug output.
>
> > 2. In documentation pg_createsubscriber.sgml. Under Warning section,
> > we have following:
> >
> > <application>pg_createsubscriber</application> sets up logical
> > replication with two-phase commit disabled. This means that any
> > prepared transactions will be replicated at the time
> > of <command>COMMIT PREPARED</command>, without advance preparation.
> > Once setup is complete, you can manually drop and re-create the
> > subscription(s) with
> > the <link linkend="sql-createsubscription-params-with-two-phase"><literal>two_phase</literal></link>
> > option enabled.
> >
> > I think we should update the documentation accordingly.
> >
>
> Previously, the warning was necessary because the 'two-phase' option
> was not available, and users needed to be informed about the default
> behavior regarding 'two-phase' commit. However, with the recent
> addition of the 'two-phase' option, users can now directly configure
> this behavior during the setup process.
> Given this enhancement, the warning message is no longer relevant and
> should be removed from the documentation to reflect the latest changes
> accurately. Updating the documentation will help ensure that it aligns
> with the current functionality and avoids any potential confusion for
> users.
>
> The attached patch contains the required changes.
>

Hi Shubham,

Thanks for providing the updated patch.

I have few comments:

1. When we are using '--enable-two-phase' option, I think we should
also set the 'two_phase = true' while creating logical replication
slots as we are using the same slot for the subscriber.
Currently by default it is being set to 'false':

appendPQExpBuffer(str,
"SELECT lsn FROM
pg_catalog.pg_create_logical_replication_slot(%s, 'pgoutput', false,
false, false)",
slot_name_esc);

2. Regarding the documentation and warning message regarding the two
phase option, I agreed with the suggestions given by Peter in [1].

[1]: https://www.postgresql.org/message-id/CAHut%2BPviJDKW0ftZSg1cX4XK5EJmhGW-Uh3wGrRE4ktFrnxn9w%40mail.gmail.com

Thanks and Regards,
Shlok Kyal

On Wed, Jan 15, 2025 at 3:54 PM Ajin Cherian <itsajin(at)gmail(dot)com> wrote:
>
>
>
> On Wed, Jan 15, 2025 at 5:33 PM Shubham Khanna <khannashubham1197(at)gmail(dot)com> wrote:
> > Previously, the warning was necessary because the 'two-phase' option
> > was not available, and users needed to be informed about the default
> > behavior regarding 'two-phase' commit. However, with the recent
> > addition of the 'two-phase' option, users can now directly configure
> > this behavior during the setup process.
> > Given this enhancement, the warning message is no longer relevant and
> > should be removed from the documentation to reflect the latest changes
> > accurately. Updating the documentation will help ensure that it aligns
> > with the current functionality and avoids any potential confusion for
> > users.
>
> Hi Shubham,
>
> Even though the documentation is updated, the actual code still gives a warning, when you try and create pg_createsubscriber with the --enable-two-phase option:
>
> pg_createsubscriber: warning: two_phase option will not be enabled for replication slots
> pg_createsubscriber: detail: Subscriptions will be created with the two_phase option disabled. Prepared transactions will be replicated at COMMIT PREPARED.
>
> This is coming from code in check_publisher()
>
> if (max_prepared_transactions != 0)
> {
> pg_log_warning("two_phase option will not be enabled for replication slots");
> pg_log_warning_detail("Subscriptions will be created with the two_phase option disabled. "
> "Prepared transactions will be replicated at COMMIT PREPARED.");
> }
>
> I think this code needs to be updated as well.
>

Fixed the given comment. The attached patch contains the required changes.

Thanks and regards,
Shubham Khanna.

On Thu, Jan 16, 2025 at 4:53 AM Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
>
> On Wed, Jan 15, 2025 at 9:24 PM Ajin Cherian <itsajin(at)gmail(dot)com> wrote:
> >
> >
> >
> > On Wed, Jan 15, 2025 at 5:33 PM Shubham Khanna <khannashubham1197(at)gmail(dot)com> wrote:
> > > Previously, the warning was necessary because the 'two-phase' option
> > > was not available, and users needed to be informed about the default
> > > behavior regarding 'two-phase' commit. However, with the recent
> > > addition of the 'two-phase' option, users can now directly configure
> > > this behavior during the setup process.
> > > Given this enhancement, the warning message is no longer relevant and
> > > should be removed from the documentation to reflect the latest changes
> > > accurately. Updating the documentation will help ensure that it aligns
> > > with the current functionality and avoids any potential confusion for
> > > users.
> >
> > Hi Shubham,
> >
> > Even though the documentation is updated, the actual code still gives a warning, when you try and create pg_createsubscriber with the --enable-two-phase option:
> >
> > pg_createsubscriber: warning: two_phase option will not be enabled for replication slots
> > pg_createsubscriber: detail: Subscriptions will be created with the two_phase option disabled. Prepared transactions will be replicated at COMMIT PREPARED.
> >
> > This is coming from code in check_publisher()
> >
> > if (max_prepared_transactions != 0)
> > {
> > pg_log_warning("two_phase option will not be enabled for replication slots");
> > pg_log_warning_detail("Subscriptions will be created with the two_phase option disabled. "
> > "Prepared transactions will be replicated at COMMIT PREPARED.");
> > }
> >
> > I think this code needs to be updated as well.
> >
>
> Hi Shubham.
>
> I'm not so sure about the documentation change of v10.
>
> Sure, we can remove the warnings in the docs and just assume/expect
> the user to be aware that there is a new '--enable-two-phase' option
> that they should be using. That is what v10 is now doing.
>
> OTOH, if the user (accidentally?) omits the new '--enable-two-phase'
> switch then all this two-phase documentation still seems relevant.
>
> ~
>
> In other words, we could've left all this documented information
> intact, but just qualify the paragraph. For example:
>
> CURRENTLY (v9)
> pg_createsubscriber sets up logical replication with two-phase commit
> disabled. This means that any prepared transactions will be replicated
> at the time of COMMIT PREPARED, without advance preparation. Once
> setup is complete, you can manually drop and re-create the
> subscription(s) with the two_phase option enabled.
>
>
> SUGGESTION
> Unless the '--enable-two-phase' switch is specified,
> pg_createsubscriber sets up ...
>
> ~
>
> Similarly, to help (accident prone?) users we could leave this code
> warning [1] intact, but just change the condition slightly, and add a
> helpful hint on how to avoid the problem.
>
> CURRENTLY (v10)
> if (max_prepared_transactions != 0)
> {
> pg_log_warning("two_phase option will not be enabled for
> replication slots");
> pg_log_warning_detail("Subscriptions will be created with the
> two_phase option disabled. "
> "Prepared transactions will be
> replicated at COMMIT PREPARED.");
> }
>
> SUGGESTION
> if (max_prepared_transactions != 0 && !opt->two_phase)
> {
> pg_log_warning("two_phase option will not be enabled for
> replication slots");
> pg_log_warning_detail("Subscriptions will be created with the
> two_phase option disabled. "
> "Prepared transactions will be
> replicated at COMMIT PREPARED.");
> pg_log_warning_hint("You can use '--enable-two_phase' switch
> to enable two_phase.");
> }
>
> ~~~
>
> But, check what other people think just in case I am in the minority
> by thinking these warnings in docs/code are still potentially useful.
>

I agree with your suggestions. I have used your suggestions and added
them to the latest patch.
The v11 version patch attached at [1] has the changes for the same.

[1] - https://www.postgresql.org/message-id/CAHv8Rj%2BZqc9qVEaJDW03Cux_S_CMHWNM056qqJi5%2Bz9vSYgtew%40mail.gmail.com

Thanks and Regards,
Shubham Khanna.

On Thu, Jan 16, 2025 at 3:15 PM Shlok Kyal <shlok(dot)kyal(dot)oss(at)gmail(dot)com> wrote:
>
> On Wed, 15 Jan 2025 at 12:03, Shubham Khanna
> <khannashubham1197(at)gmail(dot)com> wrote:
> >
> > On Tue, Jan 14, 2025 at 4:53 PM Shlok Kyal <shlok(dot)kyal(dot)oss(at)gmail(dot)com> wrote:
> > >
> > > On Fri, 27 Dec 2024 at 12:06, Shubham Khanna
> > > <khannashubham1197(at)gmail(dot)com> wrote:
> > > >
> > > > On Fri, Dec 27, 2024 at 11:30 AM vignesh C <vignesh21(at)gmail(dot)com> wrote:
> > > > > > > > >
> > > > > The documentation requires a minor update: instead of specifying
> > > > > subscriptions, the user will specify multiple databases, and the
> > > > > subscription will be created on the specified databases. Documentation
> > > > > should be updated accordingly:
> > > > > + <para>
> > > > > + Enables <link
> > > > > linkend="sql-createsubscription-params-with-two-phase"><literal>two_phase</literal></link>
> > > > > + commit for the subscription. If there are multiple subscriptions
> > > > > + specified, this option applies to all of them.
> > > > > + The default is <literal>false</literal>.
> > > > >
> > > >
> > > > I have fixed the given comment. The attached patch contains the
> > > > suggested changes.
> > > >
> > >
> > > Hi Shubham,
> > >
> > > I have reviewed the v9 patch. It looks fine to me. I have tested it
> > > and it is working as expected.
> > > I have few comments:
> > >
> > > 1.
> > > - pg_log_debug("subscriber(%d): subscription: %s ; connection
> > > string: %s", i,
> > > + pg_log_debug("subscriber(%d): subscription: %s ; connection
> > > string: %s, two_phase: %s", i,
> > > dbinfo[i].subname ? dbinfo[i].subname : "(auto)",
> > > - dbinfo[i].subconninfo);
> > > + dbinfo[i].subconninfo,
> > > + opt->two_phase ? "true" : "false");
> > > Here the value of 'opt->two_phase' will be the same for all
> > > subscriptions. So is it good to log it here? Thoughts?
> > >
> >
> > Here, the 'two-phase option' is a global setting, so it makes sense to
> > log it consistently within the debug output. While it is true that
> > 'opt->two_phase' will have the same value for all subscriptions,
> > including it in the debug log provides clarity about the replication
> > setup at the time of execution.
> > This information can be particularly useful for debugging purposes, as
> > it gives immediate insight into whether a 'two-phase' commit was
> > enabled or disabled during the setup. Therefore, logging the
> > 'two-phase' status, even though it's consistent across subscriptions,
> > adds valuable context to the debug output.
> >
> > > 2. In documentation pg_createsubscriber.sgml. Under Warning section,
> > > we have following:
> > >
> > > <application>pg_createsubscriber</application> sets up logical
> > > replication with two-phase commit disabled. This means that any
> > > prepared transactions will be replicated at the time
> > > of <command>COMMIT PREPARED</command>, without advance preparation.
> > > Once setup is complete, you can manually drop and re-create the
> > > subscription(s) with
> > > the <link linkend="sql-createsubscription-params-with-two-phase"><literal>two_phase</literal></link>
> > > option enabled.
> > >
> > > I think we should update the documentation accordingly.
> > >
> >
> > Previously, the warning was necessary because the 'two-phase' option
> > was not available, and users needed to be informed about the default
> > behavior regarding 'two-phase' commit. However, with the recent
> > addition of the 'two-phase' option, users can now directly configure
> > this behavior during the setup process.
> > Given this enhancement, the warning message is no longer relevant and
> > should be removed from the documentation to reflect the latest changes
> > accurately. Updating the documentation will help ensure that it aligns
> > with the current functionality and avoids any potential confusion for
> > users.
> >
> > The attached patch contains the required changes.
> >
>
> Hi Shubham,
>
> Thanks for providing the updated patch.
>
> I have few comments:
>
> 1. When we are using '--enable-two-phase' option, I think we should
> also set the 'two_phase = true' while creating logical replication
> slots as we are using the same slot for the subscriber.
> Currently by default it is being set to 'false':
>
> appendPQExpBuffer(str,
> "SELECT lsn FROM
> pg_catalog.pg_create_logical_replication_slot(%s, 'pgoutput', false,
> false, false)",
> slot_name_esc);
>
> 2. Regarding the documentation and warning message regarding the two
> phase option, I agreed with the suggestions given by Peter in [1].
>
> [1]: https://www.postgresql.org/message-id/CAHut%2BPviJDKW0ftZSg1cX4XK5EJmhGW-Uh3wGrRE4ktFrnxn9w%40mail.gmail.com
>

Following our previous discussions, I have incorporated the necessary
changes to ensure that when the '--enable-two-phase' option is used,
the logical replication slots are also created with the 'two_phase =
true' setting. This addresses the current behavior where 'two_phase'
is set to false by default.

The v11 version patch attached at [1] has the changes for the same.

[1] - https://www.postgresql.org/message-id/CAHv8Rj%2BZqc9qVEaJDW03Cux_S_CMHWNM056qqJi5%2Bz9vSYgtew%40mail.gmail.com

Thanks and Regards,
Shubham Khanna.

Hi Shubham,

Some review comments for patch v11-0001.

======
src/sgml/ref/pg_createsubscriber.sgml

1.
- must accept local connections.
+ must accept local connections. If you are planning to use the
+ <option>--enable-two-phase</option> then you will also need to set the
+ <xref linkend="guc-max-prepared-transactions"/> appropriately.
</para>

The missing word "switch"?

/If you are planning to use the <option>--enable-two-phase</option>/If
you are planning to use the <option>--enable-two-phase</option>
switch/

2.
<para>
+ Unless the '--enable-two-phase' switch is specified,
<application>pg_createsubscriber</application> sets up logical

Remove the single quotes, and use the proper SGML markup same as other
places where --enable-two-phase is mentioned.

<option>--enable-two-phase</option>

======

3.
+ pg_log_warning_hint("You can use '--enable-two_phase' switch to
enable two_phase.");

3a.
Typo. The real switch name does not have underscores.

/--enable-two_phase/--enable-two-phase/

~

3b.
I checked other PG source code, but I couldn't find any examples where
the switch is given in single quotes quite like this.

Maybe choose from one of the below instead:

SUGGESTION #1
Use the \"--enable-two-phase\" switch to enable two_phase.

SUGGESTION #2
Use the --enable-two-phase switch to enable two_phase.

======
Kind Regards,
Peter Smith.
Fujitsu Australia

On Fri, Jan 17, 2025 at 5:43 AM Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
>
> Hi Shubham,
>
> Some review comments for patch v11-0001.
>
> ======
> src/sgml/ref/pg_createsubscriber.sgml
>
> 1.
> - must accept local connections.
> + must accept local connections. If you are planning to use the
> + <option>--enable-two-phase</option> then you will also need to set the
> + <xref linkend="guc-max-prepared-transactions"/> appropriately.
> </para>
>
>
> The missing word "switch"?
>
> /If you are planning to use the <option>--enable-two-phase</option>/If
> you are planning to use the <option>--enable-two-phase</option>
> switch/
>
>
> 2.
> <para>
> + Unless the '--enable-two-phase' switch is specified,
> <application>pg_createsubscriber</application> sets up logical
>
> Remove the single quotes, and use the proper SGML markup same as other
> places where --enable-two-phase is mentioned.
>
> <option>--enable-two-phase</option>
>
> ======
>
>
> 3.
> + pg_log_warning_hint("You can use '--enable-two_phase' switch to
> enable two_phase.");
>
> 3a.
> Typo. The real switch name does not have underscores.
>
> /--enable-two_phase/--enable-two-phase/
>
> ~
>
> 3b.
> I checked other PG source code, but I couldn't find any examples where
> the switch is given in single quotes quite like this.
>
> Maybe choose from one of the below instead:
>
> SUGGESTION #1
> Use the \"--enable-two-phase\" switch to enable two_phase.
>
> SUGGESTION #2
> Use the --enable-two-phase switch to enable two_phase.
>

Fixed the given comments. The attached patch contains the required changes.

Thanks and regards,
Shubham Khanna.

On Fri, 17 Jan 2025 at 09:52, Shubham Khanna
<khannashubham1197(at)gmail(dot)com> wrote:
>
> On Fri, Jan 17, 2025 at 5:43 AM Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
> >
> > Hi Shubham,
> >
> > Some review comments for patch v11-0001.
> >
> > ======
> > src/sgml/ref/pg_createsubscriber.sgml
> >
> > 1.
> > - must accept local connections.
> > + must accept local connections. If you are planning to use the
> > + <option>--enable-two-phase</option> then you will also need to set the
> > + <xref linkend="guc-max-prepared-transactions"/> appropriately.
> > </para>
> >
> >
> > The missing word "switch"?
> >
> > /If you are planning to use the <option>--enable-two-phase</option>/If
> > you are planning to use the <option>--enable-two-phase</option>
> > switch/
> >
> >
> > 2.
> > <para>
> > + Unless the '--enable-two-phase' switch is specified,
> > <application>pg_createsubscriber</application> sets up logical
> >
> > Remove the single quotes, and use the proper SGML markup same as other
> > places where --enable-two-phase is mentioned.
> >
> > <option>--enable-two-phase</option>
> >
> > ======
> >
> >
> > 3.
> > + pg_log_warning_hint("You can use '--enable-two_phase' switch to
> > enable two_phase.");
> >
> > 3a.
> > Typo. The real switch name does not have underscores.
> >
> > /--enable-two_phase/--enable-two-phase/
> >
> > ~
> >
> > 3b.
> > I checked other PG source code, but I couldn't find any examples where
> > the switch is given in single quotes quite like this.
> >
> > Maybe choose from one of the below instead:
> >
> > SUGGESTION #1
> > Use the \"--enable-two-phase\" switch to enable two_phase.
> >
> > SUGGESTION #2
> > Use the --enable-two-phase switch to enable two_phase.
> >
>
> Fixed the given comments. The attached patch contains the required changes.
>

Hi Shubham,

I have a comment for the v12 patch.
I think we can pass just the two_phase option instead of the whole
structure here. As we are just using 'opt->two_phase'.

+check_publisher(const struct LogicalRepInfo *dbinfo,
+ const struct CreateSubscriberOptions *opt)

Thanks and Regards,
Shlok Kyal

Hi Shubham,

Patch v12-0001 LGTM.

BTW, there is a precedent for passing the 'opt' arg but then only
using one member from it. See function wait_for_end_recovery in this
same file. So whether you decide to change the code per Shlok's review
[1], or decide not to change it -- either way is OK for me.

======
[1] https://www.postgresql.org/message-id/CANhcyEXQ1h%3DoSPFFziCZuU6far6a82DQafL0S85CyVRyEntA%2Bw%40mail.gmail.com

Kind Regards,
Peter Smith.
Fujitsu Australia

On Mon, Jan 20, 2025 at 1:34 AM Shlok Kyal <shlok(dot)kyal(dot)oss(at)gmail(dot)com> wrote:
>
> On Fri, 17 Jan 2025 at 09:52, Shubham Khanna
> <khannashubham1197(at)gmail(dot)com> wrote:
> >
> > On Fri, Jan 17, 2025 at 5:43 AM Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
> > >
> > > Hi Shubham,
> > >
> > > Some review comments for patch v11-0001.
> > >
> > > ======
> > > src/sgml/ref/pg_createsubscriber.sgml
> > >
> > > 1.
> > > - must accept local connections.
> > > + must accept local connections. If you are planning to use the
> > > + <option>--enable-two-phase</option> then you will also need to set the
> > > + <xref linkend="guc-max-prepared-transactions"/> appropriately.
> > > </para>
> > >
> > >
> > > The missing word "switch"?
> > >
> > > /If you are planning to use the <option>--enable-two-phase</option>/If
> > > you are planning to use the <option>--enable-two-phase</option>
> > > switch/
> > >
> > >
> > > 2.
> > > <para>
> > > + Unless the '--enable-two-phase' switch is specified,
> > > <application>pg_createsubscriber</application> sets up logical
> > >
> > > Remove the single quotes, and use the proper SGML markup same as other
> > > places where --enable-two-phase is mentioned.
> > >
> > > <option>--enable-two-phase</option>
> > >
> > > ======
> > >
> > >
> > > 3.
> > > + pg_log_warning_hint("You can use '--enable-two_phase' switch to
> > > enable two_phase.");
> > >
> > > 3a.
> > > Typo. The real switch name does not have underscores.
> > >
> > > /--enable-two_phase/--enable-two-phase/
> > >
> > > ~
> > >
> > > 3b.
> > > I checked other PG source code, but I couldn't find any examples where
> > > the switch is given in single quotes quite like this.
> > >
> > > Maybe choose from one of the below instead:
> > >
> > > SUGGESTION #1
> > > Use the \"--enable-two-phase\" switch to enable two_phase.
> > >
> > > SUGGESTION #2
> > > Use the --enable-two-phase switch to enable two_phase.
> > >
> >
> > Fixed the given comments. The attached patch contains the required changes.
> >
>
> Hi Shubham,
>
> I have a comment for the v12 patch.
> I think we can pass just the two_phase option instead of the whole
> structure here. As we are just using 'opt->two_phase'.
>
> +check_publisher(const struct LogicalRepInfo *dbinfo,
> + const struct CreateSubscriberOptions *opt)
>

Fixed the given comment. The attached patch contains the required changes.

Thanks and regards,
Shubham Khanna.

On Mon, Jan 20, 2025 at 12:00 PM Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
>
> Hi Shubham,
>
> Patch v12-0001 LGTM.
>
> BTW, there is a precedent for passing the 'opt' arg but then only
> using one member from it. See function wait_for_end_recovery in this
> same file. So whether you decide to change the code per Shlok's review
> [1], or decide not to change it -- either way is OK for me.
>
> ======
> [1] https://www.postgresql.org/message-id/CANhcyEXQ1h%3DoSPFFziCZuU6far6a82DQafL0S85CyVRyEntA%2Bw%40mail.gmail.com
>

I have incorporated Shlok's suggestion [1] into the patch and made the
necessary updates.
The attached v13 patch at [2] contains the updated changes.

[1] https://www.postgresql.org/message-id/CANhcyEXQ1h%3DoSPFFziCZuU6far6a82DQafL0S85CyVRyEntA%2Bw%40mail.gmail.com
[2] https://www.postgresql.org/message-id/CAHv8RjJfaHFMeDkHOdMpLUHCeDN%2Be5p0CsdQxcKEOiWRnRjxfg%40mail.gmail.com

Thanks and regards,
Shubham Khanna.

Hi,

Following the recent comments on Patch v13-0001, the patch was not
applied to the HEAD. I have addressed the feedback and prepared a
rebased version to incorporate the necessary adjustments.
Please find the updated patch.

Thanks and regards,
Shubham Khanna.

Patch v14-0001 LGTM

======
Kind Regards,
Peter Smith.
Fujitsu Australia

On Wed, 22 Jan 2025 at 16:23, Shubham Khanna
<khannashubham1197(at)gmail(dot)com> wrote:
>
> Hi,
>
> Following the recent comments on Patch v13-0001, the patch was not
> applied to the HEAD. I have addressed the feedback and prepared a
> rebased version to incorporate the necessary adjustments.
> Please find the updated patch.
>
> Thanks and regards,
> Shubham Khanna.

Hi Shubham,

I reviewed the patch and it looks good to me.

Thanks and Regards,
Shlok Kyal

On Wed, 22 Jan 2025 at 16:23, Shubham Khanna
<khannashubham1197(at)gmail(dot)com> wrote:
>
> Hi,
>
> Following the recent comments on Patch v13-0001, the patch was not
> applied to the HEAD. I have addressed the feedback and prepared a
> rebased version to incorporate the necessary adjustments.
> Please find the updated patch.

The patch needs to be rebased:
git am v14-0001-Add-support-for-two-phase-commit-in-pg_createsub.patch
Applying: Add support for two-phase commit in pg_createsubscriber
error: patch failed: src/bin/pg_basebackup/pg_createsubscriber.c:1326
error: src/bin/pg_basebackup/pg_createsubscriber.c: patch does not apply

Regards,
Vignesh

On Mon, Feb 17, 2025 at 9:43 PM vignesh C <vignesh21(at)gmail(dot)com> wrote:
>
> On Wed, 22 Jan 2025 at 16:23, Shubham Khanna
> <khannashubham1197(at)gmail(dot)com> wrote:
> >
> > Hi,
> >
> > Following the recent comments on Patch v13-0001, the patch was not
> > applied to the HEAD. I have addressed the feedback and prepared a
> > rebased version to incorporate the necessary adjustments.
> > Please find the updated patch.
>
> The patch needs to be rebased:
> git am v14-0001-Add-support-for-two-phase-commit-in-pg_createsub.patch
> Applying: Add support for two-phase commit in pg_createsubscriber
> error: patch failed: src/bin/pg_basebackup/pg_createsubscriber.c:1326
> error: src/bin/pg_basebackup/pg_createsubscriber.c: patch does not apply
>

I have addressed the feedback and prepared a rebased version to
incorporate the necessary adjustments.
Please find the updated patch.

Thanks and regards,
Shubham Khanna.

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?

--
With Regards,
Amit Kapila.

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.

On Mon, Feb 24, 2025 at 2:51 PM Shubham Khanna
<khannashubham1197(at)gmail(dot)com> wrote:
>
> The attached patch contains the suggested changes.
>

Pushed with minor changes. In the latest patch, you have incorrectly
copied one of the checks from the previous version patch which I have
fixed before pushing the patch.

--
With Regards,
Amit Kapila.