Re: [pgsql-hackers-win32] Edit query buffer - Mailing list pgsql-patches

OK, it turns out there were multiple problems with psql \e and \!
related to quoting and the use of Win32 API functions.  That attached
patch fixes both of these, and uses stat() under Win32, but not
WIN32_CLIENT_ONLY.

---------------------------------------------------------------------------

Wood, Bruce wrote:
> I'm not sure if this goes here or to bugs, but it seems obvious
> (to me) that if this problem existed elsewhere, it would have
> been brought up by now.
>
> In the first version of the beta native Windows release, if I
> try to edit the query buffer using the \e command, psql crashes
> spectacularly.  The error is "The instruction at "0x00422078"
> referenced memory at "0x00000000".  The memory could not be
> "read"."  If I connect from a client machine to the database
> server and attempt the same thing, psql crashes less spectacularly.
> It just dies without a whimper.  Yes, I have PSQL_EDITOR set to
> notepad and if I use \e filename, it attempts to open "filename".
> If it exists, it opens.  If it doesn't exist, it asks to create
> it.  If it's any help, when the debugger opens, it says "Unhandled
> exception in psql.exe:  0xC0000005: Access Violation."  Selecting
> OK, the debugger is sitting on this line:
>
> 00422078   mov         al,byte ptr [ecx]
>
> if that means anything.
>
> Stepping forward to the present, attempting the same \e in the
> beta 4 release of psql, it gripes "could not open temporary file
> ".\psq48F.tmp": File exists".  At least now psql doesn't crash,
> taking the DOS window with it (if you launched psql from the
> menu).
>
> Coming from an Oracle environment, I kind of like being able to
> edit the buffer because I don't type very well.
>
> ---------------------------(end of broadcast)---------------------------
> TIP 9: the planner will ignore your desire to choose an index
> scan if your
>  joining column's datatypes do not match
>

--
  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/pg_ctl/pg_ctl.c
===================================================================
RCS file: /cvsroot/pgsql/src/bin/pg_ctl/pg_ctl.c,v
retrieving revision 1.44
diff -c -c -r1.44 pg_ctl.c
*** src/bin/pg_ctl/pg_ctl.c    27 Oct 2004 19:44:14 -0000    1.44
--- src/bin/pg_ctl/pg_ctl.c    4 Nov 2004 22:18:05 -0000
***************
*** 335,341 ****
       * http://dev.remotenetworktechnology.com/cmd/cmdfaq.htm
       */
      if (log_file != NULL)
! #if !defined(WIN32)    /* Cygwin doesn't have START */
          snprintf(cmd, MAXPGPATH, "%s\"%s\" %s%s < \"%s\" >> \"%s\" 2>&1 &%s",
  #else
          snprintf(cmd, MAXPGPATH, "%sSTART /B \"\" \"%s\" %s%s < \"%s\" >> \"%s\" 2>&1%s",
--- 335,341 ----
       * http://dev.remotenetworktechnology.com/cmd/cmdfaq.htm
       */
      if (log_file != NULL)
! #ifndef WIN32    /* Cygwin doesn't have START */
          snprintf(cmd, MAXPGPATH, "%s\"%s\" %s%s < \"%s\" >> \"%s\" 2>&1 &%s",
  #else
          snprintf(cmd, MAXPGPATH, "%sSTART /B \"\" \"%s\" %s%s < \"%s\" >> \"%s\" 2>&1%s",
***************
*** 343,349 ****
                   SYSTEMQUOTE, postgres_path, pgdata_opt, post_opts,
                   DEVNULL, log_file, SYSTEMQUOTE);
      else
! #if !defined(WIN32)    /* Cygwin doesn't have START */
          snprintf(cmd, MAXPGPATH, "%s\"%s\" %s%s < \"%s\" 2>&1 &%s",
  #else
          snprintf(cmd, MAXPGPATH, "%sSTART /B \"\" \"%s\" %s%s < \"%s\" 2>&1%s",
--- 343,349 ----
                   SYSTEMQUOTE, postgres_path, pgdata_opt, post_opts,
                   DEVNULL, log_file, SYSTEMQUOTE);
      else
! #ifndef WIN32    /* Cygwin doesn't have START */
          snprintf(cmd, MAXPGPATH, "%s\"%s\" %s%s < \"%s\" 2>&1 &%s",
  #else
          snprintf(cmd, MAXPGPATH, "%sSTART /B \"\" \"%s\" %s%s < \"%s\" 2>&1%s",
Index: src/bin/psql/command.c
===================================================================
RCS file: /cvsroot/pgsql/src/bin/psql/command.c,v
retrieving revision 1.129
diff -c -c -r1.129 command.c
*** src/bin/psql/command.c    16 Oct 2004 03:10:16 -0000    1.129
--- src/bin/psql/command.c    4 Nov 2004 22:18:08 -0000
***************
*** 23,28 ****
--- 23,32 ----
  #include <io.h>
  #include <fcntl.h>
  #include <direct.h>
+ #ifndef WIN32_CLIENT_ONLY
+ #include <sys/types.h>            /* for umask() */
+ #include <sys/stat.h>            /* for stat() */
+ #endif
  #endif

  #include "libpq-fe.h"
***************
*** 1097,1103 ****
  #ifndef WIN32
              "exec "
  #endif
!             "%s '%s'", editorName, fname);
      result = system(sys);
      if (result == -1)
          psql_error("could not start editor \"%s\"\n", editorName);
--- 1101,1107 ----
  #ifndef WIN32
              "exec "
  #endif
!             "%s\"%s\" \"%s\"%s", SYSTEMQUOTE, editorName, fname, SYSTEMQUOTE);
      result = system(sys);
      if (result == -1)
          psql_error("could not start editor \"%s\"\n", editorName);
***************
*** 1119,1125 ****
      bool        error = false;
      int            fd;

! #ifndef WIN32
      struct stat before,
                  after;
  #endif
--- 1123,1129 ----
      bool        error = false;
      int            fd;

! #ifndef WIN32_CLIENT_ONLY
      struct stat before,
                  after;
  #endif
***************
*** 1130,1142 ****
      {
          /* make a temp file to edit */
  #ifndef WIN32
!         const char *tmpdirenv = getenv("TMPDIR");

!         snprintf(fnametmp, sizeof(fnametmp), "%s/psql.edit.%d.%d",
!                  tmpdirenv ? tmpdirenv : "/tmp", geteuid(), (int)getpid());
  #else
!         GetTempFileName(".", "psql", 0, fnametmp);
  #endif
          fname = (const char *) fnametmp;

          fd = open(fname, O_WRONLY | O_CREAT | O_EXCL, 0600);
--- 1134,1168 ----
      {
          /* make a temp file to edit */
  #ifndef WIN32
!         const char *tmpdir = getenv("TMPDIR");
!
!         if (!tmpdir)
!             tmpdir = "/tmp";
! #else
!         char tmpdir[MAXPGPATH];
!         int ret;

!         ret = GetTempPath(MAXPGPATH, tmpdir);
!         if (ret == 0 || ret > MAXPGPATH)
!         {
!             psql_error("Can not locate temporary directory: %s",
!                         !ret ? strerror(errno) : "");
!             return false;
!         }
!         /*
!          *    No canonicalize_path() here.
!          *    EDIT.EXE run from CMD.EXE prepends the current directory to the
!          *    supplied path unless we use only backslashes, so we do that.
!          */
! #endif
!         snprintf(fnametmp, sizeof(fnametmp), "%s%spsql.edit.%d", tmpdir,
! #ifndef WIN32
!                 "/",
  #else
!                 "",    /* trailing separator already present */
  #endif
+                 (int)getpid());
+
          fname = (const char *) fnametmp;

          fd = open(fname, O_WRONLY | O_CREAT | O_EXCL, 0600);
***************
*** 1174,1180 ****
          }
      }

! #ifndef WIN32
      if (!error && stat(fname, &before) != 0)
      {
          psql_error("%s: %s\n", fname, strerror(errno));
--- 1200,1206 ----
          }
      }

! #ifndef WIN32_CLIENT_ONLY
      if (!error && stat(fname, &before) != 0)
      {
          psql_error("%s: %s\n", fname, strerror(errno));
***************
*** 1186,1192 ****
      if (!error)
          error = !editFile(fname);

! #ifndef WIN32
      if (!error && stat(fname, &after) != 0)
      {
          psql_error("%s: %s\n", fname, strerror(errno));
--- 1212,1218 ----
      if (!error)
          error = !editFile(fname);

! #ifndef WIN32_CLIENT_ONLY
      if (!error && stat(fname, &after) != 0)
      {
          psql_error("%s: %s\n", fname, strerror(errno));
***************
*** 1509,1517 ****
      if (!command)
      {
          char       *sys;
!         const char *shellName;

!         shellName = getenv("SHELL");
          if (shellName == NULL)
              shellName = DEFAULT_SHELL;

--- 1535,1547 ----
      if (!command)
      {
          char       *sys;
!         const char *shellName = NULL;

! #ifdef WIN32
!         shellName = getenv("COMSPEC");
! #endif
!         if (shellName == NULL)
!             shellName = getenv("SHELL");
          if (shellName == NULL)
              shellName = DEFAULT_SHELL;

***************
*** 1520,1526 ****
  #ifndef WIN32
                  "exec "
  #endif
!                 "%s", shellName);
          result = system(sys);
          free(sys);
      }
--- 1550,1556 ----
  #ifndef WIN32
                  "exec "
  #endif
!                 "%s\"%s\"%s", SYSTEMQUOTE, shellName, SYSTEMQUOTE);
          result = system(sys);
          free(sys);
      }