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