Thread: 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 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] 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

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 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

Committed.

-- 
nathan