On Mon, Oct 10, 2022 at 1:17 PM Peter Eisentraut
<peter.eisentr...@enterprisedb.com> wrote:
>
> On 05.10.22 20:57, Nathan Bossart wrote:
> >> What we are talking about here is, arguably, a misconfiguration, so it
> >> should result in an error.
> >
> > Okay. What do you think about something like the attached?
The intent here looks reasonable to me. However, why should the user
be able to set both archive_command and archive_library in the first
place only to later fail in LoadArchiveLibrary() per the patch? IMO,
the check_hook() is the right way to disallow any sorts of GUC
misconfigurations, no?
FWIW, I'm attaching a small patch that uses check_hook().
> That looks like the right solution to me.
>
> Let's put that into PG 16, and maybe we can consider backpatching it.
+1 to backpatch to PG 15 where the archive modules feature was introduced.
--
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
From c5969071dfb9064bbf9e22513bf2cef57bcfd84f Mon Sep 17 00:00:00 2001
From: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Date: Thu, 13 Oct 2022 09:37:40 +0000
Subject: [PATCH v1] Handle misconfigurations of archive_command and
archive_library
The parameters archive_command and archive_library are mutually
exclusive. This patch errors out if the user is trying to set both
of them at a time.
---
doc/src/sgml/config.sgml | 11 +++++++----
src/backend/access/transam/xlog.c | 16 ++++++++++++++++
src/backend/postmaster/pgarch.c | 18 +++++++++++++++++-
src/backend/utils/misc/guc_tables.c | 4 ++--
src/include/utils/guc_hooks.h | 4 ++++
5 files changed, 46 insertions(+), 7 deletions(-)
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 66312b53b8..62e7d61da7 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -3597,9 +3597,10 @@ include_dir 'conf.d'
</para>
<para>
This parameter can only be set in the <filename>postgresql.conf</filename>
- file or on the server command line. It is ignored unless
- <varname>archive_mode</varname> was enabled at server start and
- <varname>archive_library</varname> is set to an empty string.
+ file or on the server command line. It is not allowed to set both
+ <varname>archive_command</varname> and <varname>archive_library</varname>
+ at the same time, doing so will cause an error. This parameter is ignored
+ unless <varname>archive_mode</varname> was enabled at server start.
If <varname>archive_command</varname> is an empty string (the default) while
<varname>archive_mode</varname> is enabled (and <varname>archive_library</varname>
is set to an empty string), WAL archiving is temporarily
@@ -3625,7 +3626,9 @@ include_dir 'conf.d'
The library to use for archiving completed WAL file segments. If set to
an empty string (the default), archiving via shell is enabled, and
<xref linkend="guc-archive-command"/> is used. Otherwise, the specified
- shared library is used for archiving. For more information, see
+ shared library is used for archiving. It is not allowed to set both
+ <varname>archive_library</varname> and <varname>archive_command</varname>
+ at the same time, doing so will cause an error. For more information, see
<xref linkend="backup-archiving-wal"/> and
<xref linkend="archive-modules"/>.
</para>
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 27085b15a8..64714c6940 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -4462,6 +4462,22 @@ show_archive_command(void)
return "(disabled)";
}
+/*
+ * GUC check_hook for archive_command
+ */
+bool
+check_archive_command(char **newval, void **extra, GucSource source)
+{
+ if (*newval && strcmp(*newval, "") != 0 && XLogArchiveLibrary[0] != '\0')
+ {
+ GUC_check_errmsg("cannot set \"archive_command\" when \"archive_library\" is specified");
+ GUC_check_errdetail("Only one of \"archive_command\" or \"archive_library\" can be specified.");
+ return false;
+ }
+
+ return true;
+}
+
/*
* GUC show_hook for in_hot_standby
*/
diff --git a/src/backend/postmaster/pgarch.c b/src/backend/postmaster/pgarch.c
index 3868cd7bd3..f8c05754a1 100644
--- a/src/backend/postmaster/pgarch.c
+++ b/src/backend/postmaster/pgarch.c
@@ -44,7 +44,7 @@
#include "storage/procsignal.h"
#include "storage/shmem.h"
#include "storage/spin.h"
-#include "utils/guc.h"
+#include "utils/guc_hooks.h"
#include "utils/memutils.h"
#include "utils/ps_status.h"
@@ -146,6 +146,22 @@ static int ready_file_comparator(Datum a, Datum b, void *arg);
static void LoadArchiveLibrary(void);
static void call_archive_module_shutdown_callback(int code, Datum arg);
+/*
+ * GUC check_hook for check_archive_library
+ */
+bool
+check_archive_library(char **newval, void **extra, GucSource source)
+{
+ if (*newval && strcmp(*newval, "") != 0 && XLogArchiveCommand[0] != '\0')
+ {
+ GUC_check_errmsg("cannot set \"archive_library\" when \"archive_command\" is specified");
+ GUC_check_errdetail("Only one of \"archive_library\" or \"archive_command\" can be specified.");
+ return false;
+ }
+
+ return true;
+}
+
/* Report shared memory space needed by PgArchShmemInit */
Size
PgArchShmemSize(void)
diff --git a/src/backend/utils/misc/guc_tables.c b/src/backend/utils/misc/guc_tables.c
index 05ab087934..2ec327f41e 100644
--- a/src/backend/utils/misc/guc_tables.c
+++ b/src/backend/utils/misc/guc_tables.c
@@ -3702,7 +3702,7 @@ struct config_string ConfigureNamesString[] =
},
&XLogArchiveCommand,
"",
- NULL, NULL, show_archive_command
+ check_archive_command, NULL, show_archive_command
},
{
@@ -3712,7 +3712,7 @@ struct config_string ConfigureNamesString[] =
},
&XLogArchiveLibrary,
"",
- NULL, NULL, NULL
+ check_archive_library, NULL, NULL
},
{
diff --git a/src/include/utils/guc_hooks.h b/src/include/utils/guc_hooks.h
index f1a9a183b4..daab4d0a0d 100644
--- a/src/include/utils/guc_hooks.h
+++ b/src/include/utils/guc_hooks.h
@@ -29,6 +29,10 @@ extern bool check_application_name(char **newval, void **extra,
GucSource source);
extern void assign_application_name(const char *newval, void *extra);
extern const char *show_archive_command(void);
+extern bool check_archive_command(char **newval, void **extra,
+ GucSource source);
+extern bool check_archive_library(char **newval, void **extra,
+ GucSource source);
extern bool check_autovacuum_max_workers(int *newval, void **extra,
GucSource source);
extern bool check_autovacuum_work_mem(int *newval, void **extra,
--
2.34.1
