public inbox for [email protected]
help / color / mirror / Atom feed[PATCH 17/18] *an old and "deleted [has] happened"
6+ messages / 5 participants
[nested] [flat]
* [PATCH 17/18] *an old and "deleted [has] happened"
@ 2021-02-06 21:17 Justin Pryzby <[email protected]>
0 siblings, 0 replies; 6+ messages in thread
From: Justin Pryzby @ 2021-02-06 21:17 UTC (permalink / raw)
Heikki missed this in 6b387179baab8d0e5da6570678eefbe61f3acc79
---
doc/src/sgml/protocol.sgml | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/doc/src/sgml/protocol.sgml b/doc/src/sgml/protocol.sgml
index 3763b4b995..a51f2c9920 100644
--- a/doc/src/sgml/protocol.sgml
+++ b/doc/src/sgml/protocol.sgml
@@ -6928,8 +6928,8 @@ Delete
</term>
<listitem>
<para>
- Identifies the following TupleData message as a old tuple.
- This field is present if the table in which the delete has
+ Identifies the following TupleData message as an old tuple.
+ This field is present if the table in which the delete
happened has REPLICA IDENTITY set to FULL.
</para>
</listitem>
--
2.17.0
--lc9FT7cWel8HagAv
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
filename="0018-doc-review-for-pg_stat_progress_create_index.patch"
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: Improve the log message output of basic_archive when basic_archive.archive_directory parameter is not set
@ 2023-09-15 10:49 Alvaro Herrera <[email protected]>
2023-09-15 12:48 ` Re: Improve the log message output of basic_archive when basic_archive.archive_directory parameter is not set Daniel Gustafsson <[email protected]>
0 siblings, 1 reply; 6+ messages in thread
From: Alvaro Herrera @ 2023-09-15 10:49 UTC (permalink / raw)
To: Daniel Gustafsson <[email protected]>; +Cc: bt23nguyent <[email protected]>; [email protected]
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/
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: Improve the log message output of basic_archive when basic_archive.archive_directory parameter is not set
2023-09-15 10:49 Re: Improve the log message output of basic_archive when basic_archive.archive_directory parameter is not set Alvaro Herrera <[email protected]>
@ 2023-09-15 12:48 ` Daniel Gustafsson <[email protected]>
2023-09-15 14:38 ` Re: Improve the log message output of basic_archive when basic_archive.archive_directory parameter is not set Nathan Bossart <[email protected]>
0 siblings, 1 reply; 6+ messages in thread
From: Daniel Gustafsson @ 2023-09-15 12:48 UTC (permalink / raw)
To: Alvaro Herrera <[email protected]>; +Cc: bt23nguyent <[email protected]>; [email protected]
> 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.
>>
>> 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
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: Improve the log message output of basic_archive when basic_archive.archive_directory parameter is not set
2023-09-15 10:49 Re: Improve the log message output of basic_archive when basic_archive.archive_directory parameter is not set Alvaro Herrera <[email protected]>
2023-09-15 12:48 ` Re: Improve the log message output of basic_archive when basic_archive.archive_directory parameter is not set Daniel Gustafsson <[email protected]>
@ 2023-09-15 14:38 ` Nathan Bossart <[email protected]>
2023-09-15 14:41 ` Re: Improve the log message output of basic_archive when basic_archive.archive_directory parameter is not set Daniel Gustafsson <[email protected]>
2023-09-19 09:21 ` Re: Improve the log message output of basic_archive when basic_archive.archive_directory parameter is not set bt23nguyent <[email protected]>
0 siblings, 2 replies; 6+ messages in thread
From: Nathan Bossart @ 2023-09-15 14:38 UTC (permalink / raw)
To: Daniel Gustafsson <[email protected]>; +Cc: Alvaro Herrera <[email protected]>; bt23nguyent <[email protected]>; [email protected]
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
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: Improve the log message output of basic_archive when basic_archive.archive_directory parameter is not set
2023-09-15 10:49 Re: Improve the log message output of basic_archive when basic_archive.archive_directory parameter is not set Alvaro Herrera <[email protected]>
2023-09-15 12:48 ` Re: Improve the log message output of basic_archive when basic_archive.archive_directory parameter is not set Daniel Gustafsson <[email protected]>
2023-09-15 14:38 ` Re: Improve the log message output of basic_archive when basic_archive.archive_directory parameter is not set Nathan Bossart <[email protected]>
@ 2023-09-15 14:41 ` Daniel Gustafsson <[email protected]>
1 sibling, 0 replies; 6+ messages in thread
From: Daniel Gustafsson @ 2023-09-15 14:41 UTC (permalink / raw)
To: Nathan Bossart <[email protected]>; +Cc: Alvaro Herrera <[email protected]>; bt23nguyent <[email protected]>; [email protected]
> On 15 Sep 2023, at 16:38, Nathan Bossart <[email protected]> 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
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: Improve the log message output of basic_archive when basic_archive.archive_directory parameter is not set
2023-09-15 10:49 Re: Improve the log message output of basic_archive when basic_archive.archive_directory parameter is not set Alvaro Herrera <[email protected]>
2023-09-15 12:48 ` Re: Improve the log message output of basic_archive when basic_archive.archive_directory parameter is not set Daniel Gustafsson <[email protected]>
2023-09-15 14:38 ` Re: Improve the log message output of basic_archive when basic_archive.archive_directory parameter is not set Nathan Bossart <[email protected]>
@ 2023-09-19 09:21 ` bt23nguyent <[email protected]>
1 sibling, 0 replies; 6+ messages in thread
From: bt23nguyent @ 2023-09-19 09:21 UTC (permalink / raw)
To: Nathan Bossart <[email protected]>; +Cc: Daniel Gustafsson <[email protected]>; Alvaro Herrera <[email protected]>; [email protected]
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);
^ permalink raw reply [nested|flat] 6+ messages in thread
end of thread, other threads:[~2023-09-19 09:21 UTC | newest]
Thread overview: 6+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2021-02-06 21:17 [PATCH 17/18] *an old and "deleted [has] happened" Justin Pryzby <[email protected]>
2023-09-15 10:49 Re: Improve the log message output of basic_archive when basic_archive.archive_directory parameter is not set Alvaro Herrera <[email protected]>
2023-09-15 12:48 ` Re: Improve the log message output of basic_archive when basic_archive.archive_directory parameter is not set Daniel Gustafsson <[email protected]>
2023-09-15 14:38 ` Re: Improve the log message output of basic_archive when basic_archive.archive_directory parameter is not set Nathan Bossart <[email protected]>
2023-09-15 14:41 ` Re: Improve the log message output of basic_archive when basic_archive.archive_directory parameter is not set Daniel Gustafsson <[email protected]>
2023-09-19 09:21 ` Re: Improve the log message output of basic_archive when basic_archive.archive_directory parameter is not set bt23nguyent <[email protected]>
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox