Re: pg_ctl start broken on windows

Andrew Dunstan wrote:
> Bruce Momjian wrote:
> 
> >Andrew Dunstan wrote:
> >  
> >
> >>>>C:\msys\1.0\local\pgsql>bin\pg_ctl -D data -l logfile start
> >>>>'C:/msys/1.0/local/pgsql/bin/postmaster.exe"  < nul >>"logfile' is not recognized as an internal or external
command,operable program or batch file.
 
> >>>>
> >>>>
> >>I'll test to make sure, but I can tell you now it won't work. It will 
> >>see the inner quotes and interpret them as part of the command. (I tried 
> >>all these variants when I was testing initdb).
> >>    
> >>
> >
> >Claudio reported it worked for pg_dumpall for single quotes:
> >
> >    system(''cmd.exe' 'arg1' 'arg2'');
> >
> >What exactly is the logic of the stripping?
> >
> >In fact, pg_ctl is wrong because it is using double quotes instead of
> >the safer single quotes.  Let me make that change.
> >  
> >
> 
> No. It chokes on forward slashes.
> 
> and it tries to make a log file called 'logfile' (quotes included).
> 
> I really don't see what the problem is with calling CreateProcess - it 
> should only be a few lines of code unless I'm much mistaken, and would 
> get us out of this setup that is really fragile at best.

I did a lot of poking around in MinGW and found some basic rules. First,
MinGW's system() can't use single quotes for the executable name or any
file used in redirection (as you showed above).  Second, it gets
confused with multiple double-quoted strings.  The fix is to add a
double-quote to the beginning and end of the system string, so a typical
system string becomes:

    system("\"\"ls\" 'a' 'b c' > \"out\"\");

Single quotes are fine for arguments.

The applied patch makes this change and documents the behavior.  Would
folks test these changes on Win32 please?

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073
Index: src/bin/initdb/initdb.c
===================================================================
RCS file: /cvsroot/pgsql-server/src/bin/initdb/initdb.c,v
retrieving revision 1.35
diff -c -c -r1.35 initdb.c
*** src/bin/initdb/initdb.c    3 Jun 2004 00:07:36 -0000    1.35
--- src/bin/initdb/initdb.c    10 Jun 2004 16:24:17 -0000
***************
*** 812,823 ****
      for (i = 0; i < len; i++)
      {
          snprintf(cmd, sizeof(cmd),
!                  "\"%s\" -boot -x0 %s "
                   "-c shared_buffers=%d -c max_connections=%d template1 "
!                  "<%s >%s 2>&1",
!                  backend_exec, boot_options,
                   conns[i] * 5, conns[i],
!                  DEVNULL, DEVNULL);
          status = system(cmd);
          if (status == 0)
              break;
--- 812,823 ----
      for (i = 0; i < len; i++)
      {
          snprintf(cmd, sizeof(cmd),
!                  "%s\"%s\" -boot -x0 %s "
                   "-c shared_buffers=%d -c max_connections=%d template1 "
!                  "< \"%s\" > \"%s\" 2>&1%s",
!                  SYSTEMQUOTE, backend_exec, boot_options,
                   conns[i] * 5, conns[i],
!                  DEVNULL, DEVNULL, SYSTEMQUOTE);
          status = system(cmd);
          if (status == 0)
              break;
***************
*** 848,859 ****
      for (i = 0; i < len; i++)
      {
          snprintf(cmd, sizeof(cmd),
!                  "\"%s\" -boot -x0 %s "
                   "-c shared_buffers=%d -c max_connections=%d template1 "
!                  "<%s >%s 2>&1",
!                  backend_exec, boot_options,
                   bufs[i], n_connections,
!                  DEVNULL, DEVNULL);
          status = system(cmd);
          if (status == 0)
              break;
--- 848,859 ----
      for (i = 0; i < len; i++)
      {
          snprintf(cmd, sizeof(cmd),
!                  "%s\"%s\" -boot -x0 %s "
                   "-c shared_buffers=%d -c max_connections=%d template1 "
!                  "< \"%s\" > \"%s\" 2>&1%s",
!                  SYSTEMQUOTE, backend_exec, boot_options,
                   bufs[i], n_connections,
!                  DEVNULL, DEVNULL, SYSTEMQUOTE);
          status = system(cmd);
          if (status == 0)
              break;
Index: src/bin/pg_ctl/pg_ctl.c
===================================================================
RCS file: /cvsroot/pgsql-server/src/bin/pg_ctl/pg_ctl.c,v
retrieving revision 1.8
diff -c -c -r1.8 pg_ctl.c
*** src/bin/pg_ctl/pg_ctl.c    9 Jun 2004 17:36:07 -0000    1.8
--- src/bin/pg_ctl/pg_ctl.c    10 Jun 2004 16:24:17 -0000
***************
*** 224,234 ****
  
      /* Does '&' work on Win32? */
      if (log_file != NULL)
!         snprintf(cmd, MAXPGPATH, "'%s' %s < %s >> '%s' 2>&1 &",
!                  postgres_path, post_opts, DEVNULL, log_file);
      else
!         snprintf(cmd, MAXPGPATH, "'%s' %s < %s 2>&1 &",
!                  postgres_path, post_opts, DEVNULL);
      return system(cmd);
  }
  
--- 224,235 ----
  
      /* Does '&' work on Win32? */
      if (log_file != NULL)
!         snprintf(cmd, MAXPGPATH, "%s\"%s\" %s < %s >> \"%s\" 2>&1 &%s",
!                  SYSTEMQUOTE, postgres_path, post_opts, DEVNULL, log_file,
!                  SYSTEMQUOTE);
      else
!         snprintf(cmd, MAXPGPATH, "%s\"%s\" %s < \"%s\" 2>&1 &%s",
!                  SYSTEMQUOTE, postgres_path, post_opts, DEVNULL, SYSTEMQUOTE);
      return system(cmd);
  }
  
Index: src/bin/pg_dump/pg_dumpall.c
===================================================================
RCS file: /cvsroot/pgsql-server/src/bin/pg_dump/pg_dumpall.c,v
retrieving revision 1.40
diff -c -c -r1.40 pg_dumpall.c
*** src/bin/pg_dump/pg_dumpall.c    9 Jun 2004 17:37:28 -0000    1.40
--- src/bin/pg_dump/pg_dumpall.c    10 Jun 2004 16:24:18 -0000
***************
*** 690,696 ****
      const char *p;
      int            ret;
  
!     appendPQExpBuffer(cmd, "'%s' %s -Fp '", pg_dump_bin, pgdumpopts->data);
  
      /* Shell quoting is not quite like SQL quoting, so can't use fmtId */
      for (p = dbname; *p; p++)
--- 690,697 ----
      const char *p;
      int            ret;
  
!     appendPQExpBuffer(cmd, "%s\"%s\" %s -Fp '", SYSTEMQUOTE, pg_dump_bin,
!                       pgdumpopts->data);
  
      /* Shell quoting is not quite like SQL quoting, so can't use fmtId */
      for (p = dbname; *p; p++)
***************
*** 702,707 ****
--- 703,709 ----
      }
  
      appendPQExpBufferChar(cmd, '\'');
+     appendStringLiteral(cmd, SYSTEMQUOTE, false);
  
      if (verbose)
          fprintf(stderr, _("%s: running \"%s\"\n"), progname, cmd->data);
Index: src/include/port.h
===================================================================
RCS file: /cvsroot/pgsql-server/src/include/port.h,v
retrieving revision 1.40
diff -c -c -r1.40 port.h
*** src/include/port.h    3 Jun 2004 00:07:38 -0000    1.40
--- src/include/port.h    10 Jun 2004 16:24:18 -0000
***************
*** 72,77 ****
--- 72,89 ----
  #define DEVNULL "/dev/null"
  #endif
  
+ /*
+  *    Win32 needs double quotes at the beginning and end of system()
+  *    strings.  If not, it gets confused with multiple quoted strings.
+  *    It also must use double-quotes around the executable name
+  *    and any files use for redirection.  Other args can use single-quotes.
+  */
+ #ifdef WIN32
+ #define SYSTEMQUOTE "\""
+ #else
+ #define SYSTEMQUOTE ""
+ #endif
+ 
  /* Portable delay handling */
  extern void pg_usleep(long microsec);