On Wed, Jul 24, 2024 at 10:16:51PM -0500, Nathan Bossart wrote:
> On Thu, Jul 25, 2024 at 08:43:03AM +0530, Amit Kapila wrote:
>>> Shall we close the open items?
>>
>> Sorry for the typo. There is only one open item corresponding to this:
>> "Subscription and slot information retrieval inefficienc
On Thu, Jul 25, 2024 at 08:43:03AM +0530, Amit Kapila wrote:
>> Shall we close the open items?
>
> Sorry for the typo. There is only one open item corresponding to this:
> "Subscription and slot information retrieval inefficiency in
> pg_upgrade" which according to me should be closed after your c
On Thu, Jul 25, 2024 at 8:41 AM Amit Kapila wrote:
>
> On Wed, Jul 24, 2024 at 10:03 PM Nathan Bossart
> wrote:
> >
> > On Wed, Jul 24, 2024 at 11:32:47AM +0530, Amit Kapila wrote:
> > > LGTM.
> >
> > Thanks for reviewing. Committed and back-patched to v17.
> >
>
> Shall we close the open items?
On Wed, Jul 24, 2024 at 10:03 PM Nathan Bossart
wrote:
>
> On Wed, Jul 24, 2024 at 11:32:47AM +0530, Amit Kapila wrote:
> > LGTM.
>
> Thanks for reviewing. Committed and back-patched to v17.
>
Shall we close the open items? I think even if we want to improve the
slot fetching/creation mechanism,
On Wed, Jul 24, 2024 at 11:32:47AM +0530, Amit Kapila wrote:
> LGTM.
Thanks for reviewing. Committed and back-patched to v17.
--
nathan
On Wed, Jul 24, 2024 at 1:25 AM Nathan Bossart wrote:
>
> On Tue, Jul 23, 2024 at 09:05:05AM +0530, Amit Kapila wrote:
> > Right, the other option would be to move it to the place where we call
> > check_old_cluster_for_valid_slots(), etc. Initially, it was kept in
> > the specific function (get_d
On Tue, Jul 23, 2024 at 09:05:05AM +0530, Amit Kapila wrote:
> Right, the other option would be to move it to the place where we call
> check_old_cluster_for_valid_slots(), etc. Initially, it was kept in
> the specific function (get_db_rel_and_slot_infos) as we were
> mainlining the count at the pe
On Mon, Jul 22, 2024 at 8:16 PM Nathan Bossart wrote:
>
> On Mon, Jul 22, 2024 at 03:45:19PM +0530, Amit Kapila wrote:
> > On Mon, Jul 22, 2024 at 7:35 AM Michael Paquier wrote:
> >> On Sat, Jul 20, 2024 at 09:03:07PM -0500, Nathan Bossart wrote:
> >> >> This is an extremely expensive way to perf
Dear Amit, Michael,
> > I am not sure to get the reason why get_old_cluster_logical_slot_infos()
> > could not be optimized, TBH. LogicalReplicationSlotHasPendingWal()
> > uses the fast forward mode where no changes are generated, hence there
> > should be no need for a dependency to a connection
On Tue, Jul 23, 2024 at 4:33 AM Michael Paquier wrote:
>
> On Mon, Jul 22, 2024 at 09:46:29AM -0500, Nathan Bossart wrote:
> > On Mon, Jul 22, 2024 at 03:45:19PM +0530, Amit Kapila wrote:
> >>
> >> Unlike subscriptions, logical slots are database-specific objects. We
> >> have some checks in the c
On Mon, Jul 22, 2024 at 09:46:29AM -0500, Nathan Bossart wrote:
> On Mon, Jul 22, 2024 at 03:45:19PM +0530, Amit Kapila wrote:
>> On Mon, Jul 22, 2024 at 7:35 AM Michael Paquier wrote:
>>> A comment in get_db_rel_and_slot_infos() becomes incorrect where
>>> get_old_cluster_logical_slot_infos() is
On Mon, Jul 22, 2024 at 03:45:19PM +0530, Amit Kapila wrote:
> On Mon, Jul 22, 2024 at 7:35 AM Michael Paquier wrote:
>> On Sat, Jul 20, 2024 at 09:03:07PM -0500, Nathan Bossart wrote:
>> >> This is an extremely expensive way to perform that check, and so I'm
>> >> wondering why we don't just do
>
On Mon, Jul 22, 2024 at 7:35 AM Michael Paquier wrote:
>
> On Sat, Jul 20, 2024 at 09:03:07PM -0500, Nathan Bossart wrote:
> >> This is an extremely expensive way to perform that check, and so I'm
> >> wondering why we don't just do
> >>
> >> SELECT count(*) FROM pg_catalog.pg_subscription;
>
On Sat, Jul 20, 2024 at 09:03:07PM -0500, Nathan Bossart wrote:
>> This is an extremely expensive way to perform that check, and so I'm
>> wondering why we don't just do
>>
>> SELECT count(*) FROM pg_catalog.pg_subscription;
>>
>> once in count_old_cluster_subscriptions().
>
> Like so...
A
On Fri, Jul 19, 2024 at 03:44:22PM -0500, Nathan Bossart wrote:
> I've been looking into optimizing pg_upgrade's once-in-each-database steps
> [0], and I noticed that we are opening a connection to every database in
> the cluster and running a query like
>
> SELECT count(*) FROM pg_catalog.p
I've been looking into optimizing pg_upgrade's once-in-each-database steps
[0], and I noticed that we are opening a connection to every database in
the cluster and running a query like
SELECT count(*) FROM pg_catalog.pg_subscription WHERE subdbid = %d;
Then, later on, we combine all of th
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 the
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 i
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].
I
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 wi
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.
> +$pu
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 regress_pu
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' (data
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 v
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 CRE
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 upg
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 (a
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:
> > https://cirrus-ci.com/task/46694940
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 t
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:
> > https://cirrus-ci.co
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 ".old
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] - https://www.pos
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 has
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] - https://www.post
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 iss
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 we
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 Julie
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 i
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 a
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 changes
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 code
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 w
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 suggestion
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 04:30:
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 an
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 ha
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 with
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 suggest
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 a
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 comment
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 c
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 v
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 te
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, " f
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) rela
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 = 'regress_sub'"
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... AFAIK
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 suboriginremotelsn,\n");
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
> 3
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 cluste
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 pg_upgrade_s
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
+ A
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 mai
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' (ready
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. T
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
> + *wil
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 SYN
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 PUBLICA
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
+ AL
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
> + append
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, on
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 sure
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 subscription
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 bef
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 */
> + LockRelationOid(ReplicationOr
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? I'm
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 s
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 = replorigin_by_name(
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.
> > >
> >
> > +
> > + Replicati
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,
> sizeof(originna
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'?
>
> A
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 relatio
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 worke
1 - 100 of 221 matches
Mail list logo
