Re: recovery modules

On Mon, Jan 23, 2023 at 01:44:28PM -0800, Nathan Bossart wrote:
> On Mon, Jan 23, 2023 at 11:44:57AM +0900, Michael Paquier wrote:
> I updated the documentation as you suggested, and I renamed basic_archive
> to basic_wal_module.

Thanks. The renaming of basic_archive to basic_wal_module looked
fine, so applied.

While looking at the docs, I found a bit confusing that the main
section of the WAL modules included the full description for the
archive modules, so I have moved that into the sect2 dedicated to the
archive modules instead, as of the attached. 0004 has been updated in
consequence, with details about the recovery bits within its own
sect2.

>> (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
>> - errmsg("must specify restore_command when standby mode is not enabled")));
>> + errmsg("must specify restore_command or a restore_library that defines "
>> + "a restore callback when standby mode is not enabled")));
>> This is long. Shouldn't this be split into an errdetail() to list all
>> the options at hand?
>
> Should the errmsg() be something like "recovery parameters misconfigured"?

Hmm. Here is an idea:
- errmsg: "must specify restore option when standby mode is not enabled"
- errdetail: "Either restore_command or restore_library need to be
specified."

>> - if (XLogArchiveLibrary[0] != '\0' && XLogArchiveCommand[0] != '\0')
>> - ereport(ERROR,
>> - (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
>> - errmsg("both archive_command and archive_library set"),
>> - errdetail("Only one of archive_command, archive_library may be set.")));
>> + CheckMutuallyExclusiveGUCs(XLogArchiveLibrary, "archive_library",
>> + XLogArchiveCommand, "archive_command");
>>
>> The introduction of this routine could be a patch on its own, as it
>> impacts the archiving path.
>
> I moved this to a separate patch.

While pondering about that, I found a bit sad that this only works for
string GUCs, while it could be possible to do the same kind of checks
for the other GUC types with a more generic routine? Not enum,
obviously, but int, float, bool and real, with the a failure if both
GUCs are set to non-default values? Also, and I may be missing
something here, do we really need to pass the value of the parameters
to check? Wouldn't it be enough to check for the case where both
parameters are set to their non-default values after reloading?

>> - Startup reloading, as of StartupRereadConfig(). This code could
>> involve a WAL receiver restart depending on a change in the slot
>> change or in primary_conninfo, and force at the same time an ERROR
>> because of conflicting recovery library and command configuration.
>> This one should be safe because pendingWalRcvRestart would just be
>> considered later on by the startup process itself while waiting for
>> WAL to become available. Still this could deserve a comment? Even if
>> there is a misconfiguration, a reload on a standby would enforce a
>> FATAL in the startup process, taking down the whole server.
>
> Do you think the parameter checks should go before the WAL receiver restart
> logic?

Yeah, switching the order makes the logic more robust IMO.

>> - Checkpointer initialization, as of CheckpointerMain(). A
>> configuration failure in this code path, aka server startup, causes
>> the server to loop infinitely on FATAL with the misconfiguration
>> showing up all the time.. This is a problem.
>
> Perhaps this is a reason to move the parameter check in CheckpointerMain()
> to after the sigsetjmp() block. This should avoid full server restarts.
> Only the checkpointer process would loop with the ERROR.

The loop part is annoying.. I've never been a fan of adding this
cross-value checks for the archiver modules in the first place, and it
would make things much simpler in the checkpointer if we need to think
about that as we want these values to be reloadable. Perhaps this
could just be an exception where we just give priority on one over the
other archive_cleanup_command? The startup process has a well-defined
sequence after a failure, while the checkpointer is designed to remain
robust.

>> - Last comes the checkpointer GUC reloading, as of
>> HandleCheckpointerInterrupts(), with a second problem. This
>> introduces a failure path where ConfigReloadPending is processed at
>> the same time as ShutdownRequestPending based on the way it is coded,
>> interacting with what would be a normal shutdown in some cases? And
>> actually, if you enforce a misconfiguration on reload, the
>> checkpointer reports an error but it does not enforce a process
>> restart, hence it keeps around the new, incorrect, configuration while
>> waiting for a new checkpoint to happen once restore_library and
>> archive_cleanup_command are set. This could lead to surprises, IMO.
>> Upgrading to a FATAL in this code path triggers an infinite loop, like
>> the startup path.
>
> If we move the parameter check in CheckpointerMain() as described above,
> the checkpointer should be unable to proceed with an incorrect
> configuration. For the normal shutdown part, do you think the
> ShutdownRequestPending block should be moved to before the
> ConfigReloadPending block in HandleCheckpointerInterrupts()?

I would not touch this order. This could influence the setup a
shutdown checkpoint relies on, for one.

>> If the archive_cleanup_command ballback of a restore library triggers
>> a FATAL, it seems to me that it would continuously trigger a server
>> restart, actually. Perhaps that's something to document, in
>> comparison to the safe fallbacks of the shell command where we don't
>> force an ERROR to give priority to the stability of the checkpointer.
>
> I'm not sure it's worth documenting that ereport(FATAL, ...) in the
> checkpointer process will cause a server restart. In most cases, an
> extension author would use ERROR, which, if we make the aforementioned
> changes, would at most cause the checkpointer to effectively restart. This
> is similar to archive modules where an ERROR causes only the archiver
> process to restart. Also, we document that recovery libraries are loaded
> in the startup and checkpointer processes, so IMO it should be relatively
> apparent that doing something like FATAL or proc_exit() is bad.

Okay. Fine by me. This could always be amended later, as required.

+ if (recoveryRestoreCommand[0] == '\0')
+ RecoveryContext.restore_cb = NULL;
+ if (archiveCleanupCommand[0] == '\0')
+ RecoveryContext.archive_cleanup_cb = NULL;
+ if (recoveryEndCommand[0] == '\0')
+ RecoveryContext.recovery_end_cb = NULL;

Could it be cleaner to put this knowledge directly in shell_restore.c
with a fast-exit path after entering each callback? It does not
strike me as a good thing to sprinkle more than necessary the
knowledge about the commands.

Another question that popped in my mind: could it be better to have
two different shutdown callbacks for the checkpointer and the startup
process? Having some tests for both, like shell_archive.c, would be
nice, actually.
--
Michael