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

On Mon, Aug 7, 2023 at 2:02 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> On Mon, Aug 7, 2023 at 12:54 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > On Sun, Aug 6, 2023 at 6:02 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> > >
> > > IIUC the above query checks if the WAL record written at the slot's
> > > confirmed_flush_lsn is a CHECKPOINT_SHUTDOWN, but there is no check if
> > > this WAL record is the latest record.
> > >
> >
> > Yeah, I also think there should be some way to ensure this. How about
> > passing the number of records to read to this API? Actually, that will
> > address my other concern as well where the current API can lead to
> > reading an unbounded number of records if the confirmed_flush_lsn
> > location is far behind the CHECKPOINT_SHUTDOWN. Do you have any better
> > ideas to address it?
>
> It makes sense to me to limit the number of WAL records to read. But
> as I mentioned below, if there is a chance of any WAL activity during
> the upgrade, I'm not sure what limit to set.
>

In that case, we won't be able to pass the number of records. We need
to check based on the type of records.

>
> > However, I am not
> > sure if there can't be any additional WAL from checkpointer or
> > bgwriter. Checkpointer has a code that ensures that if there is no
> > important WAL activity then it would be skipped. Similarly, bgwriter
> > also doesn't LOG xl_running_xacts unless there is an important
> > activity.
>
> WAL records for hint bit updates could be generated even in upgrading mode?
>

Do you mean these records can be generated during reading catalog tables?

> > I feel if there is a chance of any WAL activity during the
> > upgrade, we need to either change the check to ensure such WAL records
> > are expected or document the same in some way.
>
> Yes, but how does it work with the above idea of limiting the number
> of WAL records to read? If XLOG_FPI_FOR_HINT can still be generated in
> the upgrade mode, we cannot predict how many such records are
> generated after the latest CHECKPOINT_SHUTDOWN.
>

Right, as said earlier, in that case, we need to rely on the type of records.

> I'm not really sure we should always perform the slot's
> confirmed_flush_lsn check by default in the first place. With this
> check, the upgrade won't be able to proceed if there is any logical
> slot that is not used by logical replication (or something streaming
> the changes using walsender), right? For example, if a user uses a
> program that periodically consumes the changes from the logical slot,
> the slot would not be able to pass the check even if the user executed
> pg_logical_slot_get_changes() just before shutdown. The backend
> process who consumes the changes is always terminated before the
> shutdown checkpoint. On the other hand, I think there are cases where
> the user can ensure that no meaningful WAL records are generated after
> the last pg_logical_slot_get_changes(). I'm concerned that this check
> might make upgrading such cases cumbersome unnecessarily.
>

You are right and I have mentioned the same case today in my response
to Jonathan but do you have better ideas to deal with such slots than
to give an ERROR?

--
With Regards,
Amit Kapila.