Re: fix archive module shutdown callback

At Sun, 16 Oct 2022 13:39:14 +0530, Bharath Rupireddy <bharath(dot)rupireddyforpostgres(at)gmail(dot)com> wrote in
> On Sun, Oct 16, 2022 at 3:43 AM Nathan Bossart <nathandbossart(at)gmail(dot)com> wrote:
> >
> > Hi hackers,
> >
> > Presently, when an archive module sets up a shutdown callback, it will be
> > called upon ERROR/FATAL (via PG_ENSURE_ERROR_CLEANUP), when the archive
> > library changes (via HandlePgArchInterrupts()), and upon normal shutdown.
> > There are a couple of problems with this:
>
> Yeah.
>
> > * HandlePgArchInterrupts() calls the shutdown callback directly before
> > proc_exit(). However, the PG_ENSURE_ERROR_CLEANUP surrounding the call to
> > pgarch_MainLoop() sets up a before_shmem_exit callback that also calls the
> > shutdown callback. This means that the shutdown callback will be called
> > twice whenever archive_library is changed via SIGHUP.
> >
> > * PG_ENSURE_ERROR_CLEANUP is intended for both ERROR and FATAL. However,
> > the archiver operates at the bottom of the exception stack, so ERRORs are
> > treated as FATALs. This means that PG_ENSURE_ERROR_CLEANUP is excessive.
> > We only need to set up the before_shmem_exit callback.
> >
> > To fix, the attached patch removes the use of PG_ENSURE_ERROR_CLEANUP and
> > the call to the shutdown callback in HandlePgArchInterrupts() in favor of
> > just setting up a before_shmem_exit callback in LoadArchiveLibrary().
>
> We could have used a flag in call_archive_module_shutdown_callback()
> to avoid it being called multiple times, but having it as
> before_shmem_exit () callback without direct calls to it is the right
> approach IMO.
>
> +1 to remove PG_ENSURE_ERROR_CLEANUP and PG_END_ENSURE_ERROR_CLEANUP.

That prevents archiver process from cleanly shut down when something
wrong happnes outside the interuppt handler. In the frist place, why
do we need to call the cleanup callback directly in the handler? We
can let the handler return something instead to tell the
pgarch_MainLoop to exit immediately on the normal path.

> Is the shutdown callback meant to be called only after the archive
> library is loaded? The documentation [1] says that it just gets called
> before the archiver process exits. If this is true, can we just place
> before_shmem_exit(call_archive_module_shutdown_callback, 0); in
> PgArchiverMain() after on_shmem_exit(pgarch_die, 0);?

+1 for using before_shmem_exit.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center