Обсуждение: Re: Adding a '--two-phase' option to 'pg_createsubscriber' utility.
On Mon, 9 Dec 2024 at 16:25, Shubham Khanna <khannashubham1197@gmail.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
On Tue, 10 Dec 2024 at 17:17, Shubham Khanna
<khannashubham1197@gmail.com> wrote:
>
> On Mon, Dec 9, 2024 at 7:43 PM vignesh C <vignesh21@gmail.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 10:59 AM vignesh C <vignesh21@gmail.com> wrote:
>
> On Tue, 10 Dec 2024 at 17:17, Shubham Khanna
> <khannashubham1197@gmail.com> wrote:
> >
> > On Mon, Dec 9, 2024 at 7:43 PM vignesh C <vignesh21@gmail.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.
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
RE: Adding a '--two-phase' option to 'pg_createsubscriber' utility.
От
"Hayato Kuroda (Fujitsu)"
Дата:
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.hayato@fujitsu.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 8:14 AM Hayato Kuroda (Fujitsu)
<kuroda.hayato@fujitsu.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@gmail.com> wrote:
>
> On Thu, 12 Dec 2024 at 08:14, Hayato Kuroda (Fujitsu)
> <kuroda.hayato@fujitsu.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
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, 13 Dec 2024 at 13:01, Shubham Khanna
<khannashubham1197@gmail.com> wrote:
>
> On Fri, Dec 13, 2024 at 12:20 PM Peter Smith <smithpb2250@gmail.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
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@gmail.com> wrote:
>
> On Fri, Dec 13, 2024 at 2:39 PM vignesh C <vignesh21@gmail.com> wrote:
> >
> > On Fri, 13 Dec 2024 at 13:01, Shubham Khanna
> > <khannashubham1197@gmail.com> wrote:
> > >
> > > On Fri, Dec 13, 2024 at 12:20 PM Peter Smith <smithpb2250@gmail.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 5:36 PM Shubham Khanna <khannashubham1197@gmail.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@gmail.com> wrote: > > On Fri, Dec 27, 2024 at 5:36 PM Shubham Khanna > <khannashubham1197@gmail.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@gmail.com> wrote:
>
> On Fri, Dec 27, 2024 at 11:30 AM vignesh C <vignesh21@gmail.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 Wed, Jan 15, 2025 at 5:33 PM Shubham Khanna <khannashubham1197@gmail.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@gmail.com> wrote:
>
>
>
> On Wed, Jan 15, 2025 at 5:33 PM Shubham Khanna <khannashubham1197@gmail.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_createsubscriberwith 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
willbe 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@gmail.com> wrote:
>
> On Tue, Jan 14, 2025 at 4:53 PM Shlok Kyal <shlok.kyal.oss@gmail.com> wrote:
> >
> > On Fri, 27 Dec 2024 at 12:06, Shubham Khanna
> > <khannashubham1197@gmail.com> wrote:
> > >
> > > On Fri, Dec 27, 2024 at 11:30 AM vignesh C <vignesh21@gmail.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 Thu, Jan 16, 2025 at 4:53 AM Peter Smith <smithpb2250@gmail.com> wrote:
>
> On Wed, Jan 15, 2025 at 9:24 PM Ajin Cherian <itsajin@gmail.com> wrote:
> >
> >
> >
> > On Wed, Jan 15, 2025 at 5:33 PM Shubham Khanna <khannashubham1197@gmail.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_createsubscriberwith 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
transactionswill 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.kyal.oss@gmail.com> wrote:
>
> On Wed, 15 Jan 2025 at 12:03, Shubham Khanna
> <khannashubham1197@gmail.com> wrote:
> >
> > On Tue, Jan 14, 2025 at 4:53 PM Shlok Kyal <shlok.kyal.oss@gmail.com> wrote:
> > >
> > > On Fri, 27 Dec 2024 at 12:06, Shubham Khanna
> > > <khannashubham1197@gmail.com> wrote:
> > > >
> > > > On Fri, Dec 27, 2024 at 11:30 AM vignesh C <vignesh21@gmail.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, 17 Jan 2025 at 09:52, Shubham Khanna
<khannashubham1197@gmail.com> wrote:
>
> On Fri, Jan 17, 2025 at 5:43 AM Peter Smith <smithpb2250@gmail.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 12:00 PM Peter Smith <smithpb2250@gmail.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.
Patch v14-0001 LGTM ====== Kind Regards, Peter Smith. Fujitsu Australia
On Wed, 22 Jan 2025 at 16:23, Shubham Khanna <khannashubham1197@gmail.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@gmail.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 Tue, Feb 18, 2025 at 12:16 AM Shubham Khanna
<khannashubham1197@gmail.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.kapila16@gmail.com> wrote: > > On Tue, Feb 18, 2025 at 12:16 AM Shubham Khanna > <khannashubham1197@gmail.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@gmail.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.