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

On Wed, Jun 5, 2024 at 3:18 PM Nathan Bossart <nathandbossart@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.

Thanks,
Greg

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 <nathandbossart@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]/messages/by-id/20230609232257.GA121461@nathanxps13 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]: /messages/by-id/20230609232257.GA121461@nathanxps13

--
nathan

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

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

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.

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

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

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

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

On Mon, Aug 26, 2024 at 11:43 AM Nathan Bossart <nathandbossart@gmail.com>
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.

Here's what I have staged for commit. I didn't like how v4 added the ERROR
to ParseLongOption(), so in v5 I've moved it to the callers of
ParseLongOption(), which is where the existing option validation lives.
This results in a bit of code duplication, but IMHO that's better than
adding nonobvious behavior to ParseLongOption().

Barring additional feedback or cfbot failures, I'm planning on committing
this shortly.

--
nathan

On 2024-Dec-03, Nathan Bossart wrote:

+Subprogram
+parse_subprogram(const char *name)
+{

Please add a comment atop this function. Also, I don't think it should
go at the end of the file; maybe right after main() is a more
appropriate location?

+/* special must-be-first options for dispatching to various subprograms */
+typedef enum Subprogram
+{

I'm not sure this comment properly explains what this enum is used for.
Maybe add a reference to parse_subprogram to the comment?

Thanks

--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
Al principio era UNIX, y UNIX habló y dijo: "Hello world\n".
No dijo "Hello New Jersey\n", ni "Hello USA\n".

Nathan Bossart <nathandbossart@gmail.com> writes:

Here's what I have staged for commit.

In addition to Alvaro's comments:

+/* 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

Also, I think our usual convention for annotating a special
last entry is more like

+	SUBPROGRAM_SINGLE,
+	SUBPROGRAM_POSTMASTER,		/* must be last */
+} Subprogram;

I don't like the comment with "above" because it's not
very clear above what.

regards, tom lane

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

Committed.

--
nathan