Re: [HACKERS] What's left? - Mailing list pgsql-patches

Bruce Momjian wrote:
> Tom Lane wrote:
> > Bruce Momjian <pgman@candle.pha.pa.us> writes:
> > > In this way, no one ever has the rename file open while we are holding
> > > the locks, and we can loop without holding an exclusive lock on
> > > pg_shadow, and file writes remain in order.
> >
> > You're doing this where exactly, and are certain that you are holding no
> > locks why exactly?  And if you aren't holding a lock, what prevents
> > concurrency bugs?
>
> Oh, for concurrency bugs, you make realfile.new while holding the
> exclusive lock, so someone could come in later and replace realfile.new
> while I am in the rename loop, but then I just install theirs instead.
>
> I could install someone who has just done the rename to realfile.new but
> not tried the rename from realfile.new to realfile, but that seems OK.
> They will just fine the file missing and fail on the rename, which is
> OK.

OK, here is a patch that I think handles rename.  It does the rename to
a secondary file while holding the lock, then releases the lock and does
a rename to the active file.  I enabled this for Win32 and Cygwin, which
has the same file system behavior.

--
  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/backend/commands/user.c
===================================================================
RCS file: /cvsroot/pgsql-server/src/backend/commands/user.c,v
retrieving revision 1.133
diff -c -c -r1.133 user.c
*** src/backend/commands/user.c    26 Jan 2004 22:35:32 -0000    1.133
--- src/backend/commands/user.c    27 Jan 2004 03:46:00 -0000
***************
*** 139,145 ****
      bufsize = strlen(filename) + 12;
      tempname = (char *) palloc(bufsize);
      snprintf(tempname, bufsize, "%s.%d", filename, MyProcPid);
!
      oumask = umask((mode_t) 077);
      fp = AllocateFile(tempname, "w");
      umask(oumask);
--- 139,149 ----
      bufsize = strlen(filename) + 12;
      tempname = (char *) palloc(bufsize);
      snprintf(tempname, bufsize, "%s.%d", filename, MyProcPid);
! #if defined(WIN32) || defined(CYGWIN)
!     filename = repalloc(filename, strlen(filename) + 1 + strlen(".new");
!     strcat(filename, ".new");
! #endif
!
      oumask = umask((mode_t) 077);
      fp = AllocateFile(tempname, "w");
      umask(oumask);
***************
*** 286,291 ****
--- 290,299 ----
      bufsize = strlen(filename) + 12;
      tempname = (char *) palloc(bufsize);
      snprintf(tempname, bufsize, "%s.%d", filename, MyProcPid);
+ #if defined(WIN32) || defined(CYGWIN)
+     filename = repalloc(filename, strlen(filename) + 1 + strlen(".new");
+     strcat(filename, ".new");
+ #endif

      oumask = umask((mode_t) 077);
      fp = AllocateFile(tempname, "w");
***************
*** 457,462 ****
--- 465,482 ----
          user_file_update_needed = false;
          write_user_file(urel);
          heap_close(urel, NoLock);
+ #if defined(WIN32) || defined(CYGWIN)
+         {
+             /* Rename active file while not holding an exclusive lock */
+             char *filename = user_getfilename(), *filename_new;
+
+             filename_new = palloc(strlen(filename) + 1 + strlen(".new")));
+             sprintf(filename_new, "%s.new", filename);
+             rename(filename_new, filename);
+             pfree(filename);
+             pfree(filename_new);
+         }
+ #endif
      }

      if (group_file_update_needed)
***************
*** 464,469 ****
--- 484,501 ----
          group_file_update_needed = false;
          write_group_file(grel);
          heap_close(grel, NoLock);
+ #if defined(WIN32) || defined(CYGWIN)
+         {
+             /* Rename active file while not holding an exclusive lock */
+             char *filename = group_getfilename(), *filename_new;
+
+             filename_new = palloc(strlen(filename) + 1 + strlen(".new")));
+             sprintf(filename_new, "%s.new", filename);
+             rename(filename_new, filename);
+             pfree(filename);
+             pfree(filename_new);
+         }
+ #endif
      }

      /*
Index: src/backend/utils/cache/relcache.c
===================================================================
RCS file: /cvsroot/pgsql-server/src/backend/utils/cache/relcache.c,v
retrieving revision 1.195
diff -c -c -r1.195 relcache.c
*** src/backend/utils/cache/relcache.c    26 Jan 2004 22:35:32 -0000    1.195
--- src/backend/utils/cache/relcache.c    27 Jan 2004 03:46:04 -0000
***************
*** 3358,3390 ****
          /*
           * OK, rename the temp file to its final name, deleting any
           * previously-existing init file.
-          *
-          * Note: a failure here is possible under Cygwin, if some other
-          * backend is holding open an unlinked-but-not-yet-gone init file.
-          * So treat this as a noncritical failure.
           */
!         if (rename(tempfilename, finalfilename) < 0)
          {
!             ereport(WARNING,
!                     (errcode_for_file_access(),
!                 errmsg("could not rename relation-cache initialization file \"%s\" to \"%s\": %m",
!                        tempfilename, finalfilename),
!                      errdetail("Continuing anyway, but there's something wrong.")));
!
!             /*
!              * If we fail, try to clean up the useless temp file; don't
!              * bother to complain if this fails too.
!              */
!             unlink(tempfilename);
          }
      }
      else
      {
          /* Delete the already-obsolete temp file */
          unlink(tempfilename);
      }
-
-     LWLockRelease(RelCacheInitLock);
  }

  /*
--- 3358,3385 ----
          /*
           * OK, rename the temp file to its final name, deleting any
           * previously-existing init file.
           */
! #if defined(WIN32) || defined(CYGWIN)
!         rename(tempfilename, finalfilename);
!         LWLockRelease(RelCacheInitLock);
! #else
          {
!             char        finalfilename_new[MAXPGPATH];
!
!             snprintf(finalfilename_new, sizeof(finalfilename_new), "%s.new", finalfilename);
!             rename(tempfilename, finalfilename_new);
!             LWLockRelease(RelCacheInitLock);
!             /* Rename to active file after lock is released */
!             rename(finalfilename_new, finalfilename);
          }
+ #endif
      }
      else
      {
          /* Delete the already-obsolete temp file */
          unlink(tempfilename);
+         LWLockRelease(RelCacheInitLock);
      }
  }

  /*
Index: src/backend/utils/misc/guc.c
===================================================================
RCS file: /cvsroot/pgsql-server/src/backend/utils/misc/guc.c,v
retrieving revision 1.181
diff -c -c -r1.181 guc.c
*** src/backend/utils/misc/guc.c    26 Jan 2004 22:35:32 -0000    1.181
--- src/backend/utils/misc/guc.c    27 Jan 2004 03:46:07 -0000
***************
*** 3981,3987 ****
          return;
      }

!     /* Put new file in place, this could delay on Win32 */
      rename(new_filename, filename);
      free(new_filename);
      free(filename);
--- 3981,3990 ----
          return;
      }

!     /*
!      *    Put new file in place.  This could delay on Win32, but we don't hold
!      *    any exclusive locks.
!      */
      rename(new_filename, filename);
      free(new_filename);
      free(filename);
Index: src/include/port.h
===================================================================
RCS file: /cvsroot/pgsql-server/src/include/port.h,v
retrieving revision 1.15
diff -c -c -r1.15 port.h
*** src/include/port.h    29 Nov 2003 22:40:53 -0000    1.15
--- src/include/port.h    27 Jan 2004 03:46:07 -0000
***************
*** 30,40 ****
  extern off_t ftello(FILE *stream);
  #endif

! #ifdef WIN32
  /*
   * Win32 doesn't have reliable rename/unlink during concurrent access
   */
- #ifndef FRONTEND
  extern int    pgrename(const char *from, const char *to);
  extern int    pgunlink(const char *path);

--- 30,39 ----
  extern off_t ftello(FILE *stream);
  #endif

! #if !defined(FRONTEND) && (defined(WIN32) || defined(CYGWIN))
  /*
   * Win32 doesn't have reliable rename/unlink during concurrent access
   */
  extern int    pgrename(const char *from, const char *to);
  extern int    pgunlink(const char *path);

***************
*** 42,47 ****
--- 41,47 ----
  #define unlink(path)        pgunlink(path)
  #endif

+ #ifdef WIN32
  extern int    copydir(char *fromdir, char *todir);

  /* Last parameter not used */
Index: src/port/dirmod.c
===================================================================
RCS file: /cvsroot/pgsql-server/src/port/dirmod.c,v
retrieving revision 1.8
diff -c -c -r1.8 dirmod.c
*** src/port/dirmod.c    29 Nov 2003 19:52:13 -0000    1.8
--- src/port/dirmod.c    27 Jan 2004 03:46:08 -0000
***************
*** 27,35 ****
--- 27,45 ----
  {
      int            loops = 0;

+ #ifdef WIN32
      while (!MoveFileEx(from, to, MOVEFILE_REPLACE_EXISTING))
+ #endif
+ #ifdef CYGWIN
+     while (rename(from, to) < 0)
+ #endif
      {
+ #ifdef WIN32
          if (GetLastError() != ERROR_ACCESS_DENIED)
+ #endif
+ #ifdef CYGWIN
+         if (errno != EACCES)
+ #endif
              /* set errno? */
              return -1;
          Sleep(100);                /* ms */