Rework shutdown callback of archiver modules

As currently designed, with a callback registered in a ERROR_CLEANUP
block, the shutdown callback would get called twice when updating
archive_library on SIGHUP, which is something that we want to avoid to
ease the life of extension writers.

Anyway, an ERROR in the archiver process is treated as a FATAL, stopping
it immediately, hence there is no need for a ERROR_CLEANUP block.
Instead of that, the shutdown callback is not called upon
before_shmem_exit(), giving to the modules the opportunity to do any
cleanup actions before the server shuts down its subsystems.

While on it, this commit adds some testing coverage for the shutdown
callback.  Neither shell_archive nor basic_archive have been using it,
and one is added to shell_archive, whose trigger is checked in a TAP
test through a shutdown sequence.

Author: Nathan Bossart, Bharath Rupireddy
Reviewed-by: Kyotaro Horiguchi, Michael Paquier
Discussion: https://postgr.es/m/20221015221328.GB1821022@nathanxps13
Backpatch-through: 15

diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 099867dd1b765c7bec2e36f4f42766957dfdb4c6..703f6d5a7f44b37aec253f6440413d2d99418086 100644 (file)
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -3618,8 +3618,9 @@ include_dir 'conf.d'
         The library to use for archiving completed WAL file segments.  If set to
         an empty string (the default), archiving via shell is enabled, and
         <xref linkend="guc-archive-command"/> is used.  Otherwise, the specified
-        shared library is used for archiving.  For more information, see
-        <xref linkend="backup-archiving-wal"/> and
+        shared library is used for archiving. The WAL archiver process is
+        restarted by the postmaster when this parameter changes. For more
+        information, see <xref linkend="backup-archiving-wal"/> and
         <xref linkend="archive-modules"/>.
        </para>
        <para>

diff --git a/src/backend/postmaster/pgarch.c b/src/backend/postmaster/pgarch.c
index 1240f9142370944cd80785898f87a4e32c1201a7..327df4e9a5d3620fc6e4f0576a80796dde709b94 100644 (file)
--- a/src/backend/postmaster/pgarch.c
+++ b/src/backend/postmaster/pgarch.c
@@ -144,7 +144,7 @@ static void pgarch_die(int code, Datum arg);
 static void HandlePgArchInterrupts(void);
 static int     ready_file_comparator(Datum a, Datum b, void *arg);
 static void LoadArchiveLibrary(void);
-static void call_archive_module_shutdown_callback(int code, Datum arg);
+static void pgarch_call_module_shutdown_cb(int code, Datum arg);
 
 /* Report shared memory space needed by PgArchShmemInit */
 Size
@@ -252,13 +252,7 @@ PgArchiverMain(void)
        /* Load the archive_library. */
        LoadArchiveLibrary();
 
-       PG_ENSURE_ERROR_CLEANUP(call_archive_module_shutdown_callback, 0);
-       {
-               pgarch_MainLoop();
-       }
-       PG_END_ENSURE_ERROR_CLEANUP(call_archive_module_shutdown_callback, 0);
-
-       call_archive_module_shutdown_callback(0, 0);
+       pgarch_MainLoop();
 
        proc_exit(0);
 }
@@ -791,19 +785,14 @@ HandlePgArchInterrupts(void)
 
                if (archiveLibChanged)
                {
-                       /*
-                        * Call the currently loaded archive module's shutdown callback,
-                        * if one is defined.
-                        */
-                       call_archive_module_shutdown_callback(0, 0);
-
                        /*
                         * Ideally, we would simply unload the previous archive module and
                         * load the new one, but there is presently no mechanism for
                         * unloading a library (see the comment above
                         * internal_load_library()).  To deal with this, we simply restart
                         * the archiver.  The new archive module will be loaded when the
-                        * new archiver process starts up.
+                        * new archiver process starts up.  Note that this triggers the
+                        * module's shutdown callback, if defined.
                         */
                        ereport(LOG,
                                        (errmsg("restarting archiver process because value of "
@@ -846,15 +835,15 @@ LoadArchiveLibrary(void)
        if (ArchiveContext.archive_file_cb == NULL)
                ereport(ERROR,
                                (errmsg("archive modules must register an archive callback")));
+
+       before_shmem_exit(pgarch_call_module_shutdown_cb, 0);
 }
 
 /*
- * call_archive_module_shutdown_callback
- *
- * Calls the loaded archive module's shutdown callback, if one is defined.
+ * Call the shutdown callback of the loaded archive module, if defined.
  */
 static void
-call_archive_module_shutdown_callback(int code, Datum arg)
+pgarch_call_module_shutdown_cb(int code, Datum arg)
 {
        if (ArchiveContext.shutdown_cb != NULL)
                ArchiveContext.shutdown_cb();

diff --git a/src/backend/postmaster/shell_archive.c b/src/backend/postmaster/shell_archive.c
index 19e240c2053bc84caa8bb66127675ed10682fbac..6306f5bd7bfca84323a4f33ccc08bcb66b6ab8d5 100644 (file)
--- a/src/backend/postmaster/shell_archive.c
+++ b/src/backend/postmaster/shell_archive.c
@@ -23,6 +23,7 @@
 
 static bool shell_archive_configured(void);
 static bool shell_archive_file(const char *file, const char *path);
+static void shell_archive_shutdown(void);
 
 void
 shell_archive_init(ArchiveModuleCallbacks *cb)
@@ -31,6 +32,7 @@ shell_archive_init(ArchiveModuleCallbacks *cb)
 
        cb->check_configured_cb = shell_archive_configured;
        cb->archive_file_cb = shell_archive_file;
+       cb->shutdown_cb = shell_archive_shutdown;
 }
 
 static bool
@@ -155,3 +157,9 @@ shell_archive_file(const char *file, const char *path)
        elog(DEBUG1, "archived write-ahead log file \"%s\"", file);
        return true;
 }
+
+static void
+shell_archive_shutdown(void)
+{
+       elog(DEBUG1, "archiver process shutting down");
+}

diff --git a/src/test/recovery/t/020_archive_status.pl b/src/test/recovery/t/020_archive_status.pl
index 550fb37cb68993c3c1f1465593c7ce2049b3db85..2108d50073abf6291033187448997ccd64e1b75f 100644 (file)
--- a/src/test/recovery/t/020_archive_status.pl
+++ b/src/test/recovery/t/020_archive_status.pl
@@ -234,4 +234,18 @@ ok( -f "$standby2_data/$segment_path_1_done"
        ".done files created after archive success with archive_mode=always on standby"
 );
 
+# Check that the archiver process calls the shell archive module's shutdown
+# callback.
+$standby2->append_conf('postgresql.conf', "log_min_messages = debug1");
+$standby2->reload;
+
+# Run a query to make sure that the reload has taken effect.
+$standby2->safe_psql('postgres', q{SELECT 1});
+my $log_location = -s $standby2->logfile;
+
+$standby2->stop;
+my $logfile = slurp_file($standby2->logfile, $log_location);
+ok( $logfile =~ qr/archiver process shutting down/,
+       'check shutdown callback of shell archive module');
+
 done_testing();