Thanks to Álvaro and Tom for reviewing.
On Tue, Dec 03, 2024 at 01:01:29PM -0500, Tom Lane wrote:
> +/* special must-be-first options for dispatching to various subprograms */
> +typedef enum Subprogram
> +{
> + SUBPROGRAM_CHECK,
> + ... etc
>
> "Subprogram" doesn't quite seem like the right name for this enum.
> These are not subprograms, they are options. I'm not feeling
> especially inventive today, so this might be a lousy suggestion,
> but how about
>
> typedef enum DispatchOption
> {
> DISPATCH_CHECK,
> ... etc
WFM. An alternative might be SubprogramOption, but I don't have any strong
opinions on the matter. I've switched it to DispatchOption in the attached
patch.
--
nathan
>From bb5219251479ef8f54df299269ae738ba19ce681 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <nat...@postgresql.org>
Date: Tue, 3 Dec 2024 13:50:36 -0600
Subject: [PATCH v6 1/1] Provide a better error message for misplaced dispatch
options.
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Before this patch, specifying a special must-be-first option for
dispatching to a subprogram (e.g., postgres -D . --single) would
fail with an error like
FATAL: --single requires a value
This patch adjusts this error to more accurately complain that the
special option wasn't listed first. The previous error message now
looks like
FATAL: --single must be first argument
The dispatch option parsing code has been refactored for use
wherever ParseLongOption() is called. Beyond the obvious advantage
of avoiding code duplication, this should prevent similar problems
when new dispatch options are added. Note that we assume that none
of the dispatch option names match another valid command-line
argument, such as the name of a configuration parameter.
Ideally, we'd remove this must-be-first requirement for these
options, but after some investigation, we felt that the added
complexity and behavior changes weren't worth it.
Author: Nathan Bossart, Greg Sabino Mullane
Reviewed-by: Greg Sabino Mullane, Peter Eisentraut, Álvaro Herrera, Tom Lane
Discussion:
https://postgr.es/m/CAKAnmmJkZtZAiSryho%3DgYpbvC7H-HNjEDAh16F3SoC9LPu8rqQ%40mail.gmail.com
---
src/backend/bootstrap/bootstrap.c | 15 ++++-
src/backend/main/main.c | 86 ++++++++++++++++++++++++-----
src/backend/postmaster/postmaster.c | 15 ++++-
src/backend/tcop/postgres.c | 15 ++++-
src/include/postmaster/postmaster.h | 17 ++++++
src/tools/pgindent/typedefs.list | 1 +
6 files changed, 133 insertions(+), 16 deletions(-)
diff --git a/src/backend/bootstrap/bootstrap.c
b/src/backend/bootstrap/bootstrap.c
index d31a67599c..a5217773ff 100644
--- a/src/backend/bootstrap/bootstrap.c
+++ b/src/backend/bootstrap/bootstrap.c
@@ -224,8 +224,21 @@ BootstrapModeMain(int argc, char *argv[], bool check_only)
case 'B':
SetConfigOption("shared_buffers", optarg,
PGC_POSTMASTER, PGC_S_ARGV);
break;
- case 'c':
case '-':
+
+ /*
+ * Error if the user misplaced a special
must-be-first option
+ * for dispatching to a subprogram.
parse_dispatch_option()
+ * returns DISPATCH_POSTMASTER if it doesn't
find a match, so
+ * error for anything else.
+ */
+ if (parse_dispatch_option(optarg) !=
DISPATCH_POSTMASTER)
+ ereport(ERROR,
+
(errcode(ERRCODE_SYNTAX_ERROR),
+ errmsg("--%s must be
first argument", optarg)));
+
+ /* FALLTHROUGH */
+ case 'c':
{
char *name,
*value;
diff --git a/src/backend/main/main.c b/src/backend/main/main.c
index aea93a0229..3acb46bd46 100644
--- a/src/backend/main/main.c
+++ b/src/backend/main/main.c
@@ -43,6 +43,19 @@
const char *progname;
static bool reached_main = false;
+/* names of special must-be-first options for dispatching to subprograms */
+static const char *const DispatchOptionNames[] =
+{
+ [DISPATCH_CHECK] = "check",
+ [DISPATCH_BOOT] = "boot",
+ [DISPATCH_FORKCHILD] = "forkchild",
+ [DISPATCH_DESCRIBE_CONFIG] = "describe-config",
+ [DISPATCH_SINGLE] = "single",
+ /* DISPATCH_POSTMASTER has no name */
+};
+
+StaticAssertDecl(lengthof(DispatchOptionNames) == DISPATCH_POSTMASTER,
+ "array length mismatch");
static void startup_hacks(const char *progname);
static void init_locale(const char *categoryname, int category, const char
*locale);
@@ -57,6 +70,7 @@ int
main(int argc, char *argv[])
{
bool do_check_root = true;
+ DispatchOption dispatch_option = DISPATCH_POSTMASTER;
reached_main = true;
@@ -179,26 +193,72 @@ main(int argc, char *argv[])
* Dispatch to one of various subprograms depending on first argument.
*/
- if (argc > 1 && strcmp(argv[1], "--check") == 0)
- BootstrapModeMain(argc, argv, true);
- else if (argc > 1 && strcmp(argv[1], "--boot") == 0)
- BootstrapModeMain(argc, argv, false);
+ if (argc > 1 && argv[1][0] == '-' && argv[1][1] == '-')
+ dispatch_option = parse_dispatch_option(&argv[1][2]);
+
+ switch (dispatch_option)
+ {
+ case DISPATCH_CHECK:
+ BootstrapModeMain(argc, argv, true);
+ break;
+ case DISPATCH_BOOT:
+ BootstrapModeMain(argc, argv, false);
+ break;
+ case DISPATCH_FORKCHILD:
#ifdef EXEC_BACKEND
- else if (argc > 1 && strncmp(argv[1], "--forkchild", 11) == 0)
- SubPostmasterMain(argc, argv);
+ SubPostmasterMain(argc, argv);
+#else
+ Assert(false); /* should never happen */
#endif
- else if (argc > 1 && strcmp(argv[1], "--describe-config") == 0)
- GucInfoMain();
- else if (argc > 1 && strcmp(argv[1], "--single") == 0)
- PostgresSingleUserMain(argc, argv,
-
strdup(get_user_name_or_exit(progname)));
- else
- PostmasterMain(argc, argv);
+ break;
+ case DISPATCH_DESCRIBE_CONFIG:
+ GucInfoMain();
+ break;
+ case DISPATCH_SINGLE:
+ PostgresSingleUserMain(argc, argv,
+
strdup(get_user_name_or_exit(progname)));
+ break;
+ case DISPATCH_POSTMASTER:
+ PostmasterMain(argc, argv);
+ break;
+ }
+
/* the functions above should not return */
abort();
}
+/*
+ * Returns the matching DispatchOption value for the given option name. If no
+ * match is found, DISPATCH_POSTMASTER is returned.
+ */
+DispatchOption
+parse_dispatch_option(const char *name)
+{
+ for (int i = 0; i < lengthof(DispatchOptionNames); i++)
+ {
+ /*
+ * Unlike the other dispatch options, "forkchild" takes an
argument,
+ * so we just look for the prefix for that one. For
non-EXEC_BACKEND
+ * builds, we never want to return DISPATCH_FORKCHILD, so skip
over it
+ * in that case.
+ */
+ if (i == DISPATCH_FORKCHILD)
+ {
+#ifdef EXEC_BACKEND
+ if (strncmp(DispatchOptionNames[DISPATCH_FORKCHILD],
name,
+
strlen(DispatchOptionNames[DISPATCH_FORKCHILD])) == 0)
+ return DISPATCH_FORKCHILD;
+#endif
+ continue;
+ }
+
+ if (strcmp(DispatchOptionNames[i], name) == 0)
+ return (DispatchOption) i;
+ }
+ /* no match means this is a postmaster */
+ return DISPATCH_POSTMASTER;
+}
/*
* Place platform-specific startup hacks here. This is the right
diff --git a/src/backend/postmaster/postmaster.c
b/src/backend/postmaster/postmaster.c
index 6376d43087..ce00f4032e 100644
--- a/src/backend/postmaster/postmaster.c
+++ b/src/backend/postmaster/postmaster.c
@@ -589,8 +589,21 @@ PostmasterMain(int argc, char *argv[])
output_config_variable = strdup(optarg);
break;
- case 'c':
case '-':
+
+ /*
+ * Error if the user misplaced a special
must-be-first option
+ * for dispatching to a subprogram.
parse_dispatch_option()
+ * returns DISPATCH_POSTMASTER if it doesn't
find a match, so
+ * error for anything else.
+ */
+ if (parse_dispatch_option(optarg) !=
DISPATCH_POSTMASTER)
+ ereport(ERROR,
+
(errcode(ERRCODE_SYNTAX_ERROR),
+ errmsg("--%s must be
first argument", optarg)));
+
+ /* FALLTHROUGH */
+ case 'c':
{
char *name,
*value;
diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index 4b985bd056..42af768045 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -3947,8 +3947,21 @@ process_postgres_switches(int argc, char *argv[],
GucContext ctx,
/* ignored for consistency with the postmaster
*/
break;
- case 'c':
case '-':
+
+ /*
+ * Error if the user misplaced a special
must-be-first option
+ * for dispatching to a subprogram.
parse_dispatch_option()
+ * returns DISPATCH_POSTMASTER if it doesn't
find a match, so
+ * error for anything else.
+ */
+ if (parse_dispatch_option(optarg) !=
DISPATCH_POSTMASTER)
+ ereport(ERROR,
+
(errcode(ERRCODE_SYNTAX_ERROR),
+ errmsg("--%s must be
first argument", optarg)));
+
+ /* FALLTHROUGH */
+ case 'c':
{
char *name,
*value;
diff --git a/src/include/postmaster/postmaster.h
b/src/include/postmaster/postmaster.h
index f05eb1c470..24d49a5439 100644
--- a/src/include/postmaster/postmaster.h
+++ b/src/include/postmaster/postmaster.h
@@ -138,4 +138,21 @@ extern PMChild *FindPostmasterChildByPid(int pid);
*/
#define MAX_BACKENDS 0x3FFFF
+/*
+ * These values correspond to the special must-be-first options for dispatching
+ * to various subprograms. parse_dispatch_option() can be used to convert an
+ * option name to one of these values.
+ */
+typedef enum DispatchOption
+{
+ DISPATCH_CHECK,
+ DISPATCH_BOOT,
+ DISPATCH_FORKCHILD,
+ DISPATCH_DESCRIBE_CONFIG,
+ DISPATCH_SINGLE,
+ DISPATCH_POSTMASTER, /* must be last */
+} DispatchOption;
+
+extern DispatchOption parse_dispatch_option(const char *name);
+
#endif /* _POSTMASTER_H */
diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list
index 2d4c870423..ce33e55bf1 100644
--- a/src/tools/pgindent/typedefs.list
+++ b/src/tools/pgindent/typedefs.list
@@ -619,6 +619,7 @@ DirectoryMethodFile
DisableTimeoutParams
DiscardMode
DiscardStmt
+DispatchOption
DistanceValue
DistinctExpr
DoState
--
2.39.5 (Apple Git-154)
