Re: Impact of checkpointer during pg_upgrade

On Fri, Sep 8, 2023 at 9:00 AM Zhijie Hou (Fujitsu)
<houzj(dot)fnst(at)fujitsu(dot)com> wrote:
>
> On Friday, September 8, 2023 10:58 AM Michael Paquier <michael(at)paquier(dot)xyz> wrote:
> >
> > On Fri, Sep 08, 2023 at 08:18:14AM +0530, Amit Kapila wrote:
> > > This validation tries to ensure that we don't have any bugs/issues in
> > > our patch. It may be a candidate for assert but I feel even if we
> > > encounter any bug it is better to fix the bug.
> >
> > My guess is that an elog-like error is more adapted so as we are able to detect
> > problems in more cases, but perhaps an assert may be enough for the
> > buildfarm. If there is anything in the backend that causes slots to become
> > invalidated, I agree that any issue causing that should be fixed, but isn't the
> > point different here? Having a check at the end of an upgrade is a mean to
> > improve the detection rate of bugs where slots get invalidated, so it is actually
> > helpful to have one anyway? I am not sure what is your strategy here, do you
> > mean to keep a check at the end of pg_upgrade only in the patch to validate it?
> > Or do you mean to add something in pg_upgrade as part of the feature?
> >

We can do whatever the consensus is but I feel such an end-check to
some extent is only helpful for the testing of a patch before the
commit but not otherwise.

> > I
> > mean that doing the latter is benefitial for the sake of any patch committed and
> > as a long-term method to rely on.
>

What is your worry here? Are you worried that unknowingly in the
future we could add some other way to invalidate slots during upgrades
that we won't be able to detect?

> I feel adding a check at pg_upgrade may not 100% detect the slot invalidation
> if we check by querying the old cluster to get the slot info, because the
> invalidation can happen before the first time we fetch the info or after the
> last time we fetch the info(e.g. shutdown checkpoint could also invalidate
> slots)
>
> Personally, I think if we really want to add a check, it might be better to put
> it at server side, Like: reporting an ERROR at server side when invalidating
> the slot(InvalidatePossiblyObsoleteSlot) if in upgrade mode.
>

I don't know whether we really need to be extra careful in this case,
the same could be said about other consistency checks on the old
cluster.

--
With Regards,
Amit Kapila.