Re: Fix assert failure when decoding XLOG_PARAMETER_CHANGE on primary

Hi,

On Fri, Feb 07, 2025 at 11:00:39AM +0530, Amit Kapila wrote:
> 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.

Yeah. I think the bloat on the primary can occur only if there is also
a physical replication slot between the two (which is probably the majority
of the cases though).

> 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.

+1

> > 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.

yeah exactly. And if hot_standby = off is "really" needed then the option
would be to remove the logical replication slot on the standby (which is probably
a good thing to do with hot_standby = off due to the cons you mentioned above).

> 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?

I'm tempted to vote for the later so that we can maintain the same code across
branches.

Regards,

--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com