From: Nathan Bossart <nathandbossart@gmail.com>
To: Daniel Gustafsson <daniel@yesql.se>
Cc: bt23nguyent <bt23nguyent@oss.nttdata.com>
Cc: Alvaro Herrera <alvherre@alvh.no-ip.org>
Cc: pgsql-hackers@lists.postgresql.org
Subject: Re: Improve the log message output of basic_archive when basic_archive.archive_directory parameter is not set
Date: Mon, 4 Mar 2024 11:22:52 -0600
Message-ID: <20240304172252.GA3161951@nathanxps13> (raw)
In-Reply-To: <AB31C950-5BA6-49F2-ADF9-F8E8334EB4A2@yesql.se>
References: <20230915143827.GA1912920@nathanxps13>
<85b9e69997bbd5d8302a3a99a2de246d@oss.nttdata.com>
<D3038255-EAB5-4164-A9E3-7967B6738993@yesql.se>
<18cbd8ca6f5390cda2920644f891a419@oss.nttdata.com>
<20230925222047.GA3173226@nathanxps13>
<482056B1-D930-417C-9473-CB1A045E42AE@yesql.se>
<20231013022559.GA1266364@nathanxps13>
<E863EDCA-EFEA-4DFA-9E93-024D464DE260@yesql.se>
<20240105230357.GA2425960@nathanxps13>
<AB31C950-5BA6-49F2-ADF9-F8E8334EB4A2@yesql.se>
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.comAttachments:v5-0001-Add-macro-for-customizing-an-archive-module-WARNI.patch (text/x-diff, 5.0K) download
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.cindex 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.sgmlindex 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.cindex 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.cindex 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.hindex 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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: nathandbossart@gmail.com, daniel@yesql.se, bt23nguyent@oss.nttdata.com, alvherre@alvh.no-ip.org, pgsql-hackers@lists.postgresql.org
Subject: Re: Improve the log message output of basic_archive when basic_archive.archive_directory parameter is not set
In-Reply-To: <20240304172252.GA3161951@nathanxps13>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox