Re: fix archive module shutdown callback - Mailing list pgsql-hackers

On Sun, Oct 16, 2022 at 3:43 AM Nathan Bossart <nathandbossart@gmail.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.

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

Also, I've noticed other 3 threads and CF entries all related to
'archive modules' feature. IMO, it could be better to have all of them
under a single thread and single CF entry to reduce
reviewers/committers' efforts and seek more thoughts about all of the
fixes.

https://commitfest.postgresql.org/40/3933/
https://commitfest.postgresql.org/40/3950/
https://commitfest.postgresql.org/40/3948/

[1]
  <sect2 id="archive-module-shutdown">
   <title>Shutdown Callback</title>
   <para>
    The <function>shutdown_cb</function> callback is called when the archiver
    process exits (e.g., after an error) or the value of
    <xref linkend="guc-archive-library"/> changes.  If no
    <function>shutdown_cb</function> is defined, no special action is taken in
    these situations.

-- 
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com