On Wed, Mar 27, 2024 at 11:57 AM vignesh C wrote:
>
> The attached patch has changes to wait for regress_sub4 subscription
> to apply the changes from the publisher before verifying the data.
>
Pushed after changing the order of wait as it looks logical to wait
for regress_sub5 after enabling
Dear Vignesh,
>
> Recently there was a failure in 004_subscription tap test at [1].
> In this failure, the tab_upgraded1 table was expected to have 51
> records but has only 50 records. Before the upgrade both publisher and
> subscriber have 50 records.
Good catch!
> After the upgrade we have
On Mon, 19 Feb 2024 at 12:38, Amit Kapila wrote:
>
> On Mon, Feb 19, 2024 at 6:54 AM Hayato Kuroda (Fujitsu)
> wrote:
> >
> > Thanks for reviewing! PSA new version.
> >
>
> Pushed this after making minor changes in the comments.
Recently there was a failure in 004_subscription tap test at [1].
On Mon, Feb 19, 2024 at 6:54 AM Hayato Kuroda (Fujitsu)
wrote:
>
> Thanks for reviewing! PSA new version.
>
Pushed this after making minor changes in the comments.
--
With Regards,
Amit Kapila.
On Mon, 19 Feb 2024 at 06:54, Hayato Kuroda (Fujitsu)
wrote:
>
> Dear Vignesh,
>
> Thanks for reviewing! PSA new version.
>
> >
> > Thanks for the updated patch, Few suggestions:
> > 1) This can be moved to keep it similar to other tests:
> > +# Setup a disabled subscription. The upcoming test
Dear Vignesh,
Thanks for reviewing! PSA new version.
>
> Thanks for the updated patch, Few suggestions:
> 1) This can be moved to keep it similar to other tests:
> +# Setup a disabled subscription. The upcoming test will check the
> +# pg_createsubscriber won't work, so it is sufficient.
>
On Sat, Feb 17, 2024 at 10:05 AM vignesh C wrote:
>
> On Fri, 16 Feb 2024 at 10:50, Hayato Kuroda (Fujitsu)
> wrote:
>
> Thanks for the updated patch, Few suggestions:
> 1) This can be moved to keep it similar to other tests:
> +# Setup a disabled subscription. The upcoming test will check the
On Fri, 16 Feb 2024 at 10:50, Hayato Kuroda (Fujitsu)
wrote:
>
> Dear Vignesh,
>
> Thanks for reviewing! PSA new version.
>
> >
> > Thanks for the updated patch, few suggestions:
> > 1) Can we use a new publication for this subscription too so that the
> > publication and subscription naming will
On Fri, Feb 16, 2024 at 10:50 AM Hayato Kuroda (Fujitsu)
wrote:
>
> Thanks for reviewing! PSA new version.
>
+# Setup a disabled subscription. The upcoming test will check the
+# pg_createsubscriber won't work, so it is sufficient.
+$publisher->safe_psql('postgres', "CREATE PUBLICATION
Dear Vignesh,
Thanks for reviewing! PSA new version.
>
> Thanks for the updated patch, few suggestions:
> 1) Can we use a new publication for this subscription too so that the
> publication and subscription naming will become consistent throughout
> the test case:
> +# Table will be in 'd'
On Fri, 16 Feb 2024 at 08:22, Hayato Kuroda (Fujitsu)
wrote:
>
> Dear Amit,
>
> > This sounds like a reasonable way to address the reported problem.
>
> OK, thanks!
>
> > Justin, do let me know if you think otherwise?
> >
> > Comment:
> > ===
> > *
> > -# Setup an enabled subscription to
Dear Justin,
Thanks for replying!
> What optimizations? I can't see them, and since the patch is described
> as rearranging test cases (and therefore already difficult to read), I
> guess they should be a separate patch, or the optimizations described.
The basic idea was to reduce number of
Dear Amit,
> This sounds like a reasonable way to address the reported problem.
OK, thanks!
> Justin, do let me know if you think otherwise?
>
> Comment:
> ===
> *
> -# Setup an enabled subscription to verify that the running status and
> failover
> -# option are retained after the
On Wed, Feb 14, 2024 at 03:37:03AM +, Hayato Kuroda (Fujitsu) wrote:
> Attached patch modified the test accordingly. Also, it contains some
> optimizations.
> This can pass the test on my env:
What optimizations? I can't see them, and since the patch is described
as rearranging test cases
On Wed, Feb 14, 2024 at 9:07 AM Hayato Kuroda (Fujitsu)
wrote:
>
> > pg_upgrade/t/004_subscription.pl says
> >
> > |my $mode = $ENV{PG_TEST_PG_UPGRADE_MODE} || '--copy';
> >
> > ..but I think maybe it should not.
> >
> > When you try to use --link, it fails:
> >
Dear Vignesh,
Thanks for verifying the fix!
> Your proposal to change the tests in the following order: a) failure
> due to the insufficient max_replication_slot b) failure because the
> pg_subscription_rel has 'd' state c) successful upgrade. looks good to
> me.
Right.
> I have also verified
On Wed, 14 Feb 2024 at 09:07, Hayato Kuroda (Fujitsu)
wrote:
>
> Dear Justin,
>
> > pg_upgrade/t/004_subscription.pl says
> >
> > |my $mode = $ENV{PG_TEST_PG_UPGRADE_MODE} || '--copy';
> >
> > ..but I think maybe it should not.
> >
> > When you try to use --link, it fails:
> >
Dear Justin,
> pg_upgrade/t/004_subscription.pl says
>
> |my $mode = $ENV{PG_TEST_PG_UPGRADE_MODE} || '--copy';
>
> ..but I think maybe it should not.
>
> When you try to use --link, it fails:
> https://cirrus-ci.com/task/4669494061170688
>
> |Adding ".old" suffix to old global/pg_control
On Tue, Feb 13, 2024 at 03:05:14PM -0600, Justin Pryzby wrote:
> On Tue, Jan 02, 2024 at 03:58:25PM +0530, Amit Kapila wrote:
> > Pushed.
>
> pg_upgrade/t/004_subscription.pl says
>
> |my $mode = $ENV{PG_TEST_PG_UPGRADE_MODE} || '--copy';
>
> ..but I think maybe it should not.
>
> When you try
On Tue, Jan 02, 2024 at 03:58:25PM +0530, Amit Kapila wrote:
> Pushed.
pg_upgrade/t/004_subscription.pl says
|my $mode = $ENV{PG_TEST_PG_UPGRADE_MODE} || '--copy';
..but I think maybe it should not.
When you try to use --link, it fails:
https://cirrus-ci.com/task/4669494061170688
|Adding
On Wed, Jan 03, 2024 at 03:18:50PM +0530, Amit Kapila wrote:
> I think it would be good to finish the pending patch to improve the
> IsBinaryUpgrade check [1] which we decided to do once this patch is
> ready. Would you like to take that up or do you want me to finish it?
>
> [1] -
On Tue, 2 Jan 2024 at 15:58, Amit Kapila wrote:
>
> On Fri, Dec 29, 2023 at 2:26 PM vignesh C wrote:
> >
> > On Thu, 28 Dec 2023 at 15:59, Amit Kapila wrote:
> > >
> > > On Wed, Dec 13, 2023 at 12:09 PM vignesh C wrote:
> > > >
> > > > Thanks for the comments, the attached v25 version patch
On Wed, 3 Jan 2024 at 11:25, Amit Kapila wrote:
>
> On Wed, Jan 3, 2024 at 6:21 AM Michael Paquier wrote:
> >
> > On Tue, Jan 02, 2024 at 03:58:25PM +0530, Amit Kapila wrote:
> > > On Fri, Dec 29, 2023 at 2:26 PM vignesh C wrote:
> > >> Thanks, the changes look good.
> > >
> > > Pushed.
> >
> >
On Wed, Jan 03, 2024 at 03:18:50PM +0530, Amit Kapila wrote:
> I think it would be good to finish the pending patch to improve the
> IsBinaryUpgrade check [1] which we decided to do once this patch is
> ready. Would you like to take that up or do you want me to finish it?
>
> [1] -
On Wed, Jan 3, 2024 at 11:33 AM Michael Paquier wrote:
>
> On Wed, Jan 03, 2024 at 11:24:50AM +0530, Amit Kapila wrote:
> > I think the next possible step here is to document how to upgrade the
> > logical replication nodes as previously discussed in this thread [1].
> > IIRC, there were a few
On Wed, Jan 03, 2024 at 11:24:50AM +0530, Amit Kapila wrote:
> I think the next possible step here is to document how to upgrade the
> logical replication nodes as previously discussed in this thread [1].
> IIRC, there were a few issues with the steps mentioned but if we want
> to document those
On Wed, Jan 3, 2024 at 6:21 AM Michael Paquier wrote:
>
> On Tue, Jan 02, 2024 at 03:58:25PM +0530, Amit Kapila wrote:
> > On Fri, Dec 29, 2023 at 2:26 PM vignesh C wrote:
> >> Thanks, the changes look good.
> >
> > Pushed.
>
> Yeah! Thanks Amit and everybody involved here! Thanks also to
On Tue, Jan 02, 2024 at 03:58:25PM +0530, Amit Kapila wrote:
> On Fri, Dec 29, 2023 at 2:26 PM vignesh C wrote:
>> Thanks, the changes look good.
>
> Pushed.
Yeah! Thanks Amit and everybody involved here! Thanks also to Julien
for raising the thread and the problem, to start with.
--
Michael
On Fri, Dec 29, 2023 at 2:26 PM vignesh C wrote:
>
> On Thu, 28 Dec 2023 at 15:59, Amit Kapila wrote:
> >
> > On Wed, Dec 13, 2023 at 12:09 PM vignesh C wrote:
> > >
> > > Thanks for the comments, the attached v25 version patch has the
> > > changes for the same.
> > >
> >
> > I have looked at
On Thu, 28 Dec 2023 at 15:59, Amit Kapila wrote:
>
> On Wed, Dec 13, 2023 at 12:09 PM vignesh C wrote:
> >
> > Thanks for the comments, the attached v25 version patch has the
> > changes for the same.
> >
>
> I have looked at it again and made some cosmetic changes like changing
> some comments
On Wed, Dec 13, 2023 at 12:09 PM vignesh C wrote:
>
> Thanks for the comments, the attached v25 version patch has the
> changes for the same.
>
I have looked at it again and made some cosmetic changes like changing
some comments and a minor change in one of the error messages. See, if
the
On Wed, 13 Dec 2023 at 01:56, Masahiko Sawada wrote:
>
> On Thu, Dec 7, 2023 at 8:15 PM vignesh C wrote:
> >
> > On Tue, 5 Dec 2023 at 10:56, Michael Paquier wrote:
> > >
> > > On Mon, Dec 04, 2023 at 04:30:49PM +0530, Amit Kapila wrote:
> > > > I have made minor changes in the comments and
On Thu, Dec 7, 2023 at 8:15 PM vignesh C wrote:
>
> On Tue, 5 Dec 2023 at 10:56, Michael Paquier wrote:
> >
> > On Mon, Dec 04, 2023 at 04:30:49PM +0530, Amit Kapila wrote:
> > > I have made minor changes in the comments and code at various places.
> > > See and let me know if you are not happy
On Thu, 7 Dec 2023 at 07:20, Masahiko Sawada wrote:
>
> On Mon, Dec 4, 2023 at 8:01 PM Amit Kapila wrote:
> >
> > On Fri, Dec 1, 2023 at 11:24 PM vignesh C wrote:
> > >
> > > The attached v22 version patch has the changes for the same.
> > >
> >
> > I have made minor changes in the comments and
On Tue, 5 Dec 2023 at 10:56, Michael Paquier wrote:
>
> On Mon, Dec 04, 2023 at 04:30:49PM +0530, Amit Kapila wrote:
> > I have made minor changes in the comments and code at various places.
> > See and let me know if you are not happy with the changes. I think
> > unless there are more
On Thursday, December 7, 2023 10:23 AM Amit Kapila
wrote:
>
> On Thu, Dec 7, 2023 at 7:26 AM Masahiko Sawada
> wrote:
> >
> > On Tue, Dec 5, 2023 at 6:37 PM Amit Kapila
> wrote:
> > >
> > > On Tue, Dec 5, 2023 at 10:56 AM Michael Paquier
> wrote:
> > > >
> > > > On Mon, Dec 04, 2023 at
On Thu, Dec 7, 2023 at 7:26 AM Masahiko Sawada wrote:
>
> On Tue, Dec 5, 2023 at 6:37 PM Amit Kapila wrote:
> >
> > On Tue, Dec 5, 2023 at 10:56 AM Michael Paquier wrote:
> > >
> > > On Mon, Dec 04, 2023 at 04:30:49PM +0530, Amit Kapila wrote:
> > > > I have made minor changes in the comments
On Tue, Dec 5, 2023 at 6:37 PM Amit Kapila wrote:
>
> On Tue, Dec 5, 2023 at 10:56 AM Michael Paquier wrote:
> >
> > On Mon, Dec 04, 2023 at 04:30:49PM +0530, Amit Kapila wrote:
> > > I have made minor changes in the comments and code at various places.
> > > See and let me know if you are not
On Mon, Dec 4, 2023 at 8:01 PM Amit Kapila wrote:
>
> On Fri, Dec 1, 2023 at 11:24 PM vignesh C wrote:
> >
> > The attached v22 version patch has the changes for the same.
> >
>
> I have made minor changes in the comments and code at various places.
> See and let me know if you are not happy
On Tue, Dec 5, 2023 at 10:56 AM Michael Paquier wrote:
>
> On Mon, Dec 04, 2023 at 04:30:49PM +0530, Amit Kapila wrote:
> > I have made minor changes in the comments and code at various places.
> > See and let me know if you are not happy with the changes. I think
> > unless there are more
On Mon, Dec 04, 2023 at 04:30:49PM +0530, Amit Kapila wrote:
> I have made minor changes in the comments and code at various places.
> See and let me know if you are not happy with the changes. I think
> unless there are more suggestions or comments, we can proceed with
> committing it.
Yeah. I
On Fri, Dec 1, 2023 at 11:24 PM vignesh C wrote:
>
> The attached v22 version patch has the changes for the same.
>
I have made minor changes in the comments and code at various places.
See and let me know if you are not happy with the changes. I think
unless there are more suggestions or
On Fri, 1 Dec 2023 at 10:57, Peter Smith wrote:
>
> Here are review comments for patch v21-0001
>
> ==
> src/bin/pg_upgrade/check.c
>
> 1. check_old_cluster_subscription_state
>
> +/*
> + * check_old_cluster_subscription_state()
> + *
> + * Verify that each of the subscriptions has all their
On Fri, Dec 1, 2023 at 10:57 AM Peter Smith wrote:
>
> Here are review comments for patch v21-0001
>
>
> 2.
> In this function there are a couple of errors written to the
> "subs_invalid.txt" file:
>
> + fprintf(script, "replication origin is missing for database:\"%s\"
> subscription:\"%s\"\n",
Here are review comments for patch v21-0001
==
src/bin/pg_upgrade/check.c
1. check_old_cluster_subscription_state
+/*
+ * check_old_cluster_subscription_state()
+ *
+ * Verify that each of the subscriptions has all their corresponding tables in
+ * i (initialize) or r (ready).
+ */
+static
On Thu, 30 Nov 2023 at 13:35, Amit Kapila wrote:
>
> On Wed, Nov 29, 2023 at 3:02 PM Amit Kapila wrote:
> >
>
> In general, the test cases are a bit complex to understand, so, it
> will be difficult to enhance these later. The complexity comes from
> the fact that one upgrade test is trying to
On Thu, 30 Nov 2023 at 06:37, Peter Smith wrote:
>
> Here are some review comments for patch v20-0001
>
> ==
>
> 1. getSubscriptions
>
> + if (dopt->binary_upgrade && fout->remoteVersion >= 17)
> + appendPQExpBufferStr(query, " s.subenabled\n");
> + else
> + appendPQExpBufferStr(query, "
On Wed, 29 Nov 2023 at 15:02, Amit Kapila wrote:
>
> On Tue, Nov 28, 2023 at 4:12 PM vignesh C wrote:
> >
>
> Few comments on the latest patch:
> ===
> 1.
> + if (fout->remoteVersion >= 17)
> + appendPQExpBufferStr(query, " o.remote_lsn AS suboriginremotelsn,\n");
> +
On Wed, Nov 29, 2023 at 3:02 PM Amit Kapila wrote:
>
In general, the test cases are a bit complex to understand, so, it
will be difficult to enhance these later. The complexity comes from
the fact that one upgrade test is trying to test multiple things (a)
Enabled/Disabled subscriptions; (b)
On Thu, Nov 30, 2023 at 6:37 AM Peter Smith wrote:
>
> Here are some review comments for patch v20-0001
>
> ==
>
> 1. getSubscriptions
>
> + if (dopt->binary_upgrade && fout->remoteVersion >= 17)
> + appendPQExpBufferStr(query, " s.subenabled\n");
> + else
> + appendPQExpBufferStr(query,
On Thu, Nov 30, 2023 at 12:06 PM Peter Smith wrote:
>
> Here are some review comments for patch v20-0001
>
> 3.
> +# The subscription's running status should be preserved
> +my $result =
> + $new_sub->safe_psql('postgres',
> + "SELECT subenabled FROM pg_subscription WHERE subname =
Here are some review comments for patch v20-0001
==
1. getSubscriptions
+ if (dopt->binary_upgrade && fout->remoteVersion >= 17)
+ appendPQExpBufferStr(query, " s.subenabled\n");
+ else
+ appendPQExpBufferStr(query, " false AS subenabled\n");
Probably I misunderstood this logic...
On Tue, Nov 28, 2023 at 4:12 PM vignesh C wrote:
>
Few comments on the latest patch:
===
1.
+ if (fout->remoteVersion >= 17)
+ appendPQExpBufferStr(query, " o.remote_lsn AS suboriginremotelsn,\n");
+ else
+ appendPQExpBufferStr(query, " NULL AS
On Mon, 27 Nov 2023 at 06:53, Peter Smith wrote:
>
> Here are some review comments for patch set v19*
>
> //
>
> v19-0001.
>
> No comments
>
> ///
>
> v19-0002.
>
> (I saw that both changes below seemed cut/paste from similar
> functions, but I will ask the questions anyway).
>
> ==
>
On Sat, 25 Nov 2023 at 17:50, Amit Kapila wrote:
>
> 2.
> + * b) SUBREL_STATE_SYNCDONE: A relation upgraded while in this state
> + * would retain the replication origin in certain cases.
>
> I think this is vague. Can we briefly describe cases where the origins
> would be retained?
Modified
>
On Mon, 27 Nov 2023 at 17:12, Amit Kapila wrote:
>
> On Mon, Nov 27, 2023 at 3:18 PM vignesh C wrote:
> >
> > On Sat, 25 Nov 2023 at 17:50, Amit Kapila wrote:
> > >
> > > On Sat, Nov 25, 2023 at 7:21 AM vignesh C wrote:
> > > >
> > >
> > > Few comments on v19:
> > > ==
> > > 1.
On Mon, Nov 27, 2023 at 3:18 PM vignesh C wrote:
>
> On Sat, 25 Nov 2023 at 17:50, Amit Kapila wrote:
> >
> > On Sat, Nov 25, 2023 at 7:21 AM vignesh C wrote:
> > >
> >
> > Few comments on v19:
> > ==
> > 1.
> > +
> > + The subscriptions will be migrated to the new
On Sat, 25 Nov 2023 at 17:50, Amit Kapila wrote:
>
> On Sat, Nov 25, 2023 at 7:21 AM vignesh C wrote:
> >
>
> Few comments on v19:
> ==
> 1.
> +
> + The subscriptions will be migrated to the new cluster in a disabled
> state.
> + After migration, do this:
> +
> +
Here are some review comments for patch set v19*
//
v19-0001.
No comments
///
v19-0002.
(I saw that both changes below seemed cut/paste from similar
functions, but I will ask the questions anyway).
==
src/backend/commands/subscriptioncmds.c
1.
+/* Potentially set by
On Sat, Nov 25, 2023 at 7:21 AM vignesh C wrote:
>
Few comments on v19:
==
1.
+
+ The subscriptions will be migrated to the new cluster in a disabled state.
+ After migration, do this:
+
+
+
+
+
+ Enable the subscriptions by executing
+
On Mon, 20 Nov 2023 at 05:27, Michael Paquier wrote:
>
> On Sun, Nov 19, 2023 at 06:56:05AM +0530, vignesh C wrote:
> > On Sun, 19 Nov 2023 at 06:52, vignesh C wrote:
> >> On Fri, 10 Nov 2023 at 19:26, vignesh C wrote:
> >>> I will analyze more on this and post the analysis in the subsequent
On Fri, 24 Nov 2023 at 07:00, Peter Smith wrote:
>
> I have only trivial review comments for patch v18-0001
>
> ==
> src/bin/pg_upgrade/check.c
>
> 1. check_new_cluster_subscription_configuration
>
> + /*
> + * A slot not created yet refers to the 'i' (initialize) state, while
> + * 'r'
I have only trivial review comments for patch v18-0001
==
src/bin/pg_upgrade/check.c
1. check_new_cluster_subscription_configuration
+ /*
+ * A slot not created yet refers to the 'i' (initialize) state, while
+ * 'r' (ready) state refer to a slot created previously but already
+ * dropped.
On Thu, 23 Nov 2023 at 05:56, Peter Smith wrote:
>
> Here are some review comments for patch v17-0001
>
> ==
> src/bin/pg_dump/pg_dump.c
>
> 1. getSubscriptionTables
>
> +/*
> + * getSubscriptionTables
> + * Get information about subscription membership for dumpable tables. This
> + *
On Tue, 21 Nov 2023 at 07:11, Michael Paquier wrote:
>
> On Mon, Nov 20, 2023 at 09:49:41AM +0530, Amit Kapila wrote:
> > On Tue, Nov 14, 2023 at 7:21 AM vignesh C wrote:
> >> There are couple of things happening here: a) In the first part we
> >> take care of setting subscription relation to
Here are some review comments for patch v17-0001
==
src/bin/pg_dump/pg_dump.c
1. getSubscriptionTables
+/*
+ * getSubscriptionTables
+ * Get information about subscription membership for dumpable tables. This
+ *will be used only in binary-upgrade mode and for PG17 or later versions.
On Wed, 22 Nov 2023 at 06:48, Peter Smith wrote:
> ==
> doc/src/sgml/ref/pgupgrade.sgml
>
> 1.
> +
> + Create all the new tables that were created in the publication during
> + upgrade and refresh the publication by executing
> + ALTER
> SUBSCRIPTION ... REFRESH
Thanks for addressing my past review comments.
Here are some more review comments for patch v16-0001
==
doc/src/sgml/ref/pgupgrade.sgml
1.
+
+ Create all the new tables that were created in the publication during
+ upgrade and refresh the publication by executing
+
On Mon, 20 Nov 2023 at 10:44, Peter Smith wrote:
>
> Here are some review comments for patch v15-0001
>
> ==
> src/bin/pg_dump/pg_dump.c
>
> 1. getSubscriptions
>
> + if (fout->remoteVersion >= 17)
> + appendPQExpBufferStr(query, "o.remote_lsn AS suboriginremotelsn\n");
> + else
> +
On Mon, Nov 20, 2023 at 09:49:41AM +0530, Amit Kapila wrote:
> On Tue, Nov 14, 2023 at 7:21 AM vignesh C wrote:
>> There are couple of things happening here: a) In the first part we
>> take care of setting subscription relation to SYNCDONE and dropping
>> the replication slot at publisher node,
Here are some review comments for patch v15-0001
==
src/bin/pg_dump/pg_dump.c
1. getSubscriptions
+ if (fout->remoteVersion >= 17)
+ appendPQExpBufferStr(query, "o.remote_lsn AS suboriginremotelsn\n");
+ else
+ appendPQExpBufferStr(query, "NULL AS suboriginremotelsn\n");
+
There should
On Tue, Nov 14, 2023 at 7:21 AM vignesh C wrote:
>
> On Mon, 13 Nov 2023 at 13:52, Michael Paquier wrote:
> >
> > Anyway, after a closer lookup, I think that your conclusions regarding
> > the states that are allowed in the patch during the upgrade have some
> > flaws.
> >
> > First, are you
On Sun, Nov 19, 2023 at 06:56:05AM +0530, vignesh C wrote:
> On Sun, 19 Nov 2023 at 06:52, vignesh C wrote:
>> On Fri, 10 Nov 2023 at 19:26, vignesh C wrote:
>>> I will analyze more on this and post the analysis in the subsequent mail.
>>
>> I analyzed further and felt that retaining
On Thu, 16 Nov 2023 at 18:25, Hayato Kuroda (Fujitsu)
wrote:
>
> Dear Vignesh,
>
> Thanks for updating the patch! Here are some comments.
> They are mainly cosmetic because I have not read yours these days.
>
> 01. binary_upgrade_add_sub_rel_state()
>
> ```
> +/* We must check these things
On Thu, 16 Nov 2023 at 07:45, Peter Smith wrote:
>
> Here are some review comments for patch v14-0001
>
> ==
> src/backend/utils/adt/pg_upgrade_support.c
>
> 1. binary_upgrade_replorigin_advance
>
> + /* lock to prevent the replication origin from vanishing */
> +
On Sun, 19 Nov 2023 at 06:52, vignesh C wrote:
>
> On Fri, 10 Nov 2023 at 19:26, vignesh C wrote:
> >
> > On Thu, 9 Nov 2023 at 12:23, Michael Paquier wrote:
> > >
> >
> > > Note: actually, this would be OK if we are able to keep the OIDs of
> > > the subscribers consistent across upgrades?
On Fri, 10 Nov 2023 at 19:26, vignesh C wrote:
>
> On Thu, 9 Nov 2023 at 12:23, Michael Paquier wrote:
> >
>
> > Note: actually, this would be OK if we are able to keep the OIDs of
> > the subscribers consistent across upgrades? I'm OK to not do nothing
> > about that in this patch, to keep it
Dear Vignesh,
Thanks for updating the patch! Here are some comments.
They are mainly cosmetic because I have not read yours these days.
01. binary_upgrade_add_sub_rel_state()
```
+/* We must check these things before dereferencing the arguments */
+if (PG_ARGISNULL(0) || PG_ARGISNULL(1)
Here are some review comments for patch v14-0001
==
src/backend/utils/adt/pg_upgrade_support.c
1. binary_upgrade_replorigin_advance
+ /* lock to prevent the replication origin from vanishing */
+ LockRelationOid(ReplicationOriginRelationId, RowExclusiveLock);
+ originid =
On Mon, 13 Nov 2023 at 17:49, Amit Kapila wrote:
>
> On Mon, Nov 13, 2023 at 5:01 PM Amit Kapila wrote:
> >
> > On Fri, Nov 10, 2023 at 7:26 PM vignesh C wrote:
> > >
> > > Thanks for the comments, the attached v13 version patch has the
> > > changes for the same.
> > >
> >
> > +
> > +
On Mon, 13 Nov 2023 at 17:02, Amit Kapila wrote:
>
> On Fri, Nov 10, 2023 at 7:26 PM vignesh C wrote:
> >
> > Thanks for the comments, the attached v13 version patch has the
> > changes for the same.
> >
>
> +
> + ReplicationOriginNameForLogicalRep(subid, InvalidOid, originname,
>
On Mon, 13 Nov 2023 at 13:52, Peter Smith wrote:
>
> Here are some review comments for patch v13-0001
>
> ==
> src/bin/pg_dump/pg_dump.c
>
> 1. getSubscriptionTables
>
> + int i_srsublsn;
> + int i;
> + int cur_rel = 0;
> + int ntups;
>
> What is the difference between 'i' and 'cur_rel'?
>
>
On Tue, Nov 14, 2023 at 5:52 AM Michael Paquier wrote:
>
> On Mon, Nov 13, 2023 at 04:02:27PM +0530, Amit Kapila wrote:
> > On Mon, Nov 13, 2023 at 1:52 PM Michael Paquier wrote:
> >> It seems to me that INIT cannot be relied on for a similar reason.
> >> This state would be set for a new
On Mon, 13 Nov 2023 at 13:52, Michael Paquier wrote:
>
> On Fri, Nov 10, 2023 at 07:26:18PM +0530, vignesh C wrote:
> > I did testing in the same lines that you mentioned. Apart from that I
> > also reviewed the design where it was using the old subscription id
> > like in case of table sync
On Mon, Nov 13, 2023 at 04:02:27PM +0530, Amit Kapila wrote:
> On Mon, Nov 13, 2023 at 1:52 PM Michael Paquier wrote:
>> It seems to me that INIT cannot be relied on for a similar reason.
>> This state would be set for a new relation in
>> LogicalRepSyncTableStart(), and the relation would still
On Mon, Nov 13, 2023 at 5:01 PM Amit Kapila wrote:
>
> On Fri, Nov 10, 2023 at 7:26 PM vignesh C wrote:
> >
> > Thanks for the comments, the attached v13 version patch has the
> > changes for the same.
> >
>
> +
> + ReplicationOriginNameForLogicalRep(subid, InvalidOid, originname,
>
On Fri, Nov 10, 2023 at 7:26 PM vignesh C wrote:
>
> Thanks for the comments, the attached v13 version patch has the
> changes for the same.
>
+
+ ReplicationOriginNameForLogicalRep(subid, InvalidOid, originname,
sizeof(originname));
+ originid = replorigin_by_name(originname, false);
+
On Mon, Nov 13, 2023 at 1:52 PM Michael Paquier wrote:
>
> It seems to me that INIT cannot be relied on for a similar reason.
> This state would be set for a new relation in
> LogicalRepSyncTableStart(), and the relation would still be in INIT
> state when creating the slot via
On Fri, Nov 10, 2023 at 07:26:18PM +0530, vignesh C wrote:
> I did testing in the same lines that you mentioned. Apart from that I
> also reviewed the design where it was using the old subscription id
> like in case of table sync workers, the tables sync worker will use
> replication using old
Here are some review comments for patch v13-0001
==
src/bin/pg_dump/pg_dump.c
1. getSubscriptionTables
+ int i_srsublsn;
+ int i;
+ int cur_rel = 0;
+ int ntups;
What is the difference between 'i' and 'cur_rel'?
AFAICT these represent the same tuple index, in which case you might
as well
On Thu, 9 Nov 2023 at 07:44, Peter Smith wrote:
>
> Thanks for addressing my previous review comments.
>
> I re-checked the latest patch v12-0001 and found the following:
>
> ==
> Commit message
>
> 1.
> The new SQL binary_upgrade_create_sub_rel_state function has the following
> syntax:
>
On Thu, 9 Nov 2023 at 12:23, Michael Paquier wrote:
>
> On Thu, Nov 09, 2023 at 01:14:05PM +1100, Peter Smith wrote:
> > Looks like v12 accidentally forgot to update this to the modified
> > function name 'binary_upgrade_add_sub_rel_state'
>
> This v12 is overall cleaner than its predecessors.
On Thu, Nov 09, 2023 at 01:14:05PM +1100, Peter Smith wrote:
> Looks like v12 accidentally forgot to update this to the modified
> function name 'binary_upgrade_add_sub_rel_state'
This v12 is overall cleaner than its predecessors. Nice to see.
+my $result = $publisher->safe_psql('postgres',
On Wed, Nov 8, 2023 at 10:52 PM vignesh C wrote:
>
> Upgrading 2 node circular logical replication cluster:
> 1) Let's say we have a circular logical replication setup Node1->Node2
> & Node2->Node1. Here Node2 is subscribing to Node1 and Node1 is
> subscribing to Node2.
> 2) Stop the server in
Thanks for addressing my previous review comments.
I re-checked the latest patch v12-0001 and found the following:
==
Commit message
1.
The new SQL binary_upgrade_create_sub_rel_state function has the following
syntax:
SELECT binary_upgrade_create_sub_rel_state(subname text, relid oid,
On Wed, Nov 08, 2023 at 10:52:29PM +0530, vignesh C wrote:
> Upgrading logical replication nodes requires multiple steps to be
> performed. Because not all operations are transactional, the user is
> advised to take backups.
> Backups can be taken as described in
>
On Thu, 2 Nov 2023 at 17:01, Amit Kapila wrote:
>
> On Thu, Nov 2, 2023 at 3:41 PM vignesh C wrote:
> >
> > I have slightly modified it now and also made it consistent with the
> > replication slot upgrade, but I was not sure if we need to add
> > anything more. Let me know if anything else
On Mon, 6 Nov 2023 at 07:51, Peter Smith wrote:
>
> Here are some review comments for patch v11-0001
>
> ==
> Commit message
>
> 1.
> The subscription's replication origin are needed to ensure
> that we don't replicate anything twice.
>
> ~
>
> /are needed/is needed/
Modified
>
> 2.
>
Here are some review comments for patch v11-0001
==
Commit message
1.
The subscription's replication origin are needed to ensure
that we don't replicate anything twice.
~
/are needed/is needed/
~~~
2.
Author: Julien Rouhaud
Reviewed-by: FIXME
Discussion:
On Thu, Nov 02, 2023 at 05:00:55PM +0530, Amit Kapila wrote:
> I think it is important for users to know how they upgrade their
> multi-node setup. Say a two-node setup where replication is working
> both ways (aka each node has both publications and subscriptions),
> similarly, how to upgrade, if
1 - 100 of 205 matches
Mail list logo
