On Wed, Sep 14, 2022 at 06:12:09PM -0400, Tom Lane wrote:
> Nathan Bossart <nathandboss...@gmail.com> writes:
>> On Wed, Sep 14, 2022 at 04:47:23PM -0400, Tom Lane wrote:
>>> Yeah, the objection there is only to trying to enforce such
>>> interrelationships in GUC hooks. In this case it seems to me that
>>> we could easily check and complain at the point where we're about
>>> to use the GUC values.
>
>> I think the cleanest way to do something like that would be to load a
>> check_configured_cb that produces a WARNING. IIRC failing in
>> LoadArchiveLibrary() would just cause the archiver process to restart over
>> and over. HandlePgArchInterrupts() might need some work as well.
>
> Hm. Maybe consistency-check these settings in the postmaster, sometime
> after we've absorbed all GUC settings but before we launch any children?
> That could provide a saner implementation for the recovery_target_*
> variables too.
Both archive_command and archive_library are PGC_SIGHUP, so IIUC that
wouldn't be sufficient. I attached a quick sketch that seems to provide
the desired behavior. It's nowhere near committable yet, but it
demonstrates what I'm thinking.
For recovery_target_*, something like you are describing seems reasonable.
I believe PostmasterMain() already performs some similar checks.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
diff --git a/src/backend/postmaster/pgarch.c b/src/backend/postmaster/pgarch.c
index 6ce361707d..1d0c6029a5 100644
--- a/src/backend/postmaster/pgarch.c
+++ b/src/backend/postmaster/pgarch.c
@@ -422,8 +422,15 @@ pgarch_ArchiverCopyLoop(void)
HandlePgArchInterrupts();
/* can't do anything if not configured ... */
- if (ArchiveContext.check_configured_cb != NULL &&
- !ArchiveContext.check_configured_cb())
+ if (XLogArchiveLibrary[0] != '\0' && XLogArchiveCommand[0] != '\0')
+ {
+ ereport(WARNING,
+ (errmsg("archive_mode enabled, but archiving is misconfigured"),
+ errdetail("Only one of archive_command, archive_library may be set.")));
+ return;
+ }
+ else if (ArchiveContext.check_configured_cb != NULL &&
+ !ArchiveContext.check_configured_cb())
{
ereport(WARNING,
(errmsg("archive_mode enabled, yet archiving is not configured")));
@@ -794,6 +801,9 @@ HandlePgArchInterrupts(void)
{
char *archiveLib = pstrdup(XLogArchiveLibrary);
bool archiveLibChanged;
+ bool misconfiguredBeforeReload = (XLogArchiveCommand[0] != '\0' &&
+ XLogArchiveLibrary[0] != '\0');
+ bool misconfiguredAfterReload;
ConfigReloadPending = false;
ProcessConfigFile(PGC_SIGHUP);
@@ -801,7 +811,11 @@ HandlePgArchInterrupts(void)
archiveLibChanged = strcmp(XLogArchiveLibrary, archiveLib) != 0;
pfree(archiveLib);
- if (archiveLibChanged)
+ misconfiguredAfterReload = (XLogArchiveCommand[0] != '\0' &&
+ XLogArchiveLibrary[0] != '\0');
+
+ if ((archiveLibChanged && !misconfiguredAfterReload) ||
+ misconfiguredBeforeReload != misconfiguredAfterReload)
{
/*
* Call the currently loaded archive module's shutdown callback,
@@ -816,10 +830,17 @@ HandlePgArchInterrupts(void)
* internal_load_library()). To deal with this, we simply restart
* the archiver. The new archive module will be loaded when the
* new archiver process starts up.
+ *
+ * Similarly, we restart the archiver if our misconfiguration status
+ * changes. If the parameters were misconfigured but are no longer,
+ * we must restart to load the correct callbacks. If the parameters
+ * weren't misconfigured but now are, we must restart to unload the
+ * current callbacks.
*/
ereport(LOG,
(errmsg("restarting archiver process because value of "
- "\"archive_library\" was changed")));
+ "\"archive_library\" or \"archive_command\" was "
+ "changed")));
proc_exit(0);
}
@@ -838,6 +859,14 @@ LoadArchiveLibrary(void)
memset(&ArchiveContext, 0, sizeof(ArchiveModuleCallbacks));
+ /*
+ * If both a shell command and an archive library are specified, it is not
+ * clear what we should do, so do nothing. The archiver will emit WARNINGs
+ * about the misconfiguration.
+ */
+ if (XLogArchiveLibrary[0] != '\0' && XLogArchiveCommand[0] != '\0')
+ return;
+
/*
* If shell archiving is enabled, use our special initialization function.
* Otherwise, load the library and call its _PG_archive_module_init().
