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
signature.asc
Description: PGP signature