Re: [HACKERS] What's left?

[Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us>]

Bruce Momjian wrote:
> Tom Lane wrote:
> > Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)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(at)candle(dot)pha(dot)pa(dot)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 */