Re: fork/exec patch
От | Claudio Natoli |
---|---|
Тема | Re: fork/exec patch |
Дата | |
Msg-id | A02DEC4D1073D611BAE8525405FCCE2B028094@harris.memetrics.local обсуждение исходный текст |
Ответ на | fork/exec patch (Claudio Natoli <claudio.natoli@memetrics.com>) |
Ответы |
Re: fork/exec patch
(Neil Conway <neilc@samurai.com>)
Re: fork/exec patch (Bruce Momjian <pgman@candle.pha.pa.us>) |
Список | 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>
В списке pgsql-patches по дате отправления: