Re: fork/exec patch

Claudio Natoli <claudio(dot)natoli(at)memetrics(dot)com> 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