On Mon, Oct 17, 2022 at 01:46:39PM +0900, Kyotaro Horiguchi wrote:
> As the code written, when archive library is being added while archive
> command is already set, archiver first emits seemingly positive
> message "restarting archive process because of..", then errors out
> after the resatart and keep restarting with complaining for the wrong
> setting. I think we don't need the first message.
>
> The ERROR always turns into FATAL, so FATAL would less confusing here,
> maybe.
You mean the second message in HandlePgArchInterrupts() when
archiveLibChanged is false? An ERROR or a FATAL would not change much
as there is a proc_exit() anyway down the road.
+ if (XLogArchiveLibrary[0] != '\0' && XLogArchiveCommand[0] != '\0')
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("both archive_command and archive_library specified"),
+ errdetail("Only one of archive_command, archive_library may be set.")));
So, backpedalling from upthread where Peter mentions that we should
complain if both archive_command and archive_library are set (creating
a parallel with recovery parameters), I'd like to think that pgarch.c
should have zero knowledge of what an archive_command is and should
just handle the library part. This makes the whole reasoning around
what pgarch.c should be much simpler, aka it just needs to know about
archive *libraries*, not *commands*. That's the kind of business that
check_configured_cb() is designed for, actually, as far as I
understand, or this callback could just be removed entirely for the
same effect, as there would be no point in having pgarch.c do its
thing without archive_library or archive_command where a WARNING is
issued in the default case (shell_archive with no archive_command).
And, by the way, this patch would prevent the existence of archive
modules that need to be loaded but *want* an archive_command with
what they want to achieve. That does not strike me as a good idea if
we want to have a maximum of flexibility with this facility. I think
that for all that, we should put the responsability of what should be
set or not set directly to the modules, aka basic_archive could
complain if archive_command is set, but that does not strike me as a
mandatory requirement, either. It is true that archive_library has
been introduced as a way to avoid using archive_command, but the point
of creating a stronger dependency between both would be IMO annoying
in the long-term.
--
Michael
