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: <4109578306242a7cd5661171647e11b2@oss.nttdata.com>
Date: Fri, 15 Sep 2023 11:57:31 +0200
Cc: pgsql-hackers@lists.postgresql.org
Content-Transfer-Encoding: quoted-printable
Message-Id: <3395F53B-934F-47F9-9BA9-E19AC3842937@yesql.se>
References: <4109578306242a7cd5661171647e11b2@oss.nttdata.com>
To: bt23nguyent <bt23nguyent@oss.nttdata.com>
Archived-At: <https://www.postgresql.org/message-id/3395F53B-934F-47F9-9BA9-E19AC3842937%40yesql.se>
Precedence: bulk

> On 15 Sep 2023, at 11:38, bt23nguyent <bt23nguyent@oss.nttdata.com> =
wrote:
>=20
> Hi,
>=20
> When archive_library is set to 'basic_archive' but =
basic_archive.archive_directory is not set, WAL archiving doesn't work =
and only the following warning message is logged.
>=20
>    $ emacs $PGDATA/postgresql.conf
>    archive_mode =3D on
>    archive_library =3D 'basic_archive'
>=20
>    $ bin/pg_ctl -D $PGDATA restart
>    ....
>    WARNING:  archive_mode enabled, yet archiving is not configured
>=20
> The issue here is that this warning message doesn't suggest any hint =
regarding the cause of WAL archiving failure. In other words, I think =
that the log message in this case should report that WAL archiving =
failed because basic_archive.archive_directory is not set.

That doesn't seem unreasonable, and I can imagine other callbacks having =
the
need to give errhints as well to assist the user.

> Thus, I think it's worth implementing new patch that improves that =
warning message, and here is the patch for that.

-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.

--
Daniel Gustafsson