On Friday, September 8, 2023 10:58 AM Michael Paquier <michael@paquier.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? I
> mean that doing the latter is benefitial for the sake of any patch committed and
> as a long-term method to rely on.
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.
Having said that I feel it's fine if we don't add this check as setting
max_slot_wal_keep_size to -1 looks sufficient.
Best Regards,
Hou zj