[Thanks to Tom + Bruce]

For the remaining comments...


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

All of them, as we go along. Treat this as a first step.


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

I can't believe that got through! It is an editing mistake, pure and simple.
Thank you for catching it. [bashes head against wall]


> + #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?

"var" refers to "variable". Will add a do while block in a following patch.


> + #define get_tmp_backend_var_dir(buf)                \
> +             sprintf((buf),"%s/%s",DataDir,PG_TEMP_FILES_DIR)
> 
> This seems a little pointless, IMHO.

True.


> + static void
> [snip]
> ereport(FATAL) seems too strong, doesn't it?

Possibly.


> +     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().

No. This isn't necessary (and what action would it take in any case?). If it
doesn't unlink the file, tough. A backend crash could also leave these files
on the disk. Like the other pgtmp files they'll get cleaned up on postmaster
restart.


> (circa line 335 of ipc/shmem.c:)
> [snip]
> Doesn't this function still acquire ShmemIndexLock? (i.e. why was this
comment changed?)

AFAICS this is just whitespace differences.

With the exception of that missing "break" (Bruce, I guess it goes without
saying, but could you please remove that line from the patch before
applying... and again "Thank you Neil"), these are stylistic/cosmetic and
effect the EXEC_BACKEND code only.

Would a follow-up patch to correct these, along with the next step of the
fork/exec changes, be acceptable?

Cheers,
Claudio

--- 
Certain disclaimers and policies apply to all email sent from Memetrics.
For the full text of these disclaimers and policies see 
<a
href="http://www.memetrics.com/emailpolicy.html";>http://www.memetrics.com/em
ailpolicy.html</a>

---------------------------(end of broadcast)---------------------------
TIP 6: Have you searched our list archives?

               http://archives.postgresql.org

Reply via email to