Re: pg_ctl and port number detection

Tom Lane wrote:
> Actually, if we're going to do this at all, we should do
>
>     pid
>     datadir
>     port
>     socketdir
>     ... here be dragons ...
>
> so that pg_ctl doesn't have to assume the server is running with a
> default value of unix_socket_dir.  Not sure what to put in the fourth
> line on Windows though ... maybe just leave it empty?

OK, here is a patch that adds the port number and optionally socket
directory location to postmaster.pid, and modifies pg_ctl to use that
information.  I throw an error on using Win32 with pre-9.1 servers
because we can't get the port number from that file.

This removes some crufty code from pg_ctl and removes dependency on
serveral user-configurable settings that we added as a work-around.

This will allow pg_ctl -w to work more reliabily than it did in the
past.

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

  + It's impossible for everything to be true. +
diff --git a/doc/src/sgml/ref/pg_ctl-ref.sgml b/doc/src/sgml/ref/pg_ctl-ref.sgml
index 2c01e12..c526e8e 100644
*** /tmp/pgrevert.8079/5dqQCd_pg_ctl-ref.sgml    Wed Dec 22 10:10:23 2010
--- doc/src/sgml/ref/pg_ctl-ref.sgml    Wed Dec 22 10:06:33 2010
*************** PostgreSQL documentation
*** 348,368 ****
         <para>
          Wait for the startup or shutdown to complete.
          Waiting is the default option for shutdowns, but not startups.
          When waiting for shutdown, <command>pg_ctl</command> waits for
          the server to remove its <acronym>PID</acronym> file.
!         When waiting for startup, <command>pg_ctl</command> repeatedly
!         attempts to connect to the server via <application>psql</>, and
!         reports success when this is successful.
!         <command>pg_ctl</command> will attempt to use the proper port for
!         <application>psql</>. If the environment variable
!         <envar>PGPORT</envar> exists, that is used.  Otherwise,
!         <command>pg_ctl</command> will see if a port has been set in the
!         <filename>postgresql.conf</filename> file.  If not, it will use the
!         default port that <productname>PostgreSQL</productname> was compiled
!         with (5432 by default).
!         When waiting, <command>pg_ctl</command> will
!         return an exit code based on the success of the startup
!         or shutdown.
         </para>
        </listitem>
       </varlistentry>
--- 348,359 ----
         <para>
          Wait for the startup or shutdown to complete.
          Waiting is the default option for shutdowns, but not startups.
+         When waiting for startup, <command>pg_ctl</command> repeatedly
+         attempts to connect to the server.
          When waiting for shutdown, <command>pg_ctl</command> waits for
          the server to remove its <acronym>PID</acronym> file.
!         <command>pg_ctl</command> returns an exit code based on the
!         success of the startup or shutdown.
         </para>
        </listitem>
       </varlistentry>
*************** PostgreSQL documentation
*** 442,469 ****
      </listitem>
     </varlistentry>

-    <varlistentry>
-     <term><envar>PGHOST</envar></term>
-
-     <listitem>
-      <para>
-       Default host name or Unix-domain socket location for <xref
-       linkend="app-psql"> (used when waiting for startup).
-      </para>
-     </listitem>
-    </varlistentry>
-
-    <varlistentry>
-     <term><envar>PGPORT</envar></term>
-
-     <listitem>
-      <para>
-       Default port number for <xref linkend="app-psql">
-       (used when waiting for startup).
-      </para>
-     </listitem>
-    </varlistentry>
-
    </variablelist>

    <para>
--- 433,438 ----
*************** PostgreSQL documentation
*** 506,523 ****
      </listitem>
     </varlistentry>

-    <varlistentry>
-     <term><filename>postgresql.conf</filename></term>
-
-     <listitem>
-      <para>
-       This file, located in the data directory, is parsed to find the
-       proper port to use with <application>psql</application>
-       when waiting for startup.
-      </para>
-     </listitem>
-    </varlistentry>
-
    </variablelist>
   </refsect1>

--- 475,480 ----
diff --git a/src/backend/utils/init/miscinit.c b/src/backend/utils/init/miscinit.c
index 14ed914..deb2d58 100644
*** /tmp/pgrevert.8079/PNev2b_miscinit.c    Wed Dec 22 10:10:23 2010
--- src/backend/utils/init/miscinit.c    Wed Dec 22 10:06:33 2010
***************
*** 33,38 ****
--- 33,39 ----
  #include "mb/pg_wchar.h"
  #include "miscadmin.h"
  #include "postmaster/autovacuum.h"
+ #include "postmaster/postmaster.h"
  #include "storage/fd.h"
  #include "storage/ipc.h"
  #include "storage/pg_shmem.h"
*************** CreateLockFile(const char *filename, boo
*** 658,664 ****
                 bool isDDLock, const char *refName)
  {
      int            fd;
!     char        buffer[MAXPGPATH + 100];
      int            ntries;
      int            len;
      int            encoded_pid;
--- 659,665 ----
                 bool isDDLock, const char *refName)
  {
      int            fd;
!     char        buffer[MAXPGPATH * 2 + 256];
      int            ntries;
      int            len;
      int            encoded_pid;
*************** CreateLockFile(const char *filename, boo
*** 868,876 ****
      /*
       * Successfully created the file, now fill it.
       */
!     snprintf(buffer, sizeof(buffer), "%d\n%s\n",
               amPostmaster ? (int) my_pid : -((int) my_pid),
!              DataDir);
      errno = 0;
      if (write(fd, buffer, strlen(buffer)) != strlen(buffer))
      {
--- 869,877 ----
      /*
       * Successfully created the file, now fill it.
       */
!     snprintf(buffer, sizeof(buffer), "%d\n%s\n%d\n%s\n",
               amPostmaster ? (int) my_pid : -((int) my_pid),
!              DataDir, PostPortNumber, UnixSocketDir);
      errno = 0;
      if (write(fd, buffer, strlen(buffer)) != strlen(buffer))
      {
*************** RecordSharedMemoryInLockFile(unsigned lo
*** 994,1001 ****
  {
      int            fd;
      int            len;
      char       *ptr;
!     char        buffer[BLCKSZ];

      fd = open(DIRECTORY_LOCK_FILE, O_RDWR | PG_BINARY, 0);
      if (fd < 0)
--- 995,1003 ----
  {
      int            fd;
      int            len;
+     int            lineno;
      char       *ptr;
!     char        buffer[MAXPGPATH * 2 + 256];

      fd = open(DIRECTORY_LOCK_FILE, O_RDWR | PG_BINARY, 0);
      if (fd < 0)
*************** RecordSharedMemoryInLockFile(unsigned lo
*** 1019,1036 ****
      buffer[len] = '\0';

      /*
!      * Skip over first two lines (PID and path).
       */
!     ptr = strchr(buffer, '\n');
!     if (ptr == NULL ||
!         (ptr = strchr(ptr + 1, '\n')) == NULL)
      {
!         elog(LOG, "bogus data in \"%s\"", DIRECTORY_LOCK_FILE);
!         close(fd);
!         return;
      }
!     ptr++;
!
      /*
       * Append key information.    Format to try to keep it the same length
       * always (trailing junk won't hurt, but might confuse humans).
--- 1021,1040 ----
      buffer[len] = '\0';

      /*
!      * Skip over first four lines (PID, pgdata, portnum, socketdir).
       */
!     ptr = buffer;
!     for (lineno = 1; lineno <= 4; lineno++)
      {
!         if ((ptr = strchr(ptr, '\n')) == NULL)
!         {
!             elog(LOG, "bogus data in \"%s\"", DIRECTORY_LOCK_FILE);
!             close(fd);
!             return;
!         }
!         ptr++;
      }
!
      /*
       * Append key information.    Format to try to keep it the same length
       * always (trailing junk won't hurt, but might confuse humans).
diff --git a/src/bin/pg_ctl/pg_ctl.c b/src/bin/pg_ctl/pg_ctl.c
index c5f855e..b91612a 100644
*** /tmp/pgrevert.8079/9n6jMb_pg_ctl.c    Wed Dec 22 10:10:23 2010
--- src/bin/pg_ctl/pg_ctl.c    Wed Dec 22 10:10:09 2010
*************** static bool postmaster_is_alive(pid_t pi
*** 141,147 ****

  static char postopts_file[MAXPGPATH];
  static char pid_file[MAXPGPATH];
- static char conf_file[MAXPGPATH];
  static char backup_file[MAXPGPATH];
  static char recovery_file[MAXPGPATH];

--- 141,146 ----
*************** start_postmaster(void)
*** 404,516 ****
  static PGPing
  test_postmaster_connection(bool do_checkpoint)
  {
      PGPing        ret = PQPING_OK;    /* assume success for wait == zero */
      int            i;
-     char        portstr[32];
-     char       *p;
-     char       *q;
-     char        connstr[128];    /* Should be way more than enough! */

!     portstr[0] = '\0';
!
!     /*
!      * Look in post_opts for a -p switch.
!      *
!      * This parsing code is not amazingly bright; it could for instance get
!      * fooled if ' -p' occurs within a quoted argument value.  Given that few
!      * people pass complicated settings in post_opts, it's probably good
!      * enough.
!      */
!     for (p = post_opts; *p;)
      {
!         /* advance past whitespace */
!         while (isspace((unsigned char) *p))
!             p++;
!
!         if (strncmp(p, "-p", 2) == 0)
          {
!             p += 2;
!             /* advance past any whitespace/quoting */
!             while (isspace((unsigned char) *p) || *p == '\'' || *p == '"')
!                 p++;
!             /* find end of value (not including any ending quote!) */
!             q = p;
!             while (*q &&
!                    !(isspace((unsigned char) *q) || *q == '\'' || *q == '"'))
!                 q++;
!             /* and save the argument value */
!             strlcpy(portstr, p, Min((q - p) + 1, sizeof(portstr)));
!             /* keep looking, maybe there is another -p */
!             p = q;
!         }
!         /* Advance to next whitespace */
!         while (*p && !isspace((unsigned char) *p))
!             p++;
!     }
!
!     /*
!      * Search config file for a 'port' option.
!      *
!      * This parsing code isn't amazingly bright either, but it should be okay
!      * for valid port settings.
!      */
!     if (!portstr[0])
!     {
!         char      **optlines;

!         optlines = readfile(conf_file);
!         if (optlines != NULL)
!         {
!             for (; *optlines != NULL; optlines++)
!             {
!                 p = *optlines;

!                 while (isspace((unsigned char) *p))
!                     p++;
!                 if (strncmp(p, "port", 4) != 0)
!                     continue;
!                 p += 4;
!                 while (isspace((unsigned char) *p))
!                     p++;
!                 if (*p != '=')
!                     continue;
!                 p++;
!                 /* advance past any whitespace/quoting */
!                 while (isspace((unsigned char) *p) || *p == '\'' || *p == '"')
!                     p++;
!                 /* find end of value (not including any ending quote/comment!) */
!                 q = p;
!                 while (*q &&
!                        !(isspace((unsigned char) *q) ||
!                          *q == '\'' || *q == '"' || *q == '#'))
!                     q++;
!                 /* and save the argument value */
!                 strlcpy(portstr, p, Min((q - p) + 1, sizeof(portstr)));
!                 /* keep looking, maybe there is another */
              }
          }
-     }
-
-     /* Check environment */
-     if (!portstr[0] && getenv("PGPORT") != NULL)
-         strlcpy(portstr, getenv("PGPORT"), sizeof(portstr));

!     /* Else use compiled-in default */
!     if (!portstr[0])
!         snprintf(portstr, sizeof(portstr), "%d", DEF_PGPORT);
!
!     /*
!      * We need to set a connect timeout otherwise on Windows the SCM will
!      * probably timeout first
!      */
!     snprintf(connstr, sizeof(connstr),
!              "dbname=postgres port=%s connect_timeout=5", portstr);

-     for (i = 0; i < wait_seconds; i++)
-     {
-         ret = PQping(connstr);
-         if (ret == PQPING_OK || ret == PQPING_NO_ATTEMPT)
-             break;
          /* No response, or startup still in process; wait */
  #if defined(WIN32)
          if (do_checkpoint)
--- 403,509 ----
  static PGPing
  test_postmaster_connection(bool do_checkpoint)
  {
+     int            portnum = 0;
+     char        socket_dir[MAXPGPATH];
+     char        connstr[MAXPGPATH + 256];
      PGPing        ret = PQPING_OK;    /* assume success for wait == zero */
+     char      **optlines;
      int            i;

!     socket_dir[0] = '\0';
!     connstr[0] = '\0';
!
!     for (i = 0; i < wait_seconds; i++)
      {
!         /* Do we need a connection string? */
!         if (connstr[0] == '\0')
          {
!             /*
!              *     The number of lines in postmaster.pid tells us several things:
!              *
!              *    # of lines
!              *        0    lock file created but status not written
!              *        2    pre-9.1 server, shared memory not created
!              *        3    pre-9.1 server, shared memory created
!              *        4    9.1+ server, shared memory not created
!              *        5    9.1+ server, shared memory created
!              *
!              *    For pre-9.1 Unix servers, we grab the port number from the
!              *    shmem key (first value on line 3).  Pre-9.1 Win32 has no
!              *    written shmem key, so we fail.  9.1+ writes both the port
!              *    number and socket address in the file for us to use.
!              */
!
!             /* Try to read a completed postmaster.pid file */
!             if ((optlines = readfile(pid_file)) != NULL &&
!                 optlines[0] != NULL &&
!                 optlines[1] != NULL &&
!                 optlines[2] != NULL)
!             {
!                 /* A 3-line file? */
!                 if (optlines[3] == NULL)
!                 {
!                     /*
!                      *    Pre-9.1:  on Unix, we get the port number by
!                      *    deriving it from the shmem key (the first number on
!                      *    on the line);  see
!                      *    miscinit.c::RecordSharedMemoryInLockFile().
!                      */
!                     portnum = atoi(optlines[2]) / 1000;
!                     /* Win32 does not give us a shmem key, so we fail. */
!                     if (portnum == 0)
!                     {
!                         write_stderr(_("%s: -w option is not supported on this platform\nwhen connecting to a pre-9.1
server\n"),
!                                      progname);
!                         return PQPING_NO_ATTEMPT;
!                     }
!                 }
!                 else    /* 9.1+ server */
!                 {
!                     portnum = atoi(optlines[2]);
!
!                     /* Get socket directory, if specified. */
!                     if (optlines[3][0] != '\n')
!                     {
!                         /*
!                          *    While unix_socket_directory can accept relative
!                          *    directories, libpq's host must have a leading slash
!                          *    to indicate a socket directory.
!                          */
!                         if (optlines[3][0] != '/')
!                         {
!                             write_stderr(_("%s: -w option cannot use a relative socket directory specification\n"),
!                                          progname);
!                             return PQPING_NO_ATTEMPT;
!                         }
!                         strlcpy(socket_dir, optlines[3], MAXPGPATH);
!                         /* remove newline */
!                         if (strchr(socket_dir, '\n') != NULL)
!                             *strchr(socket_dir, '\n') = '\0';
!                     }
!                 }

!                 /*
!                  * We need to set connect_timeout otherwise on Windows the
!                  * Service Control Manager (SCM) will probably timeout first.
!                  */
!                 snprintf(connstr, sizeof(connstr),
!                          "dbname=postgres port=%d connect_timeout=5", portnum);

!                 if (socket_dir[0] != '\0')
!                     snprintf(connstr + strlen(connstr), sizeof(connstr) - strlen(connstr),
!                         " host='%s'", socket_dir);
              }
          }

!         /* If we have a connection string, ping the server */
!         if (connstr[0] != '\0')
!         {
!             ret = PQping(connstr);
!             if (ret == PQPING_OK || ret == PQPING_NO_ATTEMPT)
!                 break;
!         }

          /* No response, or startup still in process; wait */
  #if defined(WIN32)
          if (do_checkpoint)
*************** main(int argc, char **argv)
*** 2009,2015 ****
      {
          snprintf(postopts_file, MAXPGPATH, "%s/postmaster.opts", pg_data);
          snprintf(pid_file, MAXPGPATH, "%s/postmaster.pid", pg_data);
-         snprintf(conf_file, MAXPGPATH, "%s/postgresql.conf", pg_data);
          snprintf(backup_file, MAXPGPATH, "%s/backup_label", pg_data);
          snprintf(recovery_file, MAXPGPATH, "%s/recovery.conf", pg_data);
      }
--- 2002,2007 ----