pg.ddx.io pgsql-hackers@postgresql.org mailing list archive help / color / mirror / Atom feed From: Nathan Bossart <nathandbossart@gmail.com>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Peter Eisentraut <peter.eisentraut@enterprisedb.com>
Cc: Benoit Lobréau <benoit.lobreau@gmail.com>
Cc: Alvaro Herrera <alvherre@alvh.no-ip.org>
Cc: Robert Haas <robertmhaas@gmail.com>
Cc: Bossart, Nathan <bossartn@amazon.com>
Cc: David Steele <david@pgmasters.net>
Cc: Fujii Masao <masao.fujii@oss.nttdata.com>
Cc: pgsql-hackers@postgresql.org <pgsql-hackers@postgresql.org>
Cc: Michael Paquier <michael@paquier.xyz>
Cc: Magnus Hagander <magnus@hagander.net>
Cc: Stephen Frost <sfrost@snowman.net>
Subject: Re: archive modules
Date: Wed, 14 Sep 2022 15:27:36 -0700
Message-ID: <20220914222736.GA3042279@nathanxps13> (raw )
In-Reply-To: <2910705.1663193529@sss.pgh.pa.us >
References: <CAPE8EZ7SQKKkreVT2Cjrheg8dChT_qSDRBS71r-r+girEMQ4bw@mail.gmail.com >
<20220830163048.GA544233@nathanxps13 >
<613541.1662842656@sss.pgh.pa.us >
<20220910211046.GA2651323@nathanxps13 >
<9ee5d180-2c32-a1ca-d3d7-63a723f68d9a@enterprisedb.com >
<20220914200305.GA2984249@nathanxps13 >
<37857474-5caa-ccb3-92a3-150ddfe851ab@enterprisedb.com >
<2884265.1663188443@sss.pgh.pa.us >
<20220914211746.GA2986134@nathanxps13 >
<2910705.1663193529@sss.pgh.pa.us >
On Wed, Sep 14, 2022 at 06:12:09PM -0400, Tom Lane wrote:
> Nathan Bossart <nathandbossart@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
Attachments:
[text/x-diff] handle_archive_misconfiguration.patch (3.0K, ../20220914222736.GA3042279@nathanxps13/2-handle_archive_misconfiguration.patch)
download | inline diff:
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().
view thread (102+ messages) latest in thread
Message-ID: <20220914222736.GA3042279@nathanxps13>
Permalink: ../20220914222736.GA3042279@nathanxps13/
Also on: postgresql.org/message-id/20220914222736.GA3042279@nathanxps13
copy link · copy postgr.es · copy git am
reply Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: nathandbossart@gmail.com, tgl@sss.pgh.pa.us, peter.eisentraut@enterprisedb.com, benoit.lobreau@gmail.com, alvherre@alvh.no-ip.org, robertmhaas@gmail.com, bossartn@amazon.com, david@pgmasters.net, masao.fujii@oss.nttdata.com, michael@paquier.xyz, magnus@hagander.net, sfrost@snowman.net
Subject: Re: archive modules
In-Reply-To: <20220914222736.GA3042279@nathanxps13>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox