Content-Type: text/plain;
	charset=us-ascii
Mime-Version: 1.0 (Mac OS X Mail 16.0 \(3696.120.41.1.3\))
Subject: Re: Improve the log message output of basic_archive when
 basic_archive.archive_directory parameter is not set
From: Daniel Gustafsson <daniel@yesql.se>
In-Reply-To: <202309151049.dmfsxc5b3wyf@alvherre.pgsql>
Date: Fri, 15 Sep 2023 14:48:55 +0200
Cc: bt23nguyent <bt23nguyent@oss.nttdata.com>,
 pgsql-hackers@lists.postgresql.org
Content-Transfer-Encoding: quoted-printable
Message-Id: <7035FAFE-7E91-4B31-AA72-75EE82DB3F7B@yesql.se>
References: <202309151049.dmfsxc5b3wyf@alvherre.pgsql>
To: Alvaro Herrera <alvherre@alvh.no-ip.org>
Archived-At: <https://www.postgresql.org/message-id/7035FAFE-7E91-4B31-AA72-75EE82DB3F7B%40yesql.se>
Precedence: bulk

> On 15 Sep 2023, at 12:49, Alvaro Herrera <alvherre@alvh.no-ip.org> =
wrote:
>=20
> On 2023-Sep-15, Daniel Gustafsson wrote:
>=20
>> -basic_archive_configured(ArchiveModuleState *state)
>> +basic_archive_configured(ArchiveModuleState *state, const char =
**errmsg)
>>=20
>> 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.
>>=20
>> 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.
>=20
> 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