On Wed, Jun 05, 2024 at 11:38:48PM -0400, Greg Sabino Mullane wrote:
> On Wed, Jun 5, 2024 at 3:18 PM Nathan Bossart <nathandboss...@gmail.com>
> wrote:
>> Could we remove the requirement that --single must be first? I'm not
>> thrilled about adding a list of "must be first" options that needs to stay
>> updated, but given this list probably doesn't change too frequently, maybe
>> that's still better than a more invasive patch to allow specifying these
>> options in any order...
>
> It would be nice, and I briefly looked into removing the "first"
> requirement, but src/backend/tcop/postgres.c for one assumes that --single
> is always argv[1], and it seemed not worth the extra effort to make it work
> for argv[N] instead of argv[1]. I don't mind it being the first argument,
> but that confusing error message needs to go.
I spent some time trying to remove the must-be-first requirement and came
up with the attached draft-grade patch. However, there's a complication:
the "database" option for single-user mode must still be listed last, at
least on systems where getopt() doesn't move non-options to the end of the
array. My previous research [0] indicated that this is pretty common, and
I noticed it because getopt() on macOS doesn't seem to reorder non-options.
I thought about changing these to getopt_long(), which we do rely on to
reorder non-options, but that conflicts with our ParseLongOption() "long
argument simulation" that we use to allow specifying arbitrary GUCs via the
command-line.
This remaining discrepancy might be okay, but I was really hoping to reduce
the burden on users to figure out the correct ordering of options. The
situations in which I've had to use single-user mode are precisely the
situations in which I'd rather not have to spend time learning these kinds
of details.
[0] https://postgr.es/m/20230609232257.GA121461%40nathanxps13
--
nathan
>From 44f8747a75197b3842c41f77503fa8389ba8db3e Mon Sep 17 00:00:00 2001
From: Nathan Bossart <nat...@postgresql.org>
Date: Mon, 17 Jun 2024 21:20:13 -0500
Subject: [PATCH 1/1] allow --single, etc. in any order
---
src/backend/bootstrap/bootstrap.c | 12 ++--
src/backend/main/main.c | 101 ++++++++++++++++++++----------
src/backend/tcop/postgres.c | 15 ++---
3 files changed, 77 insertions(+), 51 deletions(-)
diff --git a/src/backend/bootstrap/bootstrap.c
b/src/backend/bootstrap/bootstrap.c
index 986f6f1d9c..53011b4300 100644
--- a/src/backend/bootstrap/bootstrap.c
+++ b/src/backend/bootstrap/bootstrap.c
@@ -210,13 +210,6 @@ BootstrapModeMain(int argc, char *argv[], bool check_only)
/* Set defaults, to be overridden by explicit options below */
InitializeGUCOptions();
- /* an initial --boot or --check should be present */
- Assert(argc > 1
- && (strcmp(argv[1], "--boot") == 0
- || strcmp(argv[1], "--check") == 0));
- argv++;
- argc--;
-
while ((flag = getopt(argc, argv, "B:c:d:D:Fkr:X:-:")) != -1)
{
switch (flag)
@@ -230,6 +223,11 @@ BootstrapModeMain(int argc, char *argv[], bool check_only)
char *name,
*value;
+ /* --boot and --check were already
processed in main() */
+ if (strcmp(optarg, "boot") == 0 ||
+ strcmp(optarg, "check") == 0)
+ break;
+
ParseLongOption(optarg, &name, &value);
if (!value)
{
diff --git a/src/backend/main/main.c b/src/backend/main/main.c
index bfd0c5ed65..b893a9f298 100644
--- a/src/backend/main/main.c
+++ b/src/backend/main/main.c
@@ -57,7 +57,17 @@ static void check_root(const char *progname);
int
main(int argc, char *argv[])
{
- bool do_check_root = true;
+ bool check = false;
+ bool boot = false;
+#ifdef EXEC_BACKEND
+ bool forkchild = false;
+#endif
+ bool describe_config = false;
+ bool single = false;
+ bool show_help = false;
+ bool version = false;
+ bool check_guc = false;
+ int modes_set = 0;
reached_main = true;
@@ -136,61 +146,86 @@ main(int argc, char *argv[])
*/
unsetenv("LC_ALL");
+ for (int i = 1; i < argc; i++)
+ {
+ modes_set++;
+
+ if (strcmp(argv[i], "--check") == 0)
+ check = true;
+ else if (strcmp(argv[i], "--boot") == 0)
+ boot = true;
+#ifdef EXEC_BACKEND
+ else if (strncmp(argv[i], "--forkchild", 11) == 0)
+ forkchild = true;
+#endif
+ else if (strcmp(argv[i], "--describe-config") == 0)
+ describe_config = true;
+ else if (strcmp(argv[i], "--single") == 0)
+ single = true;
+ else if (strcmp(argv[i], "--help") == 0 || strcmp(argv[i],
"-?") == 0)
+ show_help = true;
+ else if (strcmp(argv[i], "--version") == 0 || strcmp(argv[i],
"-V") == 0)
+ version = true;
+ else if (strcmp(argv[i], "-C") == 0)
+ check_guc = true;
+ else
+ modes_set--;
+ }
+
+ if (modes_set > 1)
+ {
+ /* hack to avoid assertion failure in proc_exit() */
+ MyProcPid = getpid();
+
+ ereport(ERROR,
+ (errcode(ERRCODE_SYNTAX_ERROR),
+ errmsg("multiple server modes set"),
+ errdetail("Only one of %s may be set.",
+#ifdef EXEC_BACKEND
+ "--check, --boot,
--forkchild, --describe-config, --single, --help/-?, --version/-V, -C")));
+#else
+ "--check, --boot,
--describe-config, --single, --help/-?, --version/-V, -C")));
+#endif
+ }
+
/*
* Catch standard options before doing much else, in particular before
we
* insist on not being root.
*/
- if (argc > 1)
+ if (show_help)
{
- if (strcmp(argv[1], "--help") == 0 || strcmp(argv[1], "-?") ==
0)
- {
- help(progname);
- exit(0);
- }
- if (strcmp(argv[1], "--version") == 0 || strcmp(argv[1], "-V")
== 0)
- {
- fputs(PG_BACKEND_VERSIONSTR, stdout);
- exit(0);
- }
+ help(progname);
+ exit(0);
+ }
- /*
- * In addition to the above, we allow "--describe-config" and
"-C var"
- * to be called by root. This is reasonably safe since these
are
- * read-only activities. The -C case is important because
pg_ctl may
- * try to invoke it while still holding administrator
privileges on
- * Windows. Note that while -C can normally be in any argv
position,
- * if you want to bypass the root check you must put it first.
This
- * reduces the risk that we might misinterpret some other
mode's -C
- * switch as being the postmaster/postgres one.
- */
- if (strcmp(argv[1], "--describe-config") == 0)
- do_check_root = false;
- else if (argc > 2 && strcmp(argv[1], "-C") == 0)
- do_check_root = false;
+ if (version)
+ {
+ fputs(PG_BACKEND_VERSIONSTR, stdout);
+ exit(0);
}
/*
* Make sure we are not running as root, unless it's safe for the
selected
* option.
*/
- if (do_check_root)
+ if (!describe_config && !check_guc)
check_root(progname);
/*
- * Dispatch to one of various subprograms depending on first argument.
+ * Dispatch to one of various subprograms.
*/
- if (argc > 1 && strcmp(argv[1], "--check") == 0)
+ if (check)
BootstrapModeMain(argc, argv, true);
- else if (argc > 1 && strcmp(argv[1], "--boot") == 0)
+ else if (boot)
BootstrapModeMain(argc, argv, false);
#ifdef EXEC_BACKEND
- else if (argc > 1 && strncmp(argv[1], "--fork", 6) == 0)
+ else if (forkchild)
SubPostmasterMain(argc, argv);
#endif
- else if (argc > 1 && strcmp(argv[1], "--describe-config") == 0)
+ else if (describe_config)
GucInfoMain();
- else if (argc > 1 && strcmp(argv[1], "--single") == 0)
+ else if (single)
PostgresSingleUserMain(argc, argv,
strdup(get_user_name_or_exit(progname)));
else
diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index 45a3794b8e..74f96ce479 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -3796,20 +3796,9 @@ process_postgres_switches(int argc, char *argv[],
GucContext ctx,
int flag;
if (secure)
- {
gucsource = PGC_S_ARGV; /* switches came from command line */
-
- /* Ignore the initial --single argument, if present */
- if (argc > 1 && strcmp(argv[1], "--single") == 0)
- {
- argv++;
- argc--;
- }
- }
else
- {
gucsource = PGC_S_CLIENT; /* switches came from client */
- }
#ifdef HAVE_INT_OPTERR
@@ -3850,6 +3839,10 @@ process_postgres_switches(int argc, char *argv[],
GucContext ctx,
char *name,
*value;
+ /* --single was already processed in
main() */
+ if (strcmp(optarg, "single") == 0)
+ break;
+
ParseLongOption(optarg, &name, &value);
if (!value)
{
--
2.39.3 (Apple Git-146)
