On Mon, Mar 04, 2024 at 03:21:59PM +0100, Daniel Gustafsson wrote: > Looking at this again I think this is about ready to go in. My only comment > is > that doc/src/sgml/archive-modules.sgml probably should be updated to refer to > setting the errdetail, especially since we document the errormessage there.
Thanks for reviewing. How does this look? -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
>From 437e4fa9ec27ecf870530fc579cd7673dfcf96af Mon Sep 17 00:00:00 2001 From: Nathan Bossart <nat...@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
