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

On Mon, Aug 7, 2023 at 6:02 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> 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?

Yes.

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

Another idea would be that before starting the old cluster we check if
the slot's confirmed_flush_lsn in the slot state file matches the
latest checkpoint LSN got by pg_controlfile. We need another tool to
dump the slot state file, though.

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

It makes sense to me to give an ERROR for such slots but does it also
make sense to make the check optional?

Regards,

--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com