Re: archive modules - Mailing list pgsql-hackers

On Mon, Oct 17, 2022 at 11:20 AM Michael Paquier <michael@paquier.xyz> wrote:
>
> 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.

Yes, ERROR or FATAL it really doesn't matter, the process exits see,
pg_re_throw(), for archiver PG_exception_stack is null.
2022-10-18 09:57:41.869 UTC [2479104] FATAL:  both archive_command and
archive_library specified
2022-10-18 09:57:41.869 UTC [2479104] DETAIL:  Only one of
archive_command, archive_library may be set.

I think Kyotaro-san's concern is to place errmsg("both archive_command
and archive_library specified"), before errmsg("restarting archiver
process because value of \"archive_library\" was changed", something
like the attached v4 patch.

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

Are you saying that we make/treat/build shell_archive.c as a separate
shared library/module (instead of just an object file) and load it in
pgarc.c? If yes, this can make pgarch.c simple.

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

If it's done as said above, the corresponding check_configured_cb()
can deal with allowing/disallowing/misconfiguring various parameters.

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

Great thought! If the responsibility of
allowing/disallowing/misconfiguring various parameters is given to
check_configured_cb(), the modules can decide whether to error out or
deal with it or use it.

--
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com