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

Bruce Momjian wrote:
> Alvaro Herrera wrote:
> >
> > Excerpts from Bruce Momjian's message of lun oct 03 17:28:53 -0300 2011:
> > >
> > > Alvaro Herrera wrote:
> >
> > > > Well, we have the Gentoo developer in this very thread.  I'm sure they
> > > > would fix their command line if we gave them a pg_ctl that worked.
> > > > Surely the package that contains the init script also contains pg_ctl,
> > > > so they would both be upgraded simultaneously.
> > >
> > > What is the fix?  If they started the server by using --data-directory,
> > > pg_ctl stop has no way to find the postmaster.pid file, and hence stop
> > > the server.  Are you suggesting we remove this ability?
> >
> > I am suggesting they don't start it by using --data-directory in the
> > first place.
>
> Agreed.  If you remove that, the logical problem goes away and it
> becomes a simple problem of dumping the contents of postgresql.conf and
> having pg_ctl (and pg_upgrade) use that.  Let me look at how much code
> that would take.

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

Patch attached.  This was much simpler than I thought.  :-)

--
  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..660458e
*** 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         dump_config_variable[MAXPGPATH] = "";
+
  /* 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':
+                 strlcpy(dump_config_variable, optarg, MAXPGPATH);
+                 break;
+
              case 'D':
                  userDoption = optarg;
                  break;
*************** PostmasterMain(int argc, char *argv[])
*** 728,733 ****
--- 734,746 ----
      if (!SelectConfigFiles(userDoption, progname))
          ExitPostmaster(2);

+     if (dump_config_variable[0] != '\0')
+     {
+         /* This allows anyone to read super-user config values. */
+         printf("%s\n", GetConfigOption(dump_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..18a02ad
*** 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,1942 ----
  }
  #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];
+     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 */
+
+     /* problem that we set this here and might cause it later not to change */
+     if (exec_path == NULL)
+         exec_path = find_other_exec_or_die(argv0, "postgres", PG_BACKEND_VERSIONSTR);
+
+     snprintf(cmd, MAXPGPATH, SYSTEMQUOTE "\"%s\" %s%s -C data_directory" SYSTEMQUOTE,
+              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, exec_path);
+         exit(1);
+     }
+     pclose(fd);
+
+     if (strlen(filename) > 0 && filename[strlen(filename) - 1] == '\n')
+         filename[strlen(filename) - 1] = '\0';
+     free(pg_data);
+     pg_data = xstrdup(filename);
+ }
+

  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"),
--- 2171,2187 ----
      }

      /* 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"),