public inbox for [email protected]
help / color / mirror / Atom feedFrom: bt23nguyent <[email protected]>
To: Nathan Bossart <[email protected]>
Cc: Daniel Gustafsson <[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: Tue, 19 Sep 2023 18:21:59 +0900
Message-ID: <[email protected]> (raw)
In-Reply-To: <20230915143827.GA1912920@nathanxps13>
References: <[email protected]>
<[email protected]>
<20230915143827.GA1912920@nathanxps13>
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 <[email protected]>
>>> 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
Attachments:
[text/x-diff] v2-0001-Improve-log-message-output-of-basic-archive.patch (5.0K, ../[email protected]/2-v2-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..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.
<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,9 @@ 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.
</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 (6+ messages)
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