Re: Fix assert failure when decoding XLOG_PARAMETER_CHANGE on primary

On Fri, Feb 7, 2025 at 12:11 AM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
>
> On Wed, Feb 5, 2025 at 10:17 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> >
> > On Thu, Feb 6, 2025 at 12:30 AM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
> > >
> > > I've updated the patch accordingly.
> > >
> >
> > Today, again thinking about the proposed fix, I was wondering about
> > the following case. Say, on hot_standby, the user created a logical
> > slot, then shut down hot_standby, turn off the hot_standby flag, and
> > restart standby. This is allowed today but not after the patch. It is
> > possible that the user can promote a non-hot_standby server after its
> > restart in the previous step and can reuse the logical slot. So, is it
> > okay to change this behavior? If not, then we may need to go back to
> > the first fix proposed by you in this thread.
>
> While non hot standby, no one can advance logical slots, meaning the
> standby server ends up accumulating WAL records and also causing the
> bloat on the primary if hot_standby_feedback is enabled. Given this
> outcome, I though that the current patch approach would be reasonable.
>

Agreed as I also can't think of any case where the user would require
a logical slot on a non-hotstandby server.

> On the other hand, as you pointed out, it would not be good in terms
> of compatibility as we end up limiting potentially existing use cases.
> And it would be prefectly fine if users promote the standby server
> shortly after starting up with hot_standby = off.

True but it sounds like there is more harm than benefit. It seems
reasonable to do this on HEAD. Shall we think of doing it differently
in HEAD and back-branches or let's restrict as your v2 patch is doing
and if by any chance users complain about it we can try to fix it in
another way?

>
> But, similarly, is
> there any concerns of my proposed fix?
>

- if (InRecovery && InHotStandby &&
+ if (InRecovery &&
xlrec.wal_level < WAL_LEVEL_LOGICAL &&

Won't the InRecovery be true even for crash-recovery as well? I think
we need to check standby mode here.

--
With Regards,
Amit Kapila.