Re: [PoC] pg_upgrade: allow to upgrade publisher node

On Sun, Aug 20, 2023 at 6:49 PM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
>
> On Thu, Aug 17, 2023 at 10:31 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> >
> > >
> > > Sorry I was not clear. I meant the logical replication slots that are
> > > *not* used by logical replication, i.e., are created manually and used
> > > by third party tools that periodically consume decoded changes. As we
> > > discussed before, these slots will never be able to pass that
> > > confirmed_flush_lsn check.
> > >
> >
> > I think normally one would have a background process to periodically
> > consume changes. Won't one can use the walsender infrastructure for
> > their plugins to consume changes probably by using replication
> > protocol?
>
> Not sure.
>

I think one can use Streaming Replication Protocol to achieve it [1].

> > Also, I feel it is the plugin author's responsibility to
> > consume changes or advance slot to the required position before
> > shutdown.
>
> How does the plugin author ensure that the slot consumes all WAL
> records including shutdown_checkpoint before shutdown?
>

By using "Streaming Replication Protocol" so that walsender can take
care of it. If not, I think users should drop such slots before the
upgrade because anyway, they won't be usable after the upgrade.

> >
> > > After some thoughts, one thing we might
> > > need to consider is that in practice, the upgrade project is performed
> > > during the maintenance window and has a backup plan that revert the
> > > upgrade process, in case something bad happens. If we require the
> > > users to drop such logical replication slots, they cannot resume to
> > > use the old cluster in that case, since they would need to create new
> > > slots, missing some changes.
> > >
> >
> > Can't one keep the backup before removing slots?
>
> Yes, but restoring the back could take time.
>
> >
> > > Other checks in pg_upgrade seem to be
> > > compatibility checks that would eventually be required for the upgrade
> > > anyway. Do we need to consider this case? For example, we do that
> > > confirmed_flush_lsn check for only the slots with pgoutput plugin.
> > >
> >
> > I think one is allowed to use pgoutput plugin even for manually
> > created slots. So, such a check may not work.
>
> Right, but I thought it's a very rare case.
>

Okay, but not sure that we can ignore it.

> Since the slot's flushed_confirmed_lsn check is not a compatibility
> check unlike the existing check, I wonder if we can make it optional.
>

There are arguments both ways. Initially, the patch proposed to make
them optional by having an option like
--include-logical-replication-slots but Jonathan raised a point that
it will be more work for users and should be the default. Then we also
discussed having an option like --exclude-logical-replication-slots
but as we don't have any other similar option, it doesn't seem natural
to add such an option. Also, I am afraid, if there is no user of such
an option, it won't be worth it. BTW, how would you like to see it as
an optional (via --include or via --exclude switch)?

Personally, I am okay to make it optional if we have a broader
consensus. My preference would be to have an --exclude kind of option.
How about first getting the main patch reviewed and committed, then
based on consensus, we can decide whether to make it optional and if
so, what is the preferred way?

[1] - https://www.postgresql.org/docs/current/protocol-replication.html

--
With Regards,
Amit Kapila.