Re: Improve the log message output of basic_archive when basic_archive.archive_directory parameter is not set
Nathan Bossart writes: > On Mon, Mar 04, 2024 at 09:27:23PM +0100, Daniel Gustafsson wrote: >> Reviewed-by: Daniel Gustafsson, ¡lvaro Herrera >> Alvaro's name seems wrong. > Hm. It looks alright to me. I copied the name from his e-mail signature, > which has an accent over the first 'A'. I assume that's why it's not > showing up correctly in some places. I think that git has an expectation of commit log entries being in UTF8. The committed message looks okay from my end, but maybe some encoding mangling happened to the version Daniel was looking at? regards, tom lane
Re: Improve the log message output of basic_archive when basic_archive.archive_directory parameter is not set
On Mon, Mar 04, 2024 at 09:27:23PM +0100, Daniel Gustafsson wrote: > Looks good from a read-through, I like it. A few comments on the commit > message only: > > actionable details about the source of the miconfiguration. This > s/miconfiguration/misconfiguration/ I reworded the commit message a bit to avoid the word "misconfiguration," as it felt a bit misleading to me. In any case, this was fixed, albeit indirectly. > Reviewed-by: Daniel Gustafsson, ¡lvaro Herrera > Alvaro's name seems wrong. Hm. It looks alright to me. I copied the name from his e-mail signature, which has an accent over the first 'A'. I assume that's why it's not showing up correctly in some places. Anyway, I've committed this now. Thanks for taking a look! -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
Re: Improve the log message output of basic_archive when basic_archive.archive_directory parameter is not set
> On 4 Mar 2024, at 18:22, Nathan Bossart wrote: > > 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? Looks good from a read-through, I like it. A few comments on the commit message only: actionable details about the source of the miconfiguration. This s/miconfiguration/misconfiguration/ Reviewed-by: Daniel Gustafsson, ¡lvaro Herrera Alvaro's name seems wrong. -- Daniel Gustafsson
Re: Improve the log message output of basic_archive when basic_archive.archive_directory parameter is not set
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
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 true.
+
+
+
+ When returning false, it may be useful to append some
+ additional information to the generic warning message. To do that, provide
+ a message to the arch_module_check_errdetail macro
+ before returning false. Like
+ errdetail(), this macro accepts a format string
+ followed by an optional list of arguments. The resulting string will be
+ emitted as the DETAIL line of the warning message.
+
+
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
Re: Improve the log message output of basic_archive when basic_archive.archive_directory parameter is not set
> On 6 Jan 2024, at 00:03, Nathan Bossart wrote: > I gave it a try. 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. -- Daniel Gustafsson
Re: Improve the log message output of basic_archive when basic_archive.archive_directory parameter is not set
On Wed, Feb 28, 2024 at 10:05:26PM +0100, Daniel Gustafsson wrote: >> On 28 Feb 2024, at 19:51, Nathan Bossart wrote: >> Is there any interest in this? If not, I'll withdraw the commitfest entry. > > I'm still interested, please leave it in and I'll circle around to it. Thanks, Daniel. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
Re: Improve the log message output of basic_archive when basic_archive.archive_directory parameter is not set
> On 28 Feb 2024, at 19:51, Nathan Bossart wrote: > > On Fri, Jan 05, 2024 at 05:03:57PM -0600, Nathan Bossart wrote: >> I gave it a try. > > Is there any interest in this? If not, I'll withdraw the commitfest entry. I'm still interested, please leave it in and I'll circle around to it. -- Daniel Gustafsson
Re: Improve the log message output of basic_archive when basic_archive.archive_directory parameter is not set
On Fri, Jan 05, 2024 at 05:03:57PM -0600, Nathan Bossart wrote: > I gave it a try. Is there any interest in this? If not, I'll withdraw the commitfest entry. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
Re: Improve the log message output of basic_archive when basic_archive.archive_directory parameter is not set
On Fri, Oct 13, 2023 at 11:02:39AM +0200, Daniel Gustafsson wrote:
> 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?
I gave it a try.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
>From 7183005bb6786b63a5fd96ba5101849eb6f203e5 Mon Sep 17 00:00:00 2001
From: Nathan Bossart
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
Re: Improve the log message output of basic_archive when basic_archive.archive_directory parameter is not set
> On 13 Oct 2023, at 04:25, Nathan Bossart wrote: > > On Tue, Sep 26, 2023 at 08:13:45AM +0200, Daniel Gustafsson wrote: >>> On 26 Sep 2023, at 00:20, Nathan Bossart wrote: >>> >>> On Thu, Sep 21, 2023 at 11:18:00AM +0900, bt23nguyent wrote: -basic_archive_configured(ArchiveModuleState *state) +basic_archive_configured(ArchiveModuleState *state, char **logdetail) >>> >>> Could we do something more like GUC_check_errdetail() instead to maintain >>> backward compatibility with v16? >> >> 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? > > 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. 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 > for the string or formatting the message. Well, they still need to format it; and calling _errdetail(msg), pstrdup(msg) or psprintf(msg) isn't a world of difference. -- Daniel Gustafsson
Re: Improve the log message output of basic_archive when basic_archive.archive_directory parameter is not set
On Tue, Sep 26, 2023 at 08:13:45AM +0200, Daniel Gustafsson wrote: >> On 26 Sep 2023, at 00:20, Nathan Bossart wrote: >> >> On Thu, Sep 21, 2023 at 11:18:00AM +0900, bt23nguyent wrote: >>> -basic_archive_configured(ArchiveModuleState *state) >>> +basic_archive_configured(ArchiveModuleState *state, char **logdetail) >> >> Could we do something more like GUC_check_errdetail() instead to maintain >> backward compatibility with v16? > > 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? 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. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
Re: Improve the log message output of basic_archive when basic_archive.archive_directory parameter is not set
> On 26 Sep 2023, at 00:20, Nathan Bossart wrote: > > On Thu, Sep 21, 2023 at 11:18:00AM +0900, bt23nguyent wrote: >> -basic_archive_configured(ArchiveModuleState *state) >> +basic_archive_configured(ArchiveModuleState *state, char **logdetail) > > Could we do something more like GUC_check_errdetail() instead to maintain > backward compatibility with v16? 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
Re: Improve the log message output of basic_archive when basic_archive.archive_directory parameter is not set
On Thu, Sep 21, 2023 at 11:18:00AM +0900, bt23nguyent wrote: > -basic_archive_configured(ArchiveModuleState *state) > +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
Re: Improve the log message output of basic_archive when basic_archive.archive_directory parameter is not set
On 2023-09-20 21:14, Daniel Gustafsson wrote:
On 19 Sep 2023, at 11:21, bt23nguyent
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].
Yes! I totally missed this detail.
-archiving will proceed only when it returns
true.
+archiving will proceed only when it returns
true. 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
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 Nguyendiff --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.
-typedef bool (*ArchiveCheckConfiguredCB) (ArchiveModuleState *state);
+typedef bool (*ArchiveCheckConfiguredCB) (ArchiveModuleState *state, char **logdetail);
If true 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
In the latter case, the server will periodically call this function, and
-archiving will proceed only when it returns true.
+archiving will proceed only when it returns true. 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.
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,
Re: Improve the log message output of basic_archive when basic_archive.archive_directory parameter is not set
> On 19 Sep 2023, at 11:21, bt23nguyent 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 true.
+archiving will proceed only when it returns true. 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
Re: Improve the log message output of basic_archive when basic_archive.archive_directory parameter is not set
On 2023-09-15 23:38, Nathan Bossart wrote:
On Fri, Sep 15, 2023 at 02:48:55PM +0200, Daniel Gustafsson wrote:
On 15 Sep 2023, at 12:49, Alvaro Herrera
wrote:
On 2023-Sep-15, Daniel Gustafsson wrote:
-basic_archive_configured(ArchiveModuleState *state)
+basic_archive_configured(ArchiveModuleState *state, const char
**errmsg)
The variable name errmsg implies that it will contain the errmsg()
data when it
in fact is used for errhint() data, so it should be named
accordingly.
I have no objection to allowing this callback to provide additional
information, but IMHO this should use errdetail() instead of errhint().
In
the provided patch, the new message explains how the module is not
configured. It doesn't hint at how to fix it (although presumably one
could figure that out pretty easily).
It's probably better to define the interface as
ArchiveCheckConfiguredCB
functions returning an allocated string in the passed pointer which
the caller
is responsible for freeing.
That does seem more flexible.
Also note that this callback is documented in archive-modules.sgml,
so
that needs to be updated as well. This also means you can't
backpatch
this change, or you risk breaking external software that implements
this
interface.
Absolutely, this is master only for v17.
+1
Thank you for all of your comments!
They are all really constructive and I totally agree with the points you
brought up.
I have updated the patch accordingly.
Please let me know if you have any further suggestions that I can
improve more.
Best regards,
Tung Nguyen
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.
-typedef bool (*ArchiveCheckConfiguredCB) (ArchiveModuleState *state);
+typedef bool (*ArchiveCheckConfiguredCB) (ArchiveModuleState *state, char **logdetail);
If true 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
In the latter case, the server will periodically call this function, and
-archiving will proceed only when it returns true.
+archiving will proceed only when it returns true. The
+archiver may also emit the detail explaining how the module is not configured
+to the sever log if the archive module has any.
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 @@
Re: Improve the log message output of basic_archive when basic_archive.archive_directory parameter is not set
> On 15 Sep 2023, at 16:38, Nathan Bossart wrote: > this should use errdetail() instead of errhint(). In > the provided patch, the new message explains how the module is not > configured. It doesn't hint at how to fix it (although presumably one > could figure that out pretty easily). Fair point, I agree with your reasoning that errdetail seems more appropriate. -- Daniel Gustafsson
Re: Improve the log message output of basic_archive when basic_archive.archive_directory parameter is not set
On Fri, Sep 15, 2023 at 02:48:55PM +0200, Daniel Gustafsson wrote: >> On 15 Sep 2023, at 12:49, Alvaro Herrera wrote: >> >> On 2023-Sep-15, Daniel Gustafsson wrote: >> >>> -basic_archive_configured(ArchiveModuleState *state) >>> +basic_archive_configured(ArchiveModuleState *state, const char **errmsg) >>> >>> The variable name errmsg implies that it will contain the errmsg() data >>> when it >>> in fact is used for errhint() data, so it should be named accordingly. I have no objection to allowing this callback to provide additional information, but IMHO this should use errdetail() instead of errhint(). In the provided patch, the new message explains how the module is not configured. It doesn't hint at how to fix it (although presumably one could figure that out pretty easily). >>> It's probably better to define the interface as ArchiveCheckConfiguredCB >>> functions returning an allocated string in the passed pointer which the >>> caller >>> is responsible for freeing. That does seem more flexible. >> Also note that this callback is documented in archive-modules.sgml, so >> that needs to be updated as well. This also means you can't backpatch >> this change, or you risk breaking external software that implements this >> interface. > > Absolutely, this is master only for v17. +1 -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
Re: Improve the log message output of basic_archive when basic_archive.archive_directory parameter is not set
> On 15 Sep 2023, at 12:49, Alvaro Herrera wrote: > > On 2023-Sep-15, Daniel Gustafsson wrote: > >> -basic_archive_configured(ArchiveModuleState *state) >> +basic_archive_configured(ArchiveModuleState *state, const char **errmsg) >> >> The variable name errmsg implies that it will contain the errmsg() data when >> it >> in fact is used for errhint() data, so it should be named accordingly. >> >> It's probably better to define the interface as ArchiveCheckConfiguredCB >> functions returning an allocated string in the passed pointer which the >> caller >> is responsible for freeing. > > Also note that this callback is documented in archive-modules.sgml, so > that needs to be updated as well. This also means you can't backpatch > this change, or you risk breaking external software that implements this > interface. Absolutely, this is master only for v17. > I suggest that 'msg' shouldn't be a global variable. There's no need > for that AFAICS; but if there is, this is a terrible name for it. Agreed. -- Daniel Gustafsson
Re: Improve the log message output of basic_archive when basic_archive.archive_directory parameter is not set
On 2023-Sep-15, Daniel Gustafsson wrote: > -basic_archive_configured(ArchiveModuleState *state) > +basic_archive_configured(ArchiveModuleState *state, const char **errmsg) > > The variable name errmsg implies that it will contain the errmsg() data when > it > in fact is used for errhint() data, so it should be named accordingly. > > It's probably better to define the interface as ArchiveCheckConfiguredCB > functions returning an allocated string in the passed pointer which the caller > is responsible for freeing. Also note that this callback is documented in archive-modules.sgml, so that needs to be updated as well. This also means you can't backpatch this change, or you risk breaking external software that implements this interface. I suggest that 'msg' shouldn't be a global variable. There's no need for that AFAICS; but if there is, this is a terrible name for it. -- Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
Re: Improve the log message output of basic_archive when basic_archive.archive_directory parameter is not set
> On 15 Sep 2023, at 11:38, bt23nguyent wrote: > > Hi, > > When archive_library is set to 'basic_archive' but > basic_archive.archive_directory is not set, WAL archiving doesn't work and > only the following warning message is logged. > >$ emacs $PGDATA/postgresql.conf >archive_mode = on >archive_library = 'basic_archive' > >$ bin/pg_ctl -D $PGDATA restart > >WARNING: archive_mode enabled, yet archiving is not configured > > The issue here is that this warning message doesn't suggest any hint > regarding the cause of WAL archiving failure. In other words, I think that > the log message in this case should report that WAL archiving failed because > basic_archive.archive_directory is not set. That doesn't seem unreasonable, and I can imagine other callbacks having the need to give errhints as well to assist the user. > Thus, I think it's worth implementing new patch that improves that warning > message, and here is the patch for that. -basic_archive_configured(ArchiveModuleState *state) +basic_archive_configured(ArchiveModuleState *state, const char **errmsg) The variable name errmsg implies that it will contain the errmsg() data when it in fact is used for errhint() data, so it should be named accordingly. It's probably better to define the interface as ArchiveCheckConfiguredCB functions returning an allocated string in the passed pointer which the caller is responsible for freeing. -- Daniel Gustafsson
Improve the log message output of basic_archive when basic_archive.archive_directory parameter is not set
Hi,
When archive_library is set to 'basic_archive' but
basic_archive.archive_directory is not set, WAL archiving doesn't work
and only the following warning message is logged.
$ emacs $PGDATA/postgresql.conf
archive_mode = on
archive_library = 'basic_archive'
$ bin/pg_ctl -D $PGDATA restart
WARNING: archive_mode enabled, yet archiving is not configured
The issue here is that this warning message doesn't suggest any hint
regarding the cause of WAL archiving failure. In other words, I think
that the log message in this case should report that WAL archiving
failed because basic_archive.archive_directory is not set. Thus, I think
it's worth implementing new patch that improves that warning message,
and here is the patch for that.
Best regards,
Tung Nguyendiff --git a/contrib/basic_archive/basic_archive.c b/contrib/basic_archive/basic_archive.c
index 4d78c31859..dedc53c367 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, const char **errmsg);
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, const char **errmsg)
{
- return archive_directory != NULL && archive_directory[0] != '\0';
+ if (archive_directory == NULL || archive_directory[0] == '\0')
+{
+*errmsg = "WAL archiving failed because basic_archive.archive_directory is not set";
+return false;
+}
+else
+return true;
}
/*
diff --git a/src/backend/postmaster/pgarch.c b/src/backend/postmaster/pgarch.c
index 46af349564..100e33f48d 100644
--- a/src/backend/postmaster/pgarch.c
+++ b/src/backend/postmaster/pgarch.c
@@ -91,6 +91,7 @@ typedef struct PgArchData
} PgArchData;
char *XLogArchiveLibrary = "";
+char *msg;
/* --
@@ -410,10 +411,11 @@ 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, ))
{
ereport(WARNING,
- (errmsg("archive_mode enabled, yet archiving is not configured")));
+ (errmsg("archive_mode enabled, yet archiving is not configured")),
+ (msg != NULL) ? errhint("%s", msg) : 0);
return;
}
diff --git a/src/include/archive/archive_module.h b/src/include/archive/archive_module.h
index 679ce5a6db..7f7fbeae8a 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, const char **errmsg);
typedef bool (*ArchiveFileCB) (ArchiveModuleState *state, const char *file, const char *path);
typedef void (*ArchiveShutdownCB) (ArchiveModuleState *state);
