public inbox for [email protected]
help / color / mirror / Atom feedFrom: bt23nguyent <[email protected]>
To: Daniel Gustafsson <[email protected]>
Cc: Nathan Bossart <[email protected]>
Cc: Alvaro Herrera <[email protected]>
Cc: [email protected]
Subject: Re: Improve the log message output of basic_archive when basic_archive.archive_directory parameter is not set
Date: Thu, 21 Sep 2023 11:18:00 +0900
Message-ID: <[email protected]> (raw)
In-Reply-To: <[email protected]>
References: <[email protected]>
<[email protected]>
<20230915143827.GA1912920@nathanxps13>
<[email protected]>
<[email protected]>
On 2023-09-20 21:14, Daniel Gustafsson wrote:
>> On 19 Sep 2023, at 11:21, bt23nguyent <[email protected]>
>> 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
> <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
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
Attachments:
[text/x-diff] v3-0001-Improve-log-message-output-of-basic-archive.patch (5.1K, ../[email protected]/2-v3-0001-Improve-log-message-output-of-basic-archive.patch)
download | inline diff:
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);
view thread (11+ messages) latest in thread
reply
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: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: Improve the log message output of basic_archive when basic_archive.archive_directory parameter is not set
In-Reply-To: <[email protected]>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox