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 b
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 confli
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 upgradi
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 s
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 orig
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 r
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 "Rea
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 "Rea
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 if
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 some
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 c
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 forward
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?
>
Prevent
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 b/src/backen
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 check_o
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
> > +++ b/src/backend/
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 @@ check_wal_segment_size
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 think
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) th
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 silently
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
th
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 doing
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 to
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 worker
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 bot
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 s
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 n
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 name
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.
>
> > + GUC_check
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 tha
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 se
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 preve
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 sam
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 er
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 serve
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 t
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 value
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 ca
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 the
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 @@
> InvalidatePossibly
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 @@
> > > > InvalidatePoss
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 t
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?
> >
> > +errm
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 && SlotIsLogica
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)
> + {
> + e
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 mode"
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)
> + {
> + e
Hello.
Some messages recently introduced by commit 29d0a77fa6 seem to deviate
slightly from our standards.
+ if (*invalidated && SlotIsLogical(s) && IsBinaryUpgrade)
+ {
+ ereport(ERROR,
+
errcode(ERRCODE_INV
64 matches
Mail list logo
