Re: fork/exec patch - Mailing list pgsql-patches

Claudio Natoli <claudio.natoli@memetrics.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