Re: Bug with pg_ctl -w/wait and config-only directories

Robert Haas wrote:
> On Mon, Oct 3, 2011 at 11:04 PM, Bruce Momjian <bruce@momjian.us> wrote:
> > OK, here is a patch that adds a -C option to the postmaster so any
> > config variable can be dumped, even while the server is running (there
> > is no security check because we don't have a user name at this point),
> > e.g.:
> >
> > ? ? ? ?postgres -D /pg_upgrade/tmp -C data_directory
> > ? ? ? ?/u/pg/data
>
> It seems both ugly and unnecessary to declare dump_config_variable as
> char[MAXPGPATH].  MAXPGPATH doesn't seem like the right length for a
> buffer intended to hold a GUC name, but in fact I don't think you need
> a buffer at all.  I think you can just declare it as char * and say
> dump_config_variable = optarg. getopt() doesn't overwrite the input
> argument vector, does it?

Well, as I remember, it writes a null byte at the end of the argument
and then passes the pointer to the start --- when it advances to the
next argument, it removes the null, so the pointer might still be valid,
but does not have proper termination (or maybe that's what strtok()
does).  However, I can find no documentation that mentions this
restriction, so perhaps it is old and no longer valid.

If you look in our code you will see tons of cases of:

    user = strdup(optarg);
    pg_data = xstrdup(optarg);
    my_opts->dbname = mystrdup(optarg);

However, I see other cases where we just assign optarg and optarg is a
string, e.g. pg_dump:

    username = optarg;

Doing a Google search shows both types of coding in random code pieces.

Are the optarg string duplication calls unnecessary and can be removed?
Either that, or we need to add some.

> Also, I think you should remove this comment:
>
> +         /* This allows anyone to read super-user config values. */
>
> It allows anyone to read super-user config values *who can already
> read postgresql.conf*.  Duh.

Oh, very good point --- I had not realized that.  Comment updated.

> > It also modifies pg_ctl to use this feature. ?It works fine for pg_ctl
> > -w start/stop with a config-only directory, so this is certainly in the
> > right direction. ?You can also use pg_ctl -o '--data_directory=/abc' and
> > it will be understood:
> >
> > ? ? ? ?pg_ctl -o '--data_directory=/u/pg/data' -D tmp start
> >
> > If you used --data_directory to start the server, you will need to use
> > --data_directory to stop it, which seems reasonable.
>
> Yep.  I think that when adjust_data_dir() changes pg_data it probably
> needs to call canonicalize_path() on the new value.

Done.

> > Patch attached. ?This was much simpler than I thought. ?:-)
>
> Yes, this looks pretty simple.

What really saved my bacon was the fact that the -o parameters are
passed into the backend and processed by the backend, rather than pg_ctl
having to read through that mess and parse it.  Doing that logic was
going to be very hard and unlikely to be back-patch-able.

Updated patch attached.

--
  Bruce Momjian  <bruce@momjian.us>        http://momjian.us
  EnterpriseDB                             http://enterprisedb.com

  + It's impossible for everything to be true. +
diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c
new file mode 100644
index 0a84d97..122c206
*** a/src/backend/postmaster/postmaster.c
--- b/src/backend/postmaster/postmaster.c
*************** bool        enable_bonjour = false;
*** 203,208 ****
--- 203,210 ----
  char       *bonjour_name;
  bool        restart_after_crash = true;

+ char         *output_config_variable = NULL;
+
  /* PIDs of special child processes; 0 when not running */
  static pid_t StartupPID = 0,
              BgWriterPID = 0,
*************** PostmasterMain(int argc, char *argv[])
*** 537,543 ****
       * tcop/postgres.c (the option sets should not conflict) and with the
       * common help() function in main/main.c.
       */
!     while ((opt = getopt(argc, argv, "A:B:bc:D:d:EeFf:h:ijk:lN:nOo:Pp:r:S:sTt:W:-:")) != -1)
      {
          switch (opt)
          {
--- 539,545 ----
       * tcop/postgres.c (the option sets should not conflict) and with the
       * common help() function in main/main.c.
       */
!     while ((opt = getopt(argc, argv, "A:B:bc:C:D:d:EeFf:h:ijk:lN:nOo:Pp:r:S:sTt:W:-:")) != -1)
      {
          switch (opt)
          {
*************** PostmasterMain(int argc, char *argv[])
*** 554,559 ****
--- 556,565 ----
                  IsBinaryUpgrade = true;
                  break;

+             case 'C':
+                 output_config_variable = strdup(optarg);
+                 break;
+
              case 'D':
                  userDoption = optarg;
                  break;
*************** PostmasterMain(int argc, char *argv[])
*** 728,733 ****
--- 734,746 ----
      if (!SelectConfigFiles(userDoption, progname))
          ExitPostmaster(2);

+     if (output_config_variable != NULL)
+     {
+         /* permission is handled because the user is reading inside the data dir */
+         puts(GetConfigOption(output_config_variable, false, false));
+         ExitPostmaster(0);
+     }
+
      /* Verify that DataDir looks reasonable */
      checkDataDir();

diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
new file mode 100644
index c7eac71..a5eae49
*** a/src/backend/tcop/postgres.c
--- b/src/backend/tcop/postgres.c
*************** process_postgres_switches(int argc, char
*** 3170,3176 ****
       * postmaster/postmaster.c (the option sets should not conflict) and with
       * the common help() function in main/main.c.
       */
!     while ((flag = getopt(argc, argv, "A:B:bc:D:d:EeFf:h:ijk:lN:nOo:Pp:r:S:sTt:v:W:-:")) != -1)
      {
          switch (flag)
          {
--- 3170,3176 ----
       * postmaster/postmaster.c (the option sets should not conflict) and with
       * the common help() function in main/main.c.
       */
!     while ((flag = getopt(argc, argv, "A:B:bc:C:D:d:EeFf:h:ijk:lN:nOo:Pp:r:S:sTt:v:W:-:")) != -1)
      {
          switch (flag)
          {
*************** process_postgres_switches(int argc, char
*** 3187,3192 ****
--- 3187,3196 ----
                  IsBinaryUpgrade = true;
                  break;

+             case 'C':
+                 /* ignored for consistency with postmaster */
+                 break;
+
              case 'D':
                  if (secure)
                      userDoption = strdup(optarg);
diff --git a/src/bin/pg_ctl/pg_ctl.c b/src/bin/pg_ctl/pg_ctl.c
new file mode 100644
index 0dbdfe7..e633d0c
*** a/src/bin/pg_ctl/pg_ctl.c
--- b/src/bin/pg_ctl/pg_ctl.c
*************** static ShutdownMode shutdown_mode = SMAR
*** 81,86 ****
--- 81,87 ----
  static int    sig = SIGTERM;        /* default */
  static CtlCommand ctl_command = NO_COMMAND;
  static char *pg_data = NULL;
+ static char *pg_config = NULL;
  static char *pgdata_opt = NULL;
  static char *post_opts = NULL;
  static const char *progname;
*************** static void do_status(void);
*** 131,136 ****
--- 132,138 ----
  static void do_promote(void);
  static void do_kill(pgpid_t pid);
  static void print_msg(const char *msg);
+ static void adjust_data_dir(void);

  #if defined(WIN32) || defined(__CYGWIN__)
  static bool pgwin32_IsInstalled(SC_HANDLE);
*************** pgwin32_CommandLine(bool registration)
*** 1265,1274 ****
          strcat(cmdLine, "\"");
      }

!     if (pg_data)
      {
          strcat(cmdLine, " -D \"");
!         strcat(cmdLine, pg_data);
          strcat(cmdLine, "\"");
      }

--- 1267,1276 ----
          strcat(cmdLine, "\"");
      }

!     if (pg_config)
      {
          strcat(cmdLine, " -D \"");
!         strcat(cmdLine, pg_config);
          strcat(cmdLine, "\"");
      }

*************** set_starttype(char *starttypeopt)
*** 1886,1891 ****
--- 1888,1946 ----
  }
  #endif

+ /*
+  * adjust_data_dir
+  *
+  * If a configuration-only directory was specified, find the real data dir.
+  */
+ void
+ adjust_data_dir(void)
+ {
+     char        cmd[MAXPGPATH], filename[MAXPGPATH], *my_exec_path;
+     FILE       *fd;
+
+     /* If there is no postgresql.conf, it can't be a config-only dir */
+     snprintf(filename, sizeof(filename), "%s/postgresql.conf", pg_config);
+     if ((fd = fopen(filename, "r")) == NULL)
+         return;
+     fclose(fd);
+
+     /* If PG_VERSION exists, it can't be a config-only dir */
+     snprintf(filename, sizeof(filename), "%s/PG_VERSION", pg_config);
+     if ((fd = fopen(filename, "r")) != NULL)
+     {
+         fclose(fd);
+         return;
+     }
+
+     /* Must be a configuration directory, so find the data directory */
+
+     /* we use a private my_exec_path to avoid interfering with later uses */
+     if (exec_path == NULL)
+         my_exec_path = find_other_exec_or_die(argv0, "postgres", PG_BACKEND_VERSIONSTR);
+     else
+         my_exec_path = xstrdup(exec_path);
+
+     snprintf(cmd, MAXPGPATH, SYSTEMQUOTE "\"%s\" %s%s -C data_directory" SYSTEMQUOTE,
+              my_exec_path, pgdata_opt ? pgdata_opt : "", post_opts ?
+              post_opts : "");
+
+     fd = popen(cmd, "r");
+     if (fd == NULL || fgets(filename, sizeof(filename), fd) == NULL)
+     {
+         write_stderr(_("%s: cannot find the data directory using %s\n"), progname, my_exec_path);
+         exit(1);
+     }
+     pclose(fd);
+     free(my_exec_path);
+
+     if (strlen(filename) > 0 && filename[strlen(filename) - 1] == '\n')
+         filename[strlen(filename) - 1] = '\0';
+     free(pg_data);
+     pg_data = xstrdup(filename);
+     canonicalize_path(pg_data);
+ }
+

  int
  main(int argc, char **argv)
*************** main(int argc, char **argv)
*** 2120,2133 ****
      }

      /* Note we put any -D switch into the env var above */
!     pg_data = getenv("PGDATA");
!     if (pg_data)
      {
!         pg_data = xstrdup(pg_data);
!         canonicalize_path(pg_data);
      }

!     if (pg_data == NULL &&
          ctl_command != KILL_COMMAND && ctl_command != UNREGISTER_COMMAND)
      {
          write_stderr(_("%s: no database directory specified and environment variable PGDATA unset\n"),
--- 2175,2191 ----
      }

      /* Note we put any -D switch into the env var above */
!     pg_config = getenv("PGDATA");
!     if (pg_config)
      {
!         pg_config = xstrdup(pg_config);
!         canonicalize_path(pg_config);
!         pg_data = xstrdup(pg_config);
      }

!     adjust_data_dir();
!
!     if (pg_config == NULL &&
          ctl_command != KILL_COMMAND && ctl_command != UNREGISTER_COMMAND)
      {
          write_stderr(_("%s: no database directory specified and environment variable PGDATA unset\n"),