Re: Better error message when --single is not the first arg to postgres executable
On Mon, Aug 26, 2024 at 11:43 AM Nathan Bossart wrote: > On Sun, Aug 25, 2024 at 01:14:36PM -0400, Greg Sabino Mullane wrote: > > I'm not happy about making this and the const char[] change their > structure > > based on the ifdefs - could we not just leave forkchild in? Their usage > is > > already protected by the ifdefs in the calling code. > > Here's an attempt at this. > Looks great, thank you.
Re: Better error message when --single is not the first arg to postgres executable
On Sun, Aug 25, 2024 at 01:14:36PM -0400, Greg Sabino Mullane wrote:
> I'm not happy about making this and the const char[] change their structure
> based on the ifdefs - could we not just leave forkchild in? Their usage is
> already protected by the ifdefs in the calling code.
Here's an attempt at this.
--
nathan
>From 112026b083615be8debed02cb2c68797301b7319 Mon Sep 17 00:00:00 2001
From: Nathan Bossart
Date: Wed, 21 Aug 2024 15:39:05 -0500
Subject: [PATCH v4 1/1] better error message when subprogram argument is not
first
---
src/backend/main/main.c | 84 -
src/backend/utils/misc/guc.c| 6 +++
src/include/postmaster/postmaster.h | 16 ++
src/tools/pgindent/typedefs.list| 1 +
4 files changed, 94 insertions(+), 13 deletions(-)
diff --git a/src/backend/main/main.c b/src/backend/main/main.c
index 4672aab837..24ba012acf 100644
--- a/src/backend/main/main.c
+++ b/src/backend/main/main.c
@@ -44,6 +44,18 @@
const char *progname;
static bool reached_main = false;
+static const char *const SubprogramNames[] =
+{
+ [SUBPROGRAM_CHECK] = "check",
+ [SUBPROGRAM_BOOT] = "boot",
+ [SUBPROGRAM_FORKCHILD] = "forkchild",
+ [SUBPROGRAM_DESCRIBE_CONFIG] = "describe-config",
+ [SUBPROGRAM_SINGLE] = "single",
+ /* SUBPROGRAM_POSTMASTER has no name */
+};
+
+StaticAssertDecl(lengthof(SubprogramNames) == SUBPROGRAM_POSTMASTER,
+"array length mismatch");
static void startup_hacks(const char *progname);
static void init_locale(const char *categoryname, int category, const char
*locale);
@@ -58,6 +70,7 @@ int
main(int argc, char *argv[])
{
booldo_check_root = true;
+ Subprogram subprogram = SUBPROGRAM_POSTMASTER;
reached_main = true;
@@ -180,21 +193,36 @@ 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] == '-')
+ subprogram = parse_subprogram(&argv[1][2]);
+
+ switch (subprogram)
+ {
+ case SUBPROGRAM_CHECK:
+ BootstrapModeMain(argc, argv, true);
+ break;
+ case SUBPROGRAM_BOOT:
+ BootstrapModeMain(argc, argv, false);
+ break;
+ case SUBPROGRAM_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 SUBPROGRAM_DESCRIBE_CONFIG:
+ GucInfoMain();
+ break;
+ case SUBPROGRAM_SINGLE:
+ PostgresSingleUserMain(argc, argv,
+
strdup(get_user_name_or_exit(progname)));
+ break;
+ case SUBPROGRAM_POSTMASTER:
+ PostmasterMain(argc, argv);
+ break;
+ }
+
/* the functions above should not return */
abort();
}
@@ -441,3 +469,33 @@ __ubsan_default_options(void)
return getenv("UBSAN_OPTIONS");
}
+
+Subprogram
+parse_subprogram(const char *name)
+{
+ for (int i = 0; i < lengthof(SubprogramNames); i++)
+ {
+ /*
+* Unlike the other subprogram 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
+* SUBPROGRAM_FORKCHILD, so skip over it in that case.
+*/
+ if (i == SUBPROGRAM_FORKCHILD)
+ {
+#ifdef EXEC_BACKEND
+ if (strncmp(SubprogramNames[SUBPROGRAM_FORKCHILD], name,
+
strlen(SubprogramNames[SUBPROGRAM_FORKCHILD])) == 0)
+ return SUBPROGRAM_FORKCHILD;
+#endif
+ continue;
+ }
+
+ if (strcmp(SubprogramNames[i], name) == 0)
+ return (Subprogram) i;
+
Re: Better error message when --single is not the first arg to postgres executable
I'm not opposed to this new method, as long as the error code improves. :)
+typedef enum Subprogram
+{
+ SUBPROGRAM_CHECK,
+ SUBPROGRAM_BOOT,
+#ifdef EXEC_BACKEND
+ SUBPROGRAM_FORKCHILD,
+#endif
I'm not happy about making this and the const char[] change their structure
based on the ifdefs - could we not just leave forkchild in? Their usage is
already protected by the ifdefs in the calling code.
Heck, we could put SUBPROGRAM_FORKCHILD first in the list, keep the ifdef
in parse_subprogram, and start regular checking with i = 1;
This would reduce to a single #ifdef
Cheers,
Greg
Re: Better error message when --single is not the first arg to postgres executable
On Wed, Jun 19, 2024 at 10:58:02AM -0500, Nathan Bossart wrote:
> On Wed, Jun 19, 2024 at 05:34:52PM +0200, Peter Eisentraut wrote:
>> I'm not really sure all this here is worth solving. I think requiring
>> things like --single or --boot to be first seems ok, and the alternatives
>> just make things more complicated.
>
> Yeah, I'm fine with doing something more like what Greg originally
> proposed at this point.
Here's an attempt at centralizing the set of subprogram options (and also
adding better error messages). My intent was to make it difficult to miss
updating all the relevant places when adding a new subprogram, but I'll
admit the patch is a bit more complicated than I was hoping.
--
nathan
>From 5b2a98ad0571e93d789bec4bb9343f1307fc3f53 Mon Sep 17 00:00:00 2001
From: Nathan Bossart
Date: Wed, 21 Aug 2024 15:39:05 -0500
Subject: [PATCH v3 1/1] better error message when subprogram argument is not
first
---
src/backend/main/main.c | 77 -
src/backend/utils/misc/guc.c| 6 +++
src/include/postmaster/postmaster.h | 18 +++
src/tools/pgindent/typedefs.list| 1 +
4 files changed, 89 insertions(+), 13 deletions(-)
diff --git a/src/backend/main/main.c b/src/backend/main/main.c
index 4672aab837..ab1d823820 100644
--- a/src/backend/main/main.c
+++ b/src/backend/main/main.c
@@ -44,6 +44,20 @@
const char *progname;
static bool reached_main = false;
+static const char *const SubprogramNames[] =
+{
+ [SUBPROGRAM_CHECK] = "check",
+ [SUBPROGRAM_BOOT] = "boot",
+#ifdef EXEC_BACKEND
+ [SUBPROGRAM_FORKCHILD] = "forkchild",
+#endif
+ [SUBPROGRAM_DESCRIBE_CONFIG] = "describe-config",
+ [SUBPROGRAM_SINGLE] = "single",
+ /* SUBPROGRAM_POSTMASTER has no name */
+};
+
+StaticAssertDecl(lengthof(SubprogramNames) == SUBPROGRAM_POSTMASTER,
+"array length mismatch");
static void startup_hacks(const char *progname);
static void init_locale(const char *categoryname, int category, const char
*locale);
@@ -58,6 +72,7 @@ int
main(int argc, char *argv[])
{
booldo_check_root = true;
+ Subprogram subprogram = SUBPROGRAM_POSTMASTER;
reached_main = true;
@@ -180,21 +195,34 @@ 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] == '-')
+ subprogram = parse_subprogram(&argv[1][2]);
+
+ switch (subprogram)
+ {
+ case SUBPROGRAM_CHECK:
+ BootstrapModeMain(argc, argv, true);
+ break;
+ case SUBPROGRAM_BOOT:
+ BootstrapModeMain(argc, argv, false);
+ break;
#ifdef EXEC_BACKEND
- else if (argc > 1 && strncmp(argv[1], "--forkchild", 11) == 0)
- SubPostmasterMain(argc, argv);
+ case SUBPROGRAM_FORKCHILD:
+ SubPostmasterMain(argc, argv);
+ break;
#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);
+ case SUBPROGRAM_DESCRIBE_CONFIG:
+ GucInfoMain();
+ break;
+ case SUBPROGRAM_SINGLE:
+ PostgresSingleUserMain(argc, argv,
+
strdup(get_user_name_or_exit(progname)));
+ break;
+ case SUBPROGRAM_POSTMASTER:
+ PostmasterMain(argc, argv);
+ break;
+ }
+
/* the functions above should not return */
abort();
}
@@ -441,3 +469,26 @@ __ubsan_default_options(void)
return getenv("UBSAN_OPTIONS");
}
+
+Subprogram
+parse_subprogram(const char *name)
+{
+#ifdef EXEC_BACKEND
+ /*
+* Unlike the other subprogram options, "forkchild" takes an argument,
so
+* we just look for the prefix for that one.
+*/
+ if (strncmp(SubprogramNames[SUBPROGRAM_FORKCHILD], name,
+ strlen(SubprogramNames[SUBPROGRAM_FORKCHILD]))
== 0)
+ return SUBPROGRAM_FORKCHILD;
+#endif
+
+ for (int i = 0; i < lengthof(SubprogramNames); i++)
+ {
+ if (strcmp(SubprogramNames[i], name) == 0)
+ return (Subprogram) i;
+ }
+
+ /
Re: Better error message when --single is not the first arg to postgres executable
On Wed, Jun 19, 2024 at 05:34:52PM +0200, Peter Eisentraut wrote: > I'm not really sure all this here is worth solving. I think requiring > things like --single or --boot to be first seems ok, and the alternatives > just make things more complicated. Yeah, I'm fine with doing something more like what Greg originally proposed at this point. -- nathan
Re: Better error message when --single is not the first arg to postgres executable
On 19.06.24 16:04, Nathan Bossart wrote:
What about just inlining --version and --help e.g.
else if (strcmp(argv[i], "--version") == 0 || strcmp(argv[i], "-V") == 0)
{
fputs(PG_BACKEND_VERSIONSTR, stdout);
exit(0);
}
I'm fine with being more persnickety about the other options; they are much
rarer and not unixy.
That seems like it should work. I'm not sure I agree that's the least
surprising behavior (e.g., what exactly is the user trying to tell us with
commands like "postgres --version --help --describe-config"?), but I also
don't feel too strongly about it.
There is sort of an existing convention that --help and --version behave
like this, meaning they act immediately and exit without considering
other arguments.
I'm not really sure all this here is worth solving. I think requiring
things like --single or --boot to be first seems ok, and the
alternatives just make things more complicated.
Re: Better error message when --single is not the first arg to postgres executable
On Tue, Jun 18, 2024 at 09:42:32PM -0400, Greg Sabino Mullane wrote:
> If I am reading your patch correctly, we have lost the behavior of least
> surprise in which the first "meta" argument overrides all others:
>
> $ bin/postgres --version --boot --extrastuff
> postgres (PostgreSQL) 16.2
Right, with the patch we fail if there are multiple such options specified:
$ postgres --version --help
FATAL: multiple server modes set
DETAIL: Only one of --check, --boot, --describe-config, --single,
--help/-?, --version/-V, -C may be set.
> What about just inlining --version and --help e.g.
>
> else if (strcmp(argv[i], "--version") == 0 || strcmp(argv[i], "-V") == 0)
> {
> fputs(PG_BACKEND_VERSIONSTR, stdout);
> exit(0);
> }
>
> I'm fine with being more persnickety about the other options; they are much
> rarer and not unixy.
That seems like it should work. I'm not sure I agree that's the least
surprising behavior (e.g., what exactly is the user trying to tell us with
commands like "postgres --version --help --describe-config"?), but I also
don't feel too strongly about it.
--
nathan
Re: Better error message when --single is not the first arg to postgres executable
If I am reading your patch correctly, we have lost the behavior of least
surprise in which the first "meta" argument overrides all others:
$ bin/postgres --version --boot --extrastuff
postgres (PostgreSQL) 16.2
What about just inlining --version and --help e.g.
else if (strcmp(argv[i], "--version") == 0 || strcmp(argv[i], "-V") == 0)
{
fputs(PG_BACKEND_VERSIONSTR, stdout);
exit(0);
}
I'm fine with being more persnickety about the other options; they are much
rarer and not unixy.
However, there's a complication:
> ...
> 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.
>
Yes, that's unfortunate. But I'd be okay with the db-last requirement as
long as the error message is sane and points one in the right direction.
Cheers,
Greg
Re: Better error message when --single is not the first arg to postgres executable
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
> 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
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[])
{
- booldo_check_root = true;
+ boolcheck = false;
+ boolboot = false;
+#ifdef EXEC_BACKEND
+ boolforkchild = false;
+#endif
+ booldescribe_config = false;
+ boolsingle = false;
+ boolshow_help = false;
+ boolversion = false;
+ boolcheck_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], "--
Re: Better error message when --single is not the first arg to postgres executable
On Wed, Jun 5, 2024 at 3:18 PM Nathan Bossart 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. Thanks, Greg
Re: Better error message when --single is not the first arg to postgres executable
On Wed, Jun 05, 2024 at 02:51:05PM -0400, Greg Sabino Mullane wrote: > Please find attached a quick patch to prevent this particularly bad error > message for running "postgres", when making the common mistake of > forgetting to put the "--single" option first because you added an earlier > arg (esp. datadir) 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... -- nathan
Better error message when --single is not the first arg to postgres executable
Please find attached a quick patch to prevent this particularly bad error message for running "postgres", when making the common mistake of forgetting to put the "--single" option first because you added an earlier arg (esp. datadir) Current behavior: $ ~/pg/bin/postgres -D ~/pg/data --single 2024-06-05 18:30:40.296 GMT [22934] FATAL: --single requires a value Improved behavior: $ ~/pg/bin/postgres -D ~/pg/data --single --single must be first argument. I applied it for all the "first arg only" flags (boot, check, describe-config, and fork), as they suffer the same fate. Cheers, Greg 0001-Give-a-more-accurate-error-message-if-single-not-number-one.patch Description: Binary data
