On Tue, Jul 11, 2023 at 09:32:32AM -0700, Nathan Bossart wrote:
> Sure. І did it this way in v7.
After a couple more small edits, I've committed this. I looked through all
uses of getopt_long() in PostgreSQL earlier today, and of the programs that
accepted non-options, most accepted only one, some others accepted 2-3, and
ecpg and pg_regress accepted any number. Given this analysis, I'm not too
worried about the O(n^2) behavior that the patch introduces. You'd likely
need to provide an enormous number of non-options for it to be noticeable,
and I'm dubious such use-cases exist.
During my analysis, I noticed that pg_ctl contains a workaround for the
lack of argument reordering. I think this can now be removed. Patch
attached.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
>From 10931d7b83f3ef02f510385e1e595bc260b69a5c Mon Sep 17 00:00:00 2001
From: Nathan Bossart <nat...@postgresql.org>
Date: Wed, 12 Jul 2023 19:57:44 -0700
Subject: [PATCH v8 2/2] Rely on getopt_long() argument reordering in pg_ctl.
---
src/bin/pg_ctl/pg_ctl.c | 268 +++++++++++++++++++---------------------
1 file changed, 128 insertions(+), 140 deletions(-)
diff --git a/src/bin/pg_ctl/pg_ctl.c b/src/bin/pg_ctl/pg_ctl.c
index 465948e707..807d7023a9 100644
--- a/src/bin/pg_ctl/pg_ctl.c
+++ b/src/bin/pg_ctl/pg_ctl.c
@@ -2260,163 +2260,151 @@ main(int argc, char **argv)
if (env_wait != NULL)
wait_seconds = atoi(env_wait);
- /*
- * 'Action' can be before or after args so loop over both. Some
- * getopt_long() implementations will reorder argv[] to place all flags
- * first (GNU?), but we don't rely on it. Our /port version doesn't do
- * that.
- */
- optind = 1;
-
/* process command-line options */
- while (optind < argc)
+ while ((c = getopt_long(argc, argv, "cD:e:l:m:N:o:p:P:sS:t:U:wW",
+ long_options, &option_index)) != -1)
{
- while ((c = getopt_long(argc, argv, "cD:e:l:m:N:o:p:P:sS:t:U:wW",
- long_options, &option_index)) != -1)
+ switch (c)
{
- switch (c)
- {
- case 'D':
- {
- char *pgdata_D;
-
- pgdata_D = pg_strdup(optarg);
- canonicalize_path(pgdata_D);
- setenv("PGDATA", pgdata_D, 1);
-
- /*
- * We could pass PGDATA just in an environment
- * variable but we do -D too for clearer postmaster
- * 'ps' display
- */
- pgdata_opt = psprintf("-D \"%s\" ", pgdata_D);
- free(pgdata_D);
- break;
- }
- case 'e':
- event_source = pg_strdup(optarg);
- break;
- case 'l':
- log_file = pg_strdup(optarg);
- break;
- case 'm':
- set_mode(optarg);
- break;
- case 'N':
- register_servicename = pg_strdup(optarg);
- break;
- case 'o':
- /* append option? */
- if (!post_opts)
- post_opts = pg_strdup(optarg);
- else
- {
- char *old_post_opts = post_opts;
-
- post_opts = psprintf("%s %s", old_post_opts, optarg);
- free(old_post_opts);
- }
- break;
- case 'p':
- exec_path = pg_strdup(optarg);
- break;
- case 'P':
- register_password = pg_strdup(optarg);
- break;
- case 's':
- silent_mode = true;
+ case 'D':
+ {
+ char *pgdata_D;
+
+ pgdata_D = pg_strdup(optarg);
+ canonicalize_path(pgdata_D);
+ setenv("PGDATA", pgdata_D, 1);
+
+ /*
+ * We could pass PGDATA just in an environment variable
+ * but we do -D too for clearer postmaster 'ps' display
+ */
+ pgdata_opt = psprintf("-D \"%s\" ", pgdata_D);
+ free(pgdata_D);
break;
- case 'S':
+ }
+ case 'e':
+ event_source = pg_strdup(optarg);
+ break;
+ case 'l':
+ log_file = pg_strdup(optarg);
+ break;
+ case 'm':
+ set_mode(optarg);
+ break;
+ case 'N':
+ register_servicename = pg_strdup(optarg);
+ break;
+ case 'o':
+ /* append option? */
+ if (!post_opts)
+ post_opts = pg_strdup(optarg);
+ else
+ {
+ char *old_post_opts = post_opts;
+
+ post_opts = psprintf("%s %s", old_post_opts, optarg);
+ free(old_post_opts);
+ }
+ break;
+ case 'p':
+ exec_path = pg_strdup(optarg);
+ break;
+ case 'P':
+ register_password = pg_strdup(optarg);
+ break;
+ case 's':
+ silent_mode = true;
+ break;
+ case 'S':
#ifdef WIN32
- set_starttype(optarg);
+ set_starttype(optarg);
#else
- write_stderr(_("%s: -S option not supported on this platform\n"),
- progname);
- exit(1);
+ write_stderr(_("%s: -S option not supported on this platform\n"),
+ progname);
+ exit(1);
#endif
- break;
- case 't':
- wait_seconds = atoi(optarg);
- wait_seconds_arg = true;
- break;
- case 'U':
- if (strchr(optarg, '\\'))
- register_username = pg_strdup(optarg);
- else
- /* Prepend .\ for local accounts */
- register_username = psprintf(".\\%s", optarg);
- break;
- case 'w':
- do_wait = true;
- break;
- case 'W':
- do_wait = false;
- break;
- case 'c':
- allow_core_files = true;
- break;
- default:
- /* getopt_long already issued a suitable error message */
- do_advice();
- exit(1);
- }
+ break;
+ case 't':
+ wait_seconds = atoi(optarg);
+ wait_seconds_arg = true;
+ break;
+ case 'U':
+ if (strchr(optarg, '\\'))
+ register_username = pg_strdup(optarg);
+ else
+ /* Prepend .\ for local accounts */
+ register_username = psprintf(".\\%s", optarg);
+ break;
+ case 'w':
+ do_wait = true;
+ break;
+ case 'W':
+ do_wait = false;
+ break;
+ case 'c':
+ allow_core_files = true;
+ break;
+ default:
+ /* getopt_long already issued a suitable error message */
+ do_advice();
+ exit(1);
}
+ }
- /* Process an action */
- if (optind < argc)
+ /* Process an action */
+ if (optind < argc)
+ {
+ if (strcmp(argv[optind], "init") == 0
+ || strcmp(argv[optind], "initdb") == 0)
+ ctl_command = INIT_COMMAND;
+ else if (strcmp(argv[optind], "start") == 0)
+ ctl_command = START_COMMAND;
+ else if (strcmp(argv[optind], "stop") == 0)
+ ctl_command = STOP_COMMAND;
+ else if (strcmp(argv[optind], "restart") == 0)
+ ctl_command = RESTART_COMMAND;
+ else if (strcmp(argv[optind], "reload") == 0)
+ ctl_command = RELOAD_COMMAND;
+ else if (strcmp(argv[optind], "status") == 0)
+ ctl_command = STATUS_COMMAND;
+ else if (strcmp(argv[optind], "promote") == 0)
+ ctl_command = PROMOTE_COMMAND;
+ else if (strcmp(argv[optind], "logrotate") == 0)
+ ctl_command = LOGROTATE_COMMAND;
+ else if (strcmp(argv[optind], "kill") == 0)
{
- if (ctl_command != NO_COMMAND)
+ if (argc - optind < 3)
{
- write_stderr(_("%s: too many command-line arguments (first is \"%s\")\n"), progname, argv[optind]);
+ write_stderr(_("%s: missing arguments for kill mode\n"), progname);
do_advice();
exit(1);
}
-
- if (strcmp(argv[optind], "init") == 0
- || strcmp(argv[optind], "initdb") == 0)
- ctl_command = INIT_COMMAND;
- else if (strcmp(argv[optind], "start") == 0)
- ctl_command = START_COMMAND;
- else if (strcmp(argv[optind], "stop") == 0)
- ctl_command = STOP_COMMAND;
- else if (strcmp(argv[optind], "restart") == 0)
- ctl_command = RESTART_COMMAND;
- else if (strcmp(argv[optind], "reload") == 0)
- ctl_command = RELOAD_COMMAND;
- else if (strcmp(argv[optind], "status") == 0)
- ctl_command = STATUS_COMMAND;
- else if (strcmp(argv[optind], "promote") == 0)
- ctl_command = PROMOTE_COMMAND;
- else if (strcmp(argv[optind], "logrotate") == 0)
- ctl_command = LOGROTATE_COMMAND;
- else if (strcmp(argv[optind], "kill") == 0)
- {
- if (argc - optind < 3)
- {
- write_stderr(_("%s: missing arguments for kill mode\n"), progname);
- do_advice();
- exit(1);
- }
- ctl_command = KILL_COMMAND;
- set_sig(argv[++optind]);
- killproc = atol(argv[++optind]);
- }
+ ctl_command = KILL_COMMAND;
+ set_sig(argv[++optind]);
+ killproc = atol(argv[++optind]);
+ }
#ifdef WIN32
- else if (strcmp(argv[optind], "register") == 0)
- ctl_command = REGISTER_COMMAND;
- else if (strcmp(argv[optind], "unregister") == 0)
- ctl_command = UNREGISTER_COMMAND;
- else if (strcmp(argv[optind], "runservice") == 0)
- ctl_command = RUN_AS_SERVICE_COMMAND;
+ else if (strcmp(argv[optind], "register") == 0)
+ ctl_command = REGISTER_COMMAND;
+ else if (strcmp(argv[optind], "unregister") == 0)
+ ctl_command = UNREGISTER_COMMAND;
+ else if (strcmp(argv[optind], "runservice") == 0)
+ ctl_command = RUN_AS_SERVICE_COMMAND;
#endif
- else
- {
- write_stderr(_("%s: unrecognized operation mode \"%s\"\n"), progname, argv[optind]);
- do_advice();
- exit(1);
- }
- optind++;
+ else
+ {
+ write_stderr(_("%s: unrecognized operation mode \"%s\"\n"), progname, argv[optind]);
+ do_advice();
+ exit(1);
}
+ optind++;
+ }
+
+ if (optind < argc)
+ {
+ write_stderr(_("%s: too many command-line arguments (first is \"%s\")\n"), progname, argv[optind]);
+ do_advice();
+ exit(1);
}
if (ctl_command == NO_COMMAND)
--
2.25.1
