public inbox for [email protected]  

help / color / mirror / Atom feed

From: Nathan Bossart <[email protected]>
To: Daniel Gustafsson <[email protected]>
Cc: Alvaro Herrera <[email protected]>
Cc: bt23nguyent <[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: Fri, 15 Sep 2023 07:38:27 -0700
Message-ID: <20230915143827.GA1912920@nathanxps13> (raw)
In-Reply-To: <[email protected]>
References: <[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

view thread (6+ 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: <20230915143827.GA1912920@nathanxps13>

* 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