Re: Fix pg_ctl restart bug - Mailing list pgsql-patches

Bruce Momjian wrote:
> > > > > "", meaning zero-length string.  I should have seen the bug when I
> > > > > applied the contributed patch in 2004.
> > > >
> > > > So, shouldn't this fix be back-patched?
> > >
> > > Well, no one has actually complained about the breakage, and it has been
> > > a few years.  Also there is always the risk of a new bug being
> > > introduced, so I am unsure.
> >
> > Why do we need someone to complain?  We know the bug is there.  Has the
> > code changed a lot in that area?
>
> Do we have the policy of backpatching every fix?  I thought it was only
> the major bugs we fixed in back branches.  If someone wants to backpatch
> it, feel free to do so.

OK, I started looking at what it would take to backpatch this and found
another bug I have fixed in CVS HEAD.  What back branchs (8.0-8.3.X) are
doing is pretty odd.  On non-Win32 systems, it is looking for the null
byte, then putting a null byte before it, and passing a NULL back as the
options and binary location.  The test:

                if (postgres_path != NULL)
                    postgres_path = optline;

is backwards, which means that if in 8.3.X you start the server with any
arguments, like:

    /usr/var/local/postgres/bin/postgres -i -o -d5

and you use pg_ctl to specify the binary location:

    pg_ctl -p /u/pg/bin/postmaster restart

the server actually fails to restart because it chops off the last byte
(a bug) and the test above is wrong (another bug), and it thinks the
binary name is the full string, in quotes:

    /usr/var/local/postgres/bin/postgres -i -o -d

and you get this error from pg_ctl:

    sh: /usr/var/local/postgres/bin/postgres -i -o -d: not found

This is more than just ignoring the documentation, it is a failure.

I am attaching a minimal patch that will fix the bug in back branches.
Keep in mind that a patched pg_ctl will not be able to restart a backend
that was not patched.

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

  + If your life is a hard drive, Christ can be your backup. +
Index: src/backend/postmaster/postmaster.c
===================================================================
RCS file: /cvsroot/pgsql/src/backend/postmaster/postmaster.c,v
retrieving revision 1.551
diff -c -c -r1.551 postmaster.c
*** src/backend/postmaster/postmaster.c    11 Jan 2008 00:54:09 -0000    1.551
--- src/backend/postmaster/postmaster.c    26 Jun 2008 18:53:42 -0000
***************
*** 4163,4169 ****

      fprintf(fp, "%s", fullprogname);
      for (i = 1; i < argc; i++)
!         fprintf(fp, " %s%s%s", SYSTEMQUOTE, argv[i], SYSTEMQUOTE);
      fputs("\n", fp);

      if (fclose(fp))
--- 4163,4169 ----

      fprintf(fp, "%s", fullprogname);
      for (i = 1; i < argc; i++)
!         fprintf(fp, " \"%s\"", argv[i]);
      fputs("\n", fp);

      if (fclose(fp))
Index: src/bin/pg_ctl/pg_ctl.c
===================================================================
RCS file: /cvsroot/pgsql/src/bin/pg_ctl/pg_ctl.c,v
retrieving revision 1.92.2.3
diff -c -c -r1.92.2.3 pg_ctl.c
*** src/bin/pg_ctl/pg_ctl.c    29 Feb 2008 23:31:42 -0000    1.92.2.3
--- src/bin/pg_ctl/pg_ctl.c    26 Jun 2008 18:53:43 -0000
***************
*** 613,627 ****
              {
                  char       *arg1;

!                 arg1 = strchr(optline, *SYSTEMQUOTE);
!                 if (arg1 == NULL || arg1 == optline)
!                     post_opts = "";
!                 else
                  {
!                     *(arg1 - 1) = '\0'; /* this should be a space */
!                     post_opts = arg1;
                  }
!                 if (postgres_path != NULL)
                      postgres_path = optline;
              }
              else
--- 613,628 ----
              {
                  char       *arg1;

!                 /*
!                  * Are we at the first option, as defined by space and
!                  * double-quote?
!                  */
!                 if ((arg1 = strstr(optline, " \"")) != NULL)
                  {
!                     *arg1 = '\0';    /* terminate so we get only program name */
!                     post_opts = arg1 + 1; /* point past whitespace */
                  }
!                 if (postgres_path == NULL)
                      postgres_path = optline;
              }
              else