pgman wrote:
> > > PeerDirect handles rename by just looping.  We really can't delay a
> > > rename.  There is discussion in the Win32 TODO detail that goes over
> > > some options, I think.
> > 
> > Do we really have any problem with rename?  We don't rename table files.
> > The renames I can think of are renaming temp files into place as
> > permanent ones, and there would be no open references to such a file.
> 
> We do have a problem.  It is with cache files read on startup, like
> pg_pwd.  We can generate the file as temp, but we have to slide it in
> while a backend is not reading it.  On a busy system, I am not sure how
> large a window we will get for the rename.  The rename is all
> centralized in port/dirmod.c, so we can deal with it there, whatever the
> solution.
> 
> We also have to do the rename during xact close because we need to hold
> locks so we are sure the files are written in the same order that they
> modify pg_shadow, waiting a long time for the rename is a serious
> problem.

I think I have a solution to this.  The problem with rename is that if
the file is open under win32, we can't rename, so we loop, but we are
still holding locks.

My idea is to do this:

	grab lock(e.g. pg_shadow)
	write temp file
	rename temp file to realfile.new
	release lock
	rename realfile.new to realfile

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.

It's so easy, I think I could code it myself.  :-)

This, along with the idea of having the checkpoint delete files that are
pending should solve both problems for us.

-- 
  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

Mon, 26 Jan 2004 20:23:08 -0500 (EST)
Bruce Momjian <pgman@candle.pha.pa.us>
Re: What's left?
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?

			regards, tom lane

Mon, 26 Jan 2004 20:26:23 -0500
Tom Lane <tgl@sss.pgh.pa.us>
Re: What's left? 
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?

I am looking now at the relcache file, pg_pwd and pg_group.  I am sure I
am holding some locks, but not an exclusive lock on e.g. pg_shadow. I am
working on a patch now.  I don't expect to eliminate the looping for
rename, but to eliminate holding exclusive locks while doing the rename
to a file actively being read.  By using realfile.new, the first rename
is only being done on a file that is never opened, just renamed, which
should be quick.

I can't think of a cleaner solution.

-- 
  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

Mon, 26 Jan 2004 20:51:12 -0500 (EST)
Bruce Momjian <pgman@candle.pha.pa.us>
Re: [pgsql-hackers-win32] What's left?
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.

-- 
  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

Mon, 26 Jan 2004 20:53:02 -0500 (EST)
Bruce Momjian <pgman@candle.pha.pa.us>
Re: What's left?
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 */
Mon, 26 Jan 2004 23:01:43 -0500 (EST)
Bruce Momjian <pgman@candle.pha.pa.us>
Re: [HACKERS] What's left?