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

From: Daniel Gustafsson <[email-masked]>
Date: Wed, 20 Sep 2023 14:14:54 +0200

> On 19 Sep 2023, at 11:21, bt23nguyent <[email-masked]> wrote:

> Please let me know if you have any further suggestions that I can improve more.

+        *logdetail = pstrdup("WAL archiving failed because basic_archive.archive_directory is not set");

Nitpick: detail messages should end with a period per the error message style
guide [0].

-    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. 

I think this paragraph needs to be updated to include how the returned
logdetail is emitted, since it currently shows the WARNING without mentioning
the added detail in case returned.  It would also be good to mention that it
should be an allocated string which the caller can free.

--
Daniel Gustafsson

[0] https://www.postgresql.org/docs/devel/error-style-guide.html

---

From: bt23nguyent <[email-masked]>
Date: Thu, 21 Sep 2023 11:18:00 +0900

On 2023-09-20 21:14, Daniel Gustafsson wrote:
> +        *logdetail = pstrdup("WAL archiving failed because
> basic_archive.archive_directory is not set");
> 

Yes! I totally missed this detail.

> should be an allocated string which the caller can free.

Thank you for your kind review comment!

I agree with you that this document update is not explanatory enough.
So here is an updated patch.

If there is any further suggestion, please let me know.

Best regards,
Tung Nguyen

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: Nathan Bossart <[email-masked]>
Date: Mon, 25 Sep 2023 15:20:47 -0700

> +basic_archive_configured(ArchiveModuleState *state, char **logdetail)

Could we do something more like GUC_check_errdetail() instead to maintain
backward compatibility with v16?

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

---

From: Daniel Gustafsson <[email-masked]>
Date: Tue, 26 Sep 2023 08:13:45 +0200

> On 26 Sep 2023, at 00:20, Nathan Bossart <[email-masked]> wrote:
> 

We'd still need something exported to call into which isn't in 16, so it
wouldn't be more than optically backwards compatible since a module written for
17 won't compile for 16, or am I missing something?

--
Daniel Gustafsson

---

From: Nathan Bossart <[email-masked]>
Date: Thu, 12 Oct 2023 21:25:59 -0500

> 17 won't compile for 16, or am I missing something?

I only mean that a module written for v16 could continue to be used in v17
without any changes.  You are right that a module that uses this new
functionality wouldn't compile for v16.  But IMHO the interface is nicer,
too, since module authors wouldn't need to worry about allocating the space
for the string or formatting the message.

---

From: Daniel Gustafsson <[email-masked]>
Date: Fri, 13 Oct 2023 11:02:39 +0200

> On 13 Oct 2023, at 04:25, Nathan Bossart <[email-masked]> wrote:
> 
> functionality wouldn't compile for v16.

Sure, but that also means that few if any existing modules will be updated to
provide this =).

> But IMHO the interface is nicer,

That's a more compelling reason IMO.  I'm not sure if I prefer the
GUC_check_errdetail-like approach better, I would for sure not be opposed to
reviewing a version of the patch doing it that way.

Tung Nguyen: are you interested in updating the patch along these lines
suggested by Nathan?

> since module authors wouldn't need to worry about allocating the space

Well, they still need to format it; and calling <new_api>_errdetail(msg),
pstrdup(msg) or psprintf(msg) isn't a world of difference.

--
Daniel Gustafsson

---

From: Nathan Bossart <[email-masked]>
Date: Fri, 5 Jan 2024 17:03:57 -0600

> Tung Nguyen: are you interested in updating the patch along these lines

I gave it a try.

>From 7183005bb6786b63a5fd96ba5101849eb6f203e5 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <[email-masked]>
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: Nathan Bossart <[email-masked]>
Date: Wed, 28 Feb 2024 12:51:58 -0600

> I gave it a try.

Is there any interest in this?  If not, I'll withdraw the commitfest entry.

---

From: Daniel Gustafsson <[email-masked]>
Date: Wed, 28 Feb 2024 22:05:26 +0100

> On 28 Feb 2024, at 19:51, Nathan Bossart <[email-masked]> wrote:
> 

I'm still interested, please leave it in and I'll circle around to it.

--
Daniel Gustafsson

---

From: Nathan Bossart <[email-masked]>
Date: Wed, 28 Feb 2024 15:06:35 -0600

> I'm still interested, please leave it in and I'll circle around to it.

Thanks, Daniel.