From: | Claudio Natoli <claudio(dot)natoli(at)memetrics(dot)com> |
---|---|
To: | 'Neil Conway ' <neilc(at)samurai(dot)com>, Claudio Natoli <claudio(dot)natoli(at)memetrics(dot)com> |
Cc: | "''pgsql-patches(at)postgresql(dot)org' '" <pgsql-patches(at)postgresql(dot)org> |
Subject: | Re: fork/exec patch |
Date: | 2003-12-16 07:31:20 |
Message-ID: | A02DEC4D1073D611BAE8525405FCCE2B028094@harris.memetrics.local |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-patches |
[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>
From | Date | Subject | |
---|---|---|---|
Next Message | Neil Conway | 2003-12-16 07:44:43 | Re: fork/exec patch |
Previous Message | Tom Lane | 2003-12-16 05:11:04 | Re: fork/exec patch |