Re: fork/exec patch

Claudio Natoli  writes:
> This patch is the next step towards (re)allowing fork/exec.

I've included a few comments on the patch below. Unfortunately I only
had time to briefly look over the code...

Why did you change ShmemIndexLock from an LWLock to a spinlock?

The number of "FIXME" or "This is ugly" comments doesn't ease my mind
about this patch :-) How many of these issues do you plan to resolve?

Index: src/backend/tcop/postgres.c
===================================================================
RCS file: /projects/cvsroot/pgsql-server/src/backend/tcop/postgres.c,v
retrieving revision 1.379
diff -c -w -r1.379 postgres.c
*** src/backend/tcop/postgres.c	1 Dec 2003 22:15:37 -0000	1.379
--- src/backend/tcop/postgres.c	13 Dec 2003 06:18:44 -0000
***************
*** 2133,2139 ****
  			case 'D':			/* PGDATA directory */
  				if (secure)
  					potential_DataDir = optarg;
- 				break;
  
  			case 'd':			/* debug level */
  				{

Why was this change made? If you actually intend to fall through the
case here, please make it clear via a comment.

+ /*
+  * The following need to be available to the read/write_backend_variables
+  * functions
+  */
+ extern XLogRecPtr RedoRecPtr;
+ extern XLogwrtResult LogwrtResult;
+ extern slock_t *ShmemLock;
+ extern slock_t *ShmemIndexLock;
+ extern void *ShmemIndexAlloc;
+ typedef struct LWLock LWLock;
+ extern LWLock *LWLockArray;
+ extern slock_t  *ProcStructLock;
+ extern int	pgStatSock;

I wonder whether it is cleaner to make these properly public, rather
than using the NON_EXEC_STATIC #ifdef... (I'm not necessarily saying
it is, I'm just tossing it out there).

+ #define get_tmp_backend_var_file_name(buf,id)		\
+ 		Assert(DataDir);			\
+ 		sprintf((buf),				\
+ 			"%s/%s/%s.backend_var.%d",	\
+ 			DataDir,			\
+ 			PG_TEMP_FILES_DIR,		\
+ 			PG_TEMP_FILE_PREFIX,		\
+ 			(id))

This macro needs to be enclosed in a  do {} while (0) block. Also,
what does the "var" in the name of this macro refer to?

+ #define get_tmp_backend_var_dir(buf)		\
+ 		sprintf((buf),"%s/%s",DataDir,PG_TEMP_FILES_DIR)

This seems a little pointless, IMHO.

+ static void
+ write_backend_variables(pid_t pid, Port *port)
+ {
+ 	char	filename[MAXPGPATH];
+ 	FILE	*fp;
+ 	get_tmp_backend_var_file_name(filename,pid);
+ 
+ 	/* Open file */
+ 	fp = AllocateFile(filename, PG_BINARY_W);
+ 	if (!fp)
+ 	{
+ 		/* As per OpenTemporaryFile... */
+ 		char dirname[MAXPGPATH];
+ 		get_tmp_backend_var_dir(dirname);
+ 		mkdir(dirname, S_IRWXU);
+ 
+ 		fp = AllocateFile(filename, PG_BINARY_W);
+ 		if (!fp)
+ 		{
+ 			ereport(FATAL,
+ 				(errcode_for_file_access(),
+ 				errmsg("could not write to file \"%s\": %m", filename)));
+ 			return;
+ 		}
+ 	}

ereport(FATAL) seems too strong, doesn't it?

+ 	read_var(LWLockArray,fp);
+ 	read_var(ProcStructLock,fp);
+ 	read_var(pgStatSock,fp);
+ 
+ 	/* Release file */
+ 	FreeFile(fp);
+ 	unlink(filename);

You should probably check the return value of unlink().

(circa line 335 of ipc/shmem.c:)

  			/*
  			 * If the shmem index doesn't exist, we are bootstrapping: we must
  			 * be trying to init the shmem index itself.
  			 *
! 			 * Notice that the ShmemLock is held until the shmem index has
  			 * been completely initialized.
  			 */

Doesn't this function still acquire ShmemIndexLock? (i.e. why was this
comment changed?)

-Neil