Tom Lane wrote:
> Bruce Momjian <pgman@candle.pha.pa.us> writes:
> >> Actually, my recommendation is to remove it altogether. The mechanisms
> >> are in place to close allocated files after elog(), so why waste thought
> >> and code space to release them manually?
>
> > Fix applied. There is a FileFree() just below this in the code:
> > if (!pipe)
> > FreeFile(fp);
> > We don't need the if (!pipe) because this code is in an else of
^^^^
I should have said "in the _new_ code" above.
> > if(pipe). For clarity, it seems the FreeFile call makes sense.
>
> Huh? That FreeFile is *necessary* because it is not in an elog(ERROR)
> path; and by my reading the "if (!pipe)" is needed too.
Woh, I know we need to keep this code:
if (!pipe)
FreeFile(fp);
I was saying we don't need if (!pipe) in the new FreeFile() we just
added.
> We do have a fair amount of other code that releases resources just
> before doing elog(ERROR); so Brent was just emulating code he saw
> elsewhere... but it's a coding habit I think we should move away from.
> If the resource in question would be released anyway during error
> recovery, then I don't see the value of doing it "by hand"; it just
> bloats the backend (and adds potential for error, as in this case).
> The only exception I'd make is for the case of releasing a spinlock or
> LWLock; it's better to release the lock ASAP so as not to block other
> backends longer than necessary.
Agreed. However, I have never seen it stated that file descriptors are
freeded on elog(ERROR). I certainly didn't know that. If we are going
to remove them, let's do it systematically. Let's state in the
developers FAQ what elog(ERROR) frees automatically (file descriptors,
memory in most contexts, anything else?) and then I can check all the
elog(ERROR) cases and make sure this is done consistently.
--
Bruce Momjian | http://candle.pha.pa.us
pgman@candle.pha.pa.us | (610) 853-3000
+ If your life is a hard drive, | 830 Blythe Avenue
+ Christ can be your backup. | Drexel Hill, Pennsylvania 19026