Claudio Natoli <[EMAIL PROTECTED]> 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
---------------------------(end of broadcast)---------------------------
TIP 6: Have you searched our list archives?
http://archives.postgresql.org
