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

On Fri, Sep 22, 2023 at 10:57 AM Bharath Rupireddy
<bharath(dot)rupireddyforpostgres(at)gmail(dot)com> wrote:
>
> On Thu, Sep 21, 2023 at 5:45 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> > > 3.
> > > + /* Quick exit if the given lsn is larger than current one */
> > > + if (start_lsn >= GetFlushRecPtr(NULL))
> > > + PG_RETURN_BOOL(false);
> > > +
> > >
> > > An LSN that doesn't exists yet is an error IMO, may be an error better here?
> > >
> >
> > It will anyway lead to error at later point but we will provide more
> > information about all the slots that have invalid value of
> > confirmed_flush LSN.
>
> I disagree with the function returning false for non-existing LSN.
> IMO, failing fast when an LSN that doesn't exist yet is supplied to
> the function is the right approach. We never know, the slots on disk
> content can get corrupted for some reason and confirmed_flush_lsn is
> 'FFFFFFFF/FFFFFFFF' or a non-existing LSN.
>

I don't think it is big deal to either fail immediately or slightly
later with more information about slot. It could be better if we do
later because various slots can have the same problem, so we can
mention all such slots together.

>
> > > 5. Trying to understand the interaction of this feature with custom
> > > WAL records that a custom WAL resource manager puts in. Is it okay to
> > > have custom WAL records after the "logical WAL end"?
> > > + /*
> > > + * There is a possibility that following records may be generated
> > > + * during the upgrade.
> > > + */
> > >
> >
> > I don't think so. The only valid records for the checks in this
> > function are probably the ones that can get generated by the upgrade
> > process because we ensure that walsender sends all the records before
> > it exits at shutdown time.
>
> Can you help me understand how the list of WAL records that pg_upgrade
> can generate is put up? Identified them after running some tests?
>

Yeah, both by tests and manually verifying the WAL records. Basically,
we need to care about records that could be generated by background
processes like checkpointer/bgwriter or can be generated during system
table scans. You may want to read my latest email for a summary on how
we reached at this design choice [1].

[1] - https://www.postgresql.org/message-id/CAA4eK1JVKZGRHLOEotWi%2Be%2B09jucNedqpkkc-Do4dh5FTAU%2B5w%40mail.gmail.com
--
With Regards,
Amit Kapila.