Improve the log message output of basic_archive when basic_archive.archive_directory parameter is not set

diff --git a/contrib/basic_archive/basic_archive.c b/contrib/basic_archive/basic_archive.c
index 4d78c31859..7b8673f338 100644
--- a/contrib/basic_archive/basic_archive.c
+++ b/contrib/basic_archive/basic_archive.c
@@ -48,7 +48,7 @@ typedef struct BasicArchiveData
 static char *archive_directory = NULL;
 
 static void basic_archive_startup(ArchiveModuleState *state);
-static bool basic_archive_configured(ArchiveModuleState *state);
+static bool basic_archive_configured(ArchiveModuleState *state, char **logdetail);
 static bool basic_archive_file(ArchiveModuleState *state, const char *file, const char *path);
 static void basic_archive_file_internal(const char *file, const char *path);
 static bool check_archive_directory(char **newval, void **extra, GucSource source);
@@ -159,9 +159,15 @@ check_archive_directory(char **newval, void **extra, GucSource source)
  * Checks that archive_directory is not blank.
  */
 static bool
-basic_archive_configured(ArchiveModuleState *state)
+basic_archive_configured(ArchiveModuleState *state, char **logdetail)
 {
-	return archive_directory != NULL && archive_directory[0] != '\0';
+	if (archive_directory == NULL || archive_directory[0] == '\0')
+    {
+        *logdetail = pstrdup("WAL archiving failed because basic_archive.archive_directory is not set");
+        return false;
+    }
+    else
+        return true;
 }
 
 /*
diff --git a/doc/src/sgml/archive-modules.sgml b/doc/src/sgml/archive-modules.sgml
index 7064307d9e..f9f6177844 100644
--- a/doc/src/sgml/archive-modules.sgml
+++ b/doc/src/sgml/archive-modules.sgml
@@ -101,7 +101,7 @@ typedef void (*ArchiveStartupCB) (ArchiveModuleState *state);
     assumes the module is configured.
 
 <programlisting>
-typedef bool (*ArchiveCheckConfiguredCB) (ArchiveModuleState *state);
+typedef bool (*ArchiveCheckConfiguredCB) (ArchiveModuleState *state, char **logdetail);
 </programlisting>
 
     If <literal>true</literal> is returned, the server will proceed with
@@ -112,7 +112,9 @@ typedef bool (*ArchiveCheckConfiguredCB) (ArchiveModuleState *state);
 WARNING:  archive_mode enabled, yet archiving is not configured
 </screen>
     In the latter case, the server will periodically call this function, and
-    archiving will proceed only when it returns <literal>true</literal>.
+    archiving will proceed only when it returns <literal>true</literal>. The
+    archiver may also emit the detail explaining how the module is not configured
+    to the sever log if the archive module has any. 
    </para>
   </sect2>
 
diff --git a/src/backend/archive/shell_archive.c b/src/backend/archive/shell_archive.c
index 157ca4c751..c957a18ee7 100644
--- a/src/backend/archive/shell_archive.c
+++ b/src/backend/archive/shell_archive.c
@@ -23,7 +23,7 @@
 #include "common/percentrepl.h"
 #include "pgstat.h"
 
-static bool shell_archive_configured(ArchiveModuleState *state);
+static bool shell_archive_configured(ArchiveModuleState *state, char **logdetail);
 static bool shell_archive_file(ArchiveModuleState *state,
 							   const char *file,
 							   const char *path);
@@ -43,7 +43,7 @@ shell_archive_init(void)
 }
 
 static bool
-shell_archive_configured(ArchiveModuleState *state)
+shell_archive_configured(ArchiveModuleState *state, char **logdetail)
 {
 	return XLogArchiveCommand[0] != '\0';
 }
diff --git a/src/backend/postmaster/pgarch.c b/src/backend/postmaster/pgarch.c
index 46af349564..2fd1d03b09 100644
--- a/src/backend/postmaster/pgarch.c
+++ b/src/backend/postmaster/pgarch.c
@@ -390,7 +390,8 @@ pgarch_ArchiverCopyLoop(void)
 		{
 			struct stat stat_buf;
 			char		pathname[MAXPGPATH];
-
+			char       *logdetail;
+			
 			/*
 			 * Do not initiate any more archive commands after receiving
 			 * SIGTERM, nor after the postmaster has died unexpectedly. The
@@ -410,10 +411,13 @@ pgarch_ArchiverCopyLoop(void)
 
 			/* can't do anything if not configured ... */
 			if (ArchiveCallbacks->check_configured_cb != NULL &&
-				!ArchiveCallbacks->check_configured_cb(archive_module_state))
+				!ArchiveCallbacks->check_configured_cb(archive_module_state, &logdetail))
 			{
 				ereport(WARNING,
-						(errmsg("archive_mode enabled, yet archiving is not configured")));
+						(errmsg("archive_mode enabled, yet archiving is not configured")),
+						(logdetail != NULL) ? errdetail("%s", logdetail) : 0);
+				if (logdetail != NULL)
+					pfree(logdetail);
 				return;
 			}
 
diff --git a/src/include/archive/archive_module.h b/src/include/archive/archive_module.h
index 679ce5a6db..43bd0ff4c2 100644
--- a/src/include/archive/archive_module.h
+++ b/src/include/archive/archive_module.h
@@ -36,7 +36,7 @@ typedef struct ArchiveModuleState
  * archive modules documentation.
  */
 typedef void (*ArchiveStartupCB) (ArchiveModuleState *state);
-typedef bool (*ArchiveCheckConfiguredCB) (ArchiveModuleState *state);
+typedef bool (*ArchiveCheckConfiguredCB) (ArchiveModuleState *state, char **logdetail);
 typedef bool (*ArchiveFileCB) (ArchiveModuleState *state, const char *file, const char *path);
 typedef void (*ArchiveShutdownCB) (ArchiveModuleState *state);

diff --git a/contrib/basic_archive/basic_archive.c b/contrib/basic_archive/basic_archive.c
index 4d78c31859..dd0f2816dc 100644
--- a/contrib/basic_archive/basic_archive.c
+++ b/contrib/basic_archive/basic_archive.c
@@ -48,7 +48,7 @@ typedef struct BasicArchiveData
 static char *archive_directory = NULL;
 
 static void basic_archive_startup(ArchiveModuleState *state);
-static bool basic_archive_configured(ArchiveModuleState *state);
+static bool basic_archive_configured(ArchiveModuleState *state, char **logdetail);
 static bool basic_archive_file(ArchiveModuleState *state, const char *file, const char *path);
 static void basic_archive_file_internal(const char *file, const char *path);
 static bool check_archive_directory(char **newval, void **extra, GucSource source);
@@ -159,9 +159,15 @@ check_archive_directory(char **newval, void **extra, GucSource source)
  * Checks that archive_directory is not blank.
  */
 static bool
-basic_archive_configured(ArchiveModuleState *state)
+basic_archive_configured(ArchiveModuleState *state, char **logdetail)
 {
-	return archive_directory != NULL && archive_directory[0] != '\0';
+	if (archive_directory == NULL || archive_directory[0] == '\0')
+    {
+        *logdetail = pstrdup("WAL archiving failed because basic_archive.archive_directory is not set.");
+        return false;
+    }
+    else
+        return true;
 }
 
 /*
diff --git a/doc/src/sgml/archive-modules.sgml b/doc/src/sgml/archive-modules.sgml
index 7064307d9e..b58ed238b4 100644
--- a/doc/src/sgml/archive-modules.sgml
+++ b/doc/src/sgml/archive-modules.sgml
@@ -101,7 +101,7 @@ typedef void (*ArchiveStartupCB) (ArchiveModuleState *state);
     assumes the module is configured.
 
 <programlisting>
-typedef bool (*ArchiveCheckConfiguredCB) (ArchiveModuleState *state);
+typedef bool (*ArchiveCheckConfiguredCB) (ArchiveModuleState *state, char **logdetail);
 </programlisting>
 
     If <literal>true</literal> is returned, the server will proceed with
@@ -112,7 +112,10 @@ typedef bool (*ArchiveCheckConfiguredCB) (ArchiveModuleState *state);
 WARNING:  archive_mode enabled, yet archiving is not configured
 </screen>
     In the latter case, the server will periodically call this function, and
-    archiving will proceed only when it returns <literal>true</literal>.
+    archiving will proceed only when it returns <literal>true</literal>. The
+    archiver may also emit the detail explaining how the module is not configured
+    to the sever log if the archive module has any. The detail message of archive
+    module should be an allocated string which the caller can free.
    </para>
   </sect2>
 
diff --git a/src/backend/archive/shell_archive.c b/src/backend/archive/shell_archive.c
index 157ca4c751..c957a18ee7 100644
--- a/src/backend/archive/shell_archive.c
+++ b/src/backend/archive/shell_archive.c
@@ -23,7 +23,7 @@
 #include "common/percentrepl.h"
 #include "pgstat.h"
 
-static bool shell_archive_configured(ArchiveModuleState *state);
+static bool shell_archive_configured(ArchiveModuleState *state, char **logdetail);
 static bool shell_archive_file(ArchiveModuleState *state,
 							   const char *file,
 							   const char *path);
@@ -43,7 +43,7 @@ shell_archive_init(void)
 }
 
 static bool
-shell_archive_configured(ArchiveModuleState *state)
+shell_archive_configured(ArchiveModuleState *state, char **logdetail)
 {
 	return XLogArchiveCommand[0] != '\0';
 }
diff --git a/src/backend/postmaster/pgarch.c b/src/backend/postmaster/pgarch.c
index 46af349564..2fd1d03b09 100644
--- a/src/backend/postmaster/pgarch.c
+++ b/src/backend/postmaster/pgarch.c
@@ -390,7 +390,8 @@ pgarch_ArchiverCopyLoop(void)
 		{
 			struct stat stat_buf;
 			char		pathname[MAXPGPATH];
-
+			char       *logdetail;
+			
 			/*
 			 * Do not initiate any more archive commands after receiving
 			 * SIGTERM, nor after the postmaster has died unexpectedly. The
@@ -410,10 +411,13 @@ pgarch_ArchiverCopyLoop(void)
 
 			/* can't do anything if not configured ... */
 			if (ArchiveCallbacks->check_configured_cb != NULL &&
-				!ArchiveCallbacks->check_configured_cb(archive_module_state))
+				!ArchiveCallbacks->check_configured_cb(archive_module_state, &logdetail))
 			{
 				ereport(WARNING,
-						(errmsg("archive_mode enabled, yet archiving is not configured")));
+						(errmsg("archive_mode enabled, yet archiving is not configured")),
+						(logdetail != NULL) ? errdetail("%s", logdetail) : 0);
+				if (logdetail != NULL)
+					pfree(logdetail);
 				return;
 			}
 
diff --git a/src/include/archive/archive_module.h b/src/include/archive/archive_module.h
index 679ce5a6db..43bd0ff4c2 100644
--- a/src/include/archive/archive_module.h
+++ b/src/include/archive/archive_module.h
@@ -36,7 +36,7 @@ typedef struct ArchiveModuleState
  * archive modules documentation.
  */
 typedef void (*ArchiveStartupCB) (ArchiveModuleState *state);
-typedef bool (*ArchiveCheckConfiguredCB) (ArchiveModuleState *state);
+typedef bool (*ArchiveCheckConfiguredCB) (ArchiveModuleState *state, char **logdetail);
 typedef bool (*ArchiveFileCB) (ArchiveModuleState *state, const char *file, const char *path);
 typedef void (*ArchiveShutdownCB) (ArchiveModuleState *state);

From 7183005bb6786b63a5fd96ba5101849eb6f203e5 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <nathan@postgresql.org>
Date: Fri, 5 Jan 2024 17:01:10 -0600
Subject: [PATCH v4 1/1] add support for emitting errdetail from archive module
 check-configured callback

---
 contrib/basic_archive/basic_archive.c | 8 +++++++-
 src/backend/postmaster/pgarch.c       | 8 +++++++-
 src/include/archive/archive_module.h  | 8 ++++++++
 3 files changed, 22 insertions(+), 2 deletions(-)

diff --git a/contrib/basic_archive/basic_archive.c b/contrib/basic_archive/basic_archive.c
index 804567e919..2c8721ebf6 100644
--- a/contrib/basic_archive/basic_archive.c
+++ b/contrib/basic_archive/basic_archive.c
@@ -161,7 +161,13 @@ check_archive_directory(char **newval, void **extra, GucSource source)
 static bool
 basic_archive_configured(ArchiveModuleState *state)
 {
-	return archive_directory != NULL && archive_directory[0] != '\0';
+	if (archive_directory == NULL || archive_directory[0] == '\0')
+	{
+		arch_module_check_errdetail("basic_archive.archive_directory is not set.");
+		return false;
+	}
+
+	return true;
 }
 
 /*
diff --git a/src/backend/postmaster/pgarch.c b/src/backend/postmaster/pgarch.c
index 67693b0580..f663965d89 100644
--- a/src/backend/postmaster/pgarch.c
+++ b/src/backend/postmaster/pgarch.c
@@ -91,6 +91,7 @@ typedef struct PgArchData
 } PgArchData;
 
 char	   *XLogArchiveLibrary = "";
+char	   *arch_module_check_errdetail_string;
 
 
 /* ----------
@@ -408,12 +409,17 @@ pgarch_ArchiverCopyLoop(void)
 			 */
 			HandlePgArchInterrupts();
 
+			/* Reset variables that might be set by the callback */
+			arch_module_check_errdetail_string = NULL;
+
 			/* can't do anything if not configured ... */
 			if (ArchiveCallbacks->check_configured_cb != NULL &&
 				!ArchiveCallbacks->check_configured_cb(archive_module_state))
 			{
 				ereport(WARNING,
-						(errmsg("archive_mode enabled, yet archiving is not configured")));
+						(errmsg("archive_mode enabled, yet archiving is not configured"),
+						 arch_module_check_errdetail_string ?
+						 errdetail_internal("%s", arch_module_check_errdetail_string) : 0));
 				return;
 			}
 
diff --git a/src/include/archive/archive_module.h b/src/include/archive/archive_module.h
index fd59b9faf4..763af76e54 100644
--- a/src/include/archive/archive_module.h
+++ b/src/include/archive/archive_module.h
@@ -56,4 +56,12 @@ typedef const ArchiveModuleCallbacks *(*ArchiveModuleInit) (void);
 
 extern PGDLLEXPORT const ArchiveModuleCallbacks *_PG_archive_module_init(void);
 
+/* Support for messages reported from archive module callbacks. */
+
+extern PGDLLIMPORT char *arch_module_check_errdetail_string;
+
+#define arch_module_check_errdetail \
+	pre_format_elog_string(errno, TEXTDOMAIN), \
+	arch_module_check_errdetail_string = format_elog_string
+
 #endif							/* _ARCHIVE_MODULE_H */
-- 
2.25.1

From 437e4fa9ec27ecf870530fc579cd7673dfcf96af Mon Sep 17 00:00:00 2001
From: Nathan Bossart <nathan@postgresql.org>
Date: Mon, 4 Mar 2024 11:15:37 -0600
Subject: [PATCH v5 1/1] Add macro for customizing an archive module WARNING
 message.
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Presently, if an archive module's check_configured_cb callback
returns false, a generic WARNING about the archive module
misconfiguration is emitted, which unfortunately provides no
actionable details about the source of the miconfiguration.  This
commit introduces a macro that archive module authors can use to
add a DETAIL line to this WARNING.

Co-authored-by: Tung Nguyen
Reviewed-by: Daniel Gustafsson, Álvaro Herrera
Discussion: https://postgr.es/m/4109578306242a7cd5661171647e11b2%40oss.nttdata.com
---
 contrib/basic_archive/basic_archive.c |  7 ++++++-
 doc/src/sgml/archive-modules.sgml     | 12 ++++++++++++
 src/backend/archive/shell_archive.c   |  7 ++++++-
 src/backend/postmaster/pgarch.c       |  8 +++++++-
 src/include/archive/archive_module.h  |  8 ++++++++
 5 files changed, 39 insertions(+), 3 deletions(-)

diff --git a/contrib/basic_archive/basic_archive.c b/contrib/basic_archive/basic_archive.c
index 804567e919..6b102e9072 100644
--- a/contrib/basic_archive/basic_archive.c
+++ b/contrib/basic_archive/basic_archive.c
@@ -161,7 +161,12 @@ check_archive_directory(char **newval, void **extra, GucSource source)
 static bool
 basic_archive_configured(ArchiveModuleState *state)
 {
-	return archive_directory != NULL && archive_directory[0] != '\0';
+	if (archive_directory != NULL && archive_directory[0] != '\0')
+		return true;
+
+	arch_module_check_errdetail("%s is not set.",
+								"basic_archive.archive_directory");
+	return false;
 }
 
 /*
diff --git a/doc/src/sgml/archive-modules.sgml b/doc/src/sgml/archive-modules.sgml
index 7064307d9e..cf7438a759 100644
--- a/doc/src/sgml/archive-modules.sgml
+++ b/doc/src/sgml/archive-modules.sgml
@@ -114,6 +114,18 @@ WARNING:  archive_mode enabled, yet archiving is not configured
     In the latter case, the server will periodically call this function, and
     archiving will proceed only when it returns <literal>true</literal>.
    </para>
+
+   <note>
+    <para>
+     When returning <literal>false</literal>, it may be useful to append some
+     additional information to the generic warning message.  To do that, provide
+     a message to the <function>arch_module_check_errdetail</function> macro
+     before returning <literal>false</literal>.  Like
+     <function>errdetail()</function>, this macro accepts a format string
+     followed by an optional list of arguments.  The resulting string will be
+     emitted as the <literal>DETAIL</literal> line of the warning message.
+    </para>
+   </note>
   </sect2>
 
   <sect2 id="archive-module-archive">
diff --git a/src/backend/archive/shell_archive.c b/src/backend/archive/shell_archive.c
index c95b732495..bff0ab800d 100644
--- a/src/backend/archive/shell_archive.c
+++ b/src/backend/archive/shell_archive.c
@@ -45,7 +45,12 @@ shell_archive_init(void)
 static bool
 shell_archive_configured(ArchiveModuleState *state)
 {
-	return XLogArchiveCommand[0] != '\0';
+	if (XLogArchiveCommand[0] != '\0')
+		return true;
+
+	arch_module_check_errdetail("%s is not set.",
+								"archive_command");
+	return false;
 }
 
 static bool
diff --git a/src/backend/postmaster/pgarch.c b/src/backend/postmaster/pgarch.c
index bb0eb13a89..f97035ca03 100644
--- a/src/backend/postmaster/pgarch.c
+++ b/src/backend/postmaster/pgarch.c
@@ -88,6 +88,7 @@ typedef struct PgArchData
 } PgArchData;
 
 char	   *XLogArchiveLibrary = "";
+char	   *arch_module_check_errdetail_string;
 
 
 /* ----------
@@ -401,12 +402,17 @@ pgarch_ArchiverCopyLoop(void)
 			 */
 			HandlePgArchInterrupts();
 
+			/* Reset variables that might be set by the callback */
+			arch_module_check_errdetail_string = NULL;
+
 			/* can't do anything if not configured ... */
 			if (ArchiveCallbacks->check_configured_cb != NULL &&
 				!ArchiveCallbacks->check_configured_cb(archive_module_state))
 			{
 				ereport(WARNING,
-						(errmsg("archive_mode enabled, yet archiving is not configured")));
+						(errmsg("archive_mode enabled, yet archiving is not configured"),
+						 arch_module_check_errdetail_string ?
+						 errdetail_internal("%s", arch_module_check_errdetail_string) : 0));
 				return;
 			}
 
diff --git a/src/include/archive/archive_module.h b/src/include/archive/archive_module.h
index fd59b9faf4..763af76e54 100644
--- a/src/include/archive/archive_module.h
+++ b/src/include/archive/archive_module.h
@@ -56,4 +56,12 @@ typedef const ArchiveModuleCallbacks *(*ArchiveModuleInit) (void);
 
 extern PGDLLEXPORT const ArchiveModuleCallbacks *_PG_archive_module_init(void);
 
+/* Support for messages reported from archive module callbacks. */
+
+extern PGDLLIMPORT char *arch_module_check_errdetail_string;
+
+#define arch_module_check_errdetail \
+	pre_format_elog_string(errno, TEXTDOMAIN), \
+	arch_module_check_errdetail_string = format_elog_string
+
 #endif							/* _ARCHIVE_MODULE_H */
-- 
2.25.1