v2-0001-Fixes-related-to-archive-module-shutdown-callback.patch

From a0d5a3b8cfe1d52900027b97b8c1bf8fd9d04362 Mon Sep 17 00:00:00 2001
From: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Date: Tue, 18 Oct 2022 08:35:23 +0000
Subject: [PATCH v2] Fixes related to archive module shutdown callback

Presently, the archive module shutdown callback can get called
twice as it is first registered using PG_ENSURE_ERROR_CLEANUP and
then called directly in HandlePgArchInterrupts() before
proc_exit().

This patch fixes it by just registering it as before_shmem_exit().

In addition to the above, this patch also does the following
things:
1) Renames call_archive_module_shutdown_callback() to be more
meaningful and generic as before_shmem_exit() callback.
2) Clarifies when the archive module shutdown callback gets called
in documentation.
3) Defines a shutdown callback that just emits a log message in
shell_archive.c and tests it.

Author: Nathan Bossart
Author: Bharath Rupireddy
Reviewed-by: Michael Paquier, Kyotaro Horiguchi
Discussion: https://www.postgresql.org/message-id/20221015221328.GB1821022%40nathanxps13
Backpatch-through: 15
---
 doc/src/sgml/archive-modules.sgml         |  7 ++++---
 doc/src/sgml/config.sgml                  |  5 +++--
 src/backend/postmaster/pgarch.c           | 24 +++++++----------------
 src/backend/postmaster/shell_archive.c    |  9 +++++++++
 src/test/recovery/t/020_archive_status.pl | 19 ++++++++++++++++++
 5 files changed, 42 insertions(+), 22 deletions(-)

diff --git a/doc/src/sgml/archive-modules.sgml b/doc/src/sgml/archive-modules.sgml
index ef02051f7f..677a29182a 100644
--- a/doc/src/sgml/archive-modules.sgml
+++ b/doc/src/sgml/archive-modules.sgml
@@ -121,9 +121,10 @@ typedef bool (*ArchiveFileCB) (const char *file, const char *path);
   <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
+    The <function>shutdown_cb</function> callback is called before the server
+    begins to shut down low-level subsystems such as shared memory upon the
+    archiver process exit (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.
 
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 66312b53b8..22bbf05fef 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -3625,8 +3625,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 gets
+        restarted by the postmaster whenever 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 3868cd7bd3..f1d1f808a8 100644
--- 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_before_shmem_exit(int code, Datum arg);
 
 /* Report shared memory space needed by PgArchShmemInit */
 Size
@@ -232,6 +232,8 @@ PgArchiverMain(void)
 	/* We shouldn't be launched unnecessarily. */
 	Assert(XLogArchivingActive());
 
+	before_shmem_exit(pgarch_before_shmem_exit, 0);
+
 	/* Arrange to clean up at archiver exit */
 	on_shmem_exit(pgarch_die, 0);
 
@@ -252,13 +254,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);
 }
@@ -803,12 +799,6 @@ 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
@@ -861,12 +851,12 @@ LoadArchiveLibrary(void)
 }
 
 /*
- * call_archive_module_shutdown_callback
+ * Before shared memory exit callback for WAL archiver process.
  *
- * Calls the loaded archive module's shutdown callback, if one is defined.
+ * It calls the loaded archive module's shutdown callback, if one is defined.
  */
 static void
-call_archive_module_shutdown_callback(int code, Datum arg)
+pgarch_before_shmem_exit(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 8a54d02e7b..0a0ea0e6b5 100644
--- 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
@@ -156,3 +158,10 @@ 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)
+{
+	ereport(DEBUG1,
+			(errmsg_internal("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 550fb37cb6..b2c6a127f2 100644
--- a/src/test/recovery/t/020_archive_status.pl
+++ b/src/test/recovery/t/020_archive_status.pl
@@ -9,6 +9,7 @@ use warnings;
 use PostgreSQL::Test::Cluster;
 use PostgreSQL::Test::Utils;
 use Test::More;
+use Time::HiRes qw(usleep);
 
 my $primary = PostgreSQL::Test::Cluster->new('primary');
 $primary->init(
@@ -234,4 +235,22 @@ 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;
+
+$standby2->stop();
+
+# wait for postgres to terminate
+foreach my $i (0 .. 10 * $PostgreSQL::Test::Utils::timeout_default)
+{
+	last if !-f $standby2->data_dir . '/postmaster.pid';
+	usleep(100_000);
+}
+
+my $logfile = slurp_file($standby2->logfile());
+ok($logfile =~ qr/archiver process shutting down/,
+	'check shutdown callback of shell archive module');
+
 done_testing();
-- 
2.34.1