On Thu, Jan 11, 2024 at 10:01:16AM -0500, Tom Lane wrote:
> Alvaro Herrera writes:
>> "The logical replication launcher is disabled during binary upgrades, to
>> avoid logical replication workers running on the source cluster. That
>> would cause replication origins to move forward after having
Alvaro Herrera writes:
> "The logical replication launcher is disabled during binary upgrades, to
> avoid logical replication workers running on the source cluster. That
> would cause replication origins to move forward after having been copied
> to the target cluster, potentially creating
On 2024-Jan-11, Michael Paquier wrote:
> Hence, how about something like that:
> "The logical replication launcher is disabled during binary upgrades,
> as a logical replication workers running on the cluster upgrading from
> could cause replication origins to move forward after they are copied
>
On Thu, Jan 11, 2024 at 11:25:44AM +0530, Amit Kapila wrote:
> On Thu, Jan 11, 2024 at 9:08 AM Michael Paquier wrote:
>> Hence, how about something like that:
>> "The logical replication launcher is disabled during binary upgrades,
>> as a logical replication workers running on the cluster
On Thu, Jan 11, 2024 at 9:08 AM Michael Paquier wrote:
>
> On Wed, Jan 10, 2024 at 06:02:12PM +0530, Amit Kapila wrote:
> > - if (max_logical_replication_workers == 0)
> > + /*
> > + * The logical replication launcher is disabled during binary upgrades,
> > + * as logical replication workers can
On Wed, Jan 10, 2024 at 06:02:12PM +0530, Amit Kapila wrote:
> - if (max_logical_replication_workers == 0)
> + /*
> + * The logical replication launcher is disabled during binary upgrades,
> + * as logical replication workers can stream data during the upgrade
> + * which can cause replication
On Wed, Jan 10, 2024 at 10:11 AM Michael Paquier wrote:
>
> On Wed, Nov 15, 2023 at 07:58:06AM +0530, Amit Kapila wrote:
> > I am fine with this but there is no harm in doing this before or along
> > with the main patch. As of now, I don't see any problem but as the
> > main patch is still under
On Wed, Nov 15, 2023 at 07:58:06AM +0530, Amit Kapila wrote:
> I am fine with this but there is no harm in doing this before or along
> with the main patch. As of now, I don't see any problem but as the
> main patch is still under review, so thought we could even wait for
> the patch to become
On Wed, Nov 15, 2023 at 07:58:06AM +0530, Amit Kapila wrote:
> I am fine with this but there is no harm in doing this before or along
> with the main patch. As of now, I don't see any problem but as the
> main patch is still under review, so thought we could even wait for
> the patch to become
On Mon, Nov 13, 2023 at 10:19 AM Michael Paquier wrote:
>
> On Mon, Nov 13, 2023 at 08:45:12AM +0530, Amit Kapila wrote:
> > I think we can be specific about logical replication stuff. I have not
> > done any study on autovacuum behavior related to this, so we can
> > update about it separately
On Mon, Nov 13, 2023 at 08:45:12AM +0530, Amit Kapila wrote:
> I think we can be specific about logical replication stuff. I have not
> done any study on autovacuum behavior related to this, so we can
> update about it separately if required.
Autovacuum, as far as I recall, could decide to do
On Sat, Nov 11, 2023 at 5:46 AM Michael Paquier wrote:
>
> On Fri, Nov 10, 2023 at 03:27:25PM +0530, Amit Kapila wrote:
> > I don't think this comment is correct because there won't be any apply
> > activity on the new cluster as after restoration subscriptions should
> > be disabled. On the old
On Fri, Nov 10, 2023 at 03:27:25PM +0530, Amit Kapila wrote:
> I don't think this comment is correct because there won't be any apply
> activity on the new cluster as after restoration subscriptions should
> be disabled. On the old cluster, I think one problem is that the
> origins may move
On Fri, Nov 10, 2023 at 7:50 AM Michael Paquier wrote:
>
> On Thu, Nov 09, 2023 at 04:52:32PM +0900, Michael Paquier wrote:
> > Thanks!
>
> Also, please see also a patch about switching the logirep launcher to
> rely on IsBinaryUpgrade to prevent its startup. Any thoughts about
> that?
>
On Thu, Nov 09, 2023 at 04:52:32PM +0900, Michael Paquier wrote:
> Thanks!
Also, please see also a patch about switching the logirep launcher to
rely on IsBinaryUpgrade to prevent its startup. Any thoughts about
that?
--
Michael
diff --git a/src/backend/replication/logical/launcher.c
On 2023-Nov-09, Amit Kapila wrote:
> These comments appear mostly repetitive to what is already mentioned
> in start_postmaster(). So, I have changed those referred to already
> written comments, and slightly adjusted the comments at another place.
> See attached.
I'd still rather mention
On Thu, Nov 9, 2023 at 4:09 PM Alvaro Herrera wrote:
>
> On 2023-Nov-02, Kyotaro Horiguchi wrote:
>
> > diff --git a/src/backend/access/transam/xlog.c
> > b/src/backend/access/transam/xlog.c
> > index b541be8eec..46833f6ecd 100644
> > --- a/src/backend/access/transam/xlog.c
> > +++
On 2023-Nov-02, Kyotaro Horiguchi wrote:
> diff --git a/src/backend/access/transam/xlog.c
> b/src/backend/access/transam/xlog.c
> index b541be8eec..46833f6ecd 100644
> --- a/src/backend/access/transam/xlog.c
> +++ b/src/backend/access/transam/xlog.c
> @@ -2063,6 +2063,29 @@
Dear Horiguchi-san, hackers,
> Thanks you for the comments!
Thanks for updating the patch!
I'm not sure it is intentional, but you might miss my post...I suggested to add
a
testcase.
I attached the updated version which is almost the same as Horiguchi-san's one,
but has a test. How do you
On Thu, Nov 09, 2023 at 01:12:54PM +0530, Amit Kapila wrote:
> I think it is in an email[1].
Noted.
> I can take care of this unless we see some opposition to this idea.
Thanks!
--
Michael
signature.asc
Description: PGP signature
On Thu, Nov 9, 2023 at 12:38 PM Michael Paquier wrote:
>
> On Thu, Nov 09, 2023 at 05:04:28PM +1100, Peter Smith wrote:
> > Having a GUC hook for the "max_slot_wal_keep_size" seemed OK to me. If
> > the user overrides a GUC value (admittedly, maybe there is no reason
> > why they would want to)
On Thu, Nov 09, 2023 at 05:04:28PM +1100, Peter Smith wrote:
> Having a GUC hook for the "max_slot_wal_keep_size" seemed OK to me. If
> the user overrides a GUC value (admittedly, maybe there is no reason
> why they would want to) then at least the hook will give an error,
> rather than us
At Thu, 9 Nov 2023 12:00:59 +0530, Amit Kapila wrote
in
> I have also proposed that as one of the alternatives but didn't get
> many votes. And, I think if the user is passing a special value of
> max_slot_wal_keep_size during the upgrade, it has to be a special
> case, and rejecting it upfront
On Thu, Nov 9, 2023 at 11:40 AM Kyotaro Horiguchi
wrote:
>
> At Thu, 9 Nov 2023 09:53:07 +0530, Amit Kapila
> wrote in
> > Michael, Horiguchi-San, and others, do you have any thoughts on what
> > is the best way to proceed?
>
> As I previously mentioned, I believe that if rejection is to be the
At Thu, 9 Nov 2023 09:53:07 +0530, Amit Kapila wrote
in
> Michael, Horiguchi-San, and others, do you have any thoughts on what
> is the best way to proceed?
As I previously mentioned, I believe that if rejection is to be the
course of action, it would be best to proceed with it sooner rather
On Thu, Nov 9, 2023 at 3:55 PM Michael Paquier wrote:
>
> On Thu, Nov 09, 2023 at 09:53:07AM +0530, Amit Kapila wrote:
> > On Tue, Nov 7, 2023 at 4:16 PM Amit Kapila wrote:
> >> But then we don't need the hardcoded value of
> >> max_logical_replication_workers as zero by pg_upgrade. I think
On Thu, Nov 09, 2023 at 09:53:07AM +0530, Amit Kapila wrote:
> On Tue, Nov 7, 2023 at 4:16 PM Amit Kapila wrote:
>> But then we don't need the hardcoded value of
>> max_logical_replication_workers as zero by pg_upgrade. I think doing
>> IsBinaryUpgrade for slots won't be neat, so we anyway need
On Tue, Nov 7, 2023 at 4:16 PM Amit Kapila wrote:
>
> On Tue, Nov 7, 2023 at 8:12 AM Michael Paquier wrote:
> >
> > On Tue, Nov 07, 2023 at 07:59:46AM +0530, Amit Kapila wrote:
> > > Do you mean to say that if 'IsBinaryUpgrade' is true then let's not
> > > allow to launch launcher or apply
On Tue, Nov 7, 2023 at 8:12 AM Michael Paquier wrote:
>
> On Tue, Nov 07, 2023 at 07:59:46AM +0530, Amit Kapila wrote:
> > Do you mean to say that if 'IsBinaryUpgrade' is true then let's not
> > allow to launch launcher or apply worker? If so, I guess this won't be
> > any better than prohibiting
On Tue, Nov 07, 2023 at 07:59:46AM +0530, Amit Kapila wrote:
> Do you mean to say that if 'IsBinaryUpgrade' is true then let's not
> allow to launch launcher or apply worker? If so, I guess this won't be
> any better than prohibiting at an early stage or explicitly overriding
> those with internal
On Sun, Nov 5, 2023 at 5:33 AM Michael Paquier wrote:
>
> On Fri, Nov 03, 2023 at 01:33:26PM +1100, Peter Smith wrote:
> > On Fri, Nov 3, 2023 at 1:11 PM Amit Kapila wrote:
> >> Now, that Michael also committed another similar change in commit
> >> 7021d3b176, it is better to be consistent in
On Fri, Nov 03, 2023 at 01:33:26PM +1100, Peter Smith wrote:
> On Fri, Nov 3, 2023 at 1:11 PM Amit Kapila wrote:
>> Now, that Michael also committed another similar change in commit
>> 7021d3b176, it is better to be consistent in both cases. So, either we
>
> I agree. Both patches are setting a
On Fri, Nov 3, 2023 at 1:11 PM Amit Kapila wrote:
>
> On Thu, Nov 2, 2023 at 2:36 PM Amit Kapila wrote:
> >
> > On Thu, Nov 2, 2023 at 11:32 AM Michael Paquier wrote:
> > >
> > > On Thu, Nov 02, 2023 at 02:32:07PM +1100, Peter Smith wrote:
> > > > On Thu, Nov 2, 2023 at 2:25 PM Peter Smith
>
On Thu, Nov 2, 2023 at 2:36 PM Amit Kapila wrote:
>
> On Thu, Nov 2, 2023 at 11:32 AM Michael Paquier wrote:
> >
> > On Thu, Nov 02, 2023 at 02:32:07PM +1100, Peter Smith wrote:
> > > On Thu, Nov 2, 2023 at 2:25 PM Peter Smith wrote:
> > >> Checking this patch yesterday prompted me to create a
On Thu, Nov 2, 2023 at 11:32 AM Michael Paquier wrote:
>
> On Thu, Nov 02, 2023 at 02:32:07PM +1100, Peter Smith wrote:
> > On Thu, Nov 2, 2023 at 2:25 PM Peter Smith wrote:
> >> Checking this patch yesterday prompted me to create a new thread
> >> questioning the inconsistencies of the "GUC
On Thu, Nov 02, 2023 at 02:32:07PM +1100, Peter Smith wrote:
> On Thu, Nov 2, 2023 at 2:25 PM Peter Smith wrote:
>> Checking this patch yesterday prompted me to create a new thread
>> questioning the inconsistencies of the "GUC names in messages". In
>> that thread, Tom Lane replied and gave some
On Thu, Nov 2, 2023 at 2:25 PM Peter Smith wrote:
>
> On Thu, Nov 2, 2023 at 1:58 PM Kyotaro Horiguchi
> wrote:
> >
> > Thanks you for the comments!
> >
> > At Wed, 1 Nov 2023 18:08:19 +1100, Peter Smith
> > wrote in
> > > Hi, here are some minor review comments for the v3 patch.
> > >
> > >
On Thu, Nov 2, 2023 at 1:58 PM Kyotaro Horiguchi
wrote:
>
> Thanks you for the comments!
>
> At Wed, 1 Nov 2023 18:08:19 +1100, Peter Smith wrote
> in
> > Hi, here are some minor review comments for the v3 patch.
> >
> > ==
> > src/backend/access/transam/xlog.c
>
...
> > 2.
>
> > +
Thanks you for the comments!
At Wed, 1 Nov 2023 18:08:19 +1100, Peter Smith wrote in
> Hi, here are some minor review comments for the v3 patch.
>
> ==
> src/backend/access/transam/xlog.c
> I asked ChatGPT to suggest alternative wording for that comment, and
> it came up with something
Hi, here are some minor review comments for the v3 patch.
==
src/backend/access/transam/xlog.c
1. check_max_slot_wal_keep_size
+/*
+ * GUC check_hook for max_slot_wal_keep_size
+ *
+ * If WALs required by logical replication slots are removed, the slots are
+ * unusable. While pg_upgrade
Dear Horiguchi-san,
Thanks for making the patch!
> > 4. I think a test case to hit the error in the check hook in
> > 003_logical_slots.pl will help a lot here - not only covers the code
> > but also helps demonstrate how one can reach the error.
>
> Yeah, of course. I was planning to add tests
On Tue, Oct 31, 2023 at 4:00 PM Bharath Rupireddy
wrote:
>
> > > 5. A possible problem with this check_hook approach is that it doesn't
> > > let anyone setting max_slot_wal_keep_size to a value other than -1
> > > during pg_ugprade even if someone doesn't have logical slots or
> > > doesn't want
On Tue, Oct 31, 2023 at 2:19 PM Kyotaro Horiguchi
wrote:
>
> > GUC_check_errcode(ERRCODE_INVALID_PARAMETER_VALUE);
> > GUC_check_errmsg(""\"max_slot_wal_keep_size\" must be set
> > to -1 when in binary upgrade mode");
> > GUC_check_errdetail("A value of -1
At Mon, 30 Oct 2023 14:55:01 +0530, Bharath Rupireddy
wrote in
> > I get it. I agree to go with just the assert because the GUC
> > check_hook kinda tightens the screws against setting
> > max_slot_wal_keep_size to a value other than -1 during the binary
> > upgrade,
Thanks for being on the
On Mon, Oct 30, 2023 at 2:31 PM Bharath Rupireddy
wrote:
>
Never mind. Previous message was accidentally sent before I finished
writing my comments.
> Yeah. The check_hook is called even after the param is specified in
> postgresql.conf during the upgrade, so I see no problem there.
>
> > The
Dear Bharath,
> Will the check_hook approach work correctly?
I tested by using the first version and worked well (rejected). Please see the
log which recorded the output and log. Below lines were copied from server
log and found that max_slot_wal_keep_size must not be >= 0.
```
waiting for
On Mon, Oct 30, 2023 at 1:42 PM Kyotaro Horiguchi
wrote:
>
> At Mon, 30 Oct 2023 12:38:47 +0530, Bharath Rupireddy
> wrote in
> > Will the check_hook approach work correctly? I haven't checked that by
> > myself, but I see InitializeGUCOptions() getting called before
> > IsBinaryUpgrade is set
At Mon, 30 Oct 2023 12:38:47 +0530, Bharath Rupireddy
wrote in
> Will the check_hook approach work correctly? I haven't checked that by
> myself, but I see InitializeGUCOptions() getting called before
> IsBinaryUpgrade is set to true and the passed-in config options ('c')
> are parsed.
I'm not
At Mon, 30 Oct 2023 03:36:41 +, "Zhijie Hou (Fujitsu)"
wrote in
> Thanks for the diff and I think the approach basically works.
>
> One notable behavior of this approach it will reject the GUC setting even if
> there
> are no slots on old cluster or user set the value to a big enough
At Mon, 30 Oct 2023 08:51:18 +0530, Amit Kapila wrote
in
> I think we can simply change that error message to assert if we want
> to go with the check hook idea of yours. BTW, can we add
> GUC_check_errdetail() with a better message as some of the other check
> function uses? Also, I guess we
On Mon, Oct 30, 2023 at 8:51 AM Amit Kapila wrote:
>
> > This discussion seems like a bit off from my point. I suggested adding
> > a check for that setting when IsBinaryUpgraded is true at the GUC
> > level as shown in the attached patch. I believe Álvaro made a similar
> > suggestion. While
On Monday, October 30, 2023 10:29 AM Kyotaro Horiguchi
wrote:
>
> At Fri, 27 Oct 2023 14:57:10 +0530, Amit Kapila
> wrote in
> > On Fri, Oct 27, 2023 at 2:02 PM Alvaro Herrera
> wrote:
> > >
> > > On 2023-Oct-27, Kyotaro Horiguchi wrote:
> > >
> > > > @@ -1433,8 +1433,8 @@
>
On Mon, Oct 30, 2023 at 7:58 AM Kyotaro Horiguchi
wrote:
>
> At Fri, 27 Oct 2023 14:57:10 +0530, Amit Kapila
> wrote in
> > On Fri, Oct 27, 2023 at 2:02 PM Alvaro Herrera
> > wrote:
> > >
> > > On 2023-Oct-27, Kyotaro Horiguchi wrote:
> > >
> > > > @@ -1433,8 +1433,8 @@
> > > >
At Fri, 27 Oct 2023 14:57:10 +0530, Amit Kapila wrote
in
> On Fri, Oct 27, 2023 at 2:02 PM Alvaro Herrera
> wrote:
> >
> > On 2023-Oct-27, Kyotaro Horiguchi wrote:
> >
> > > @@ -1433,8 +1433,8 @@
> > > InvalidatePossiblyObsoleteSlot(ReplicationSlotInvalidationCause cause,
> > >
On Fri, Oct 27, 2023 at 2:02 PM Alvaro Herrera wrote:
>
> On 2023-Oct-27, Kyotaro Horiguchi wrote:
>
> > @@ -1433,8 +1433,8 @@
> > InvalidatePossiblyObsoleteSlot(ReplicationSlotInvalidationCause cause,
> > {
> > ereport(ERROR,
> >
On 2023-Oct-27, Kyotaro Horiguchi wrote:
> @@ -1433,8 +1433,8 @@
> InvalidatePossiblyObsoleteSlot(ReplicationSlotInvalidationCause cause,
> {
> ereport(ERROR,
>
> errcode(ERRCODE_INVALID_PARAMETER_VALUE),
> -
At Fri, 27 Oct 2023 09:51:43 +0530, Amit Kapila wrote
in
> On Fri, Oct 27, 2023 at 9:37 AM Peter Smith wrote:
> > IIUC the only possible way to reach this error (according to the
> > comment preceding it) is by the user overriding the GUC value (which
> > was defaulted -1) on the command line.
On Fri, Oct 27, 2023 at 9:52 AM Amit Kapila wrote:
>
> > errmsg("\"max_slot_wal_keep_size\" must be set to -1 during the upgrade"),
> > errhint("Do not override \"max_slot_wal_keep_size\" using command line
> > options."));
> >
>
> But OTOH, we don't have a value of user-passed options to ensure
On Fri, Oct 27, 2023 at 9:36 AM Amit Kapila wrote:
>
> On Fri, Oct 27, 2023 at 8:52 AM Bharath Rupireddy
> wrote:
> >
> > On Fri, Oct 27, 2023 at 8:28 AM Kyotaro Horiguchi:
> > The above errhint LGTM. How about a slightly different errmsg, like
> > the following?
> >
> > +
On Fri, Oct 27, 2023 at 9:37 AM Peter Smith wrote:
>
> On Fri, Oct 27, 2023 at 1:58 PM Kyotaro Horiguchi
> wrote:
> >
> > Hello.
> >
> > Some messages recently introduced by commit 29d0a77fa6 seem to deviate
> > slightly from our standards.
> >
> > + if (*invalidated &&
On Fri, Oct 27, 2023 at 1:58 PM Kyotaro Horiguchi
wrote:
>
> Hello.
>
> Some messages recently introduced by commit 29d0a77fa6 seem to deviate
> slightly from our standards.
>
> + if (*invalidated && SlotIsLogical(s) && IsBinaryUpgrade)
> + {
> +
On Fri, Oct 27, 2023 at 8:52 AM Bharath Rupireddy
wrote:
>
> On Fri, Oct 27, 2023 at 8:28 AM Kyotaro Horiguchi:
> The above errhint LGTM. How about a slightly different errmsg, like
> the following?
>
> +errmsg("cannot invalidate replication slots when
> in binary upgrade
On Fri, Oct 27, 2023 at 8:28 AM Kyotaro Horiguchi
wrote:
>
> Hello.
>
> Some messages recently introduced by commit 29d0a77fa6 seem to deviate
> slightly from our standards.
>
> + if (*invalidated && SlotIsLogical(s) && IsBinaryUpgrade)
> + {
> +
63 matches
Mail list logo
