Re: Improve the log message output of basic_archive when basic_archive.archive_directory parameter is not set

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 <alvherre@alvh.no-ip.org> 
>>> 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