On 2023-09-20 21:14, Daniel Gustafsson wrote:
On 19 Sep 2023, at 11:21, bt23nguyent <bt23nguy...@oss.nttdata.com>
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
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);
