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