Re: Patch to implement pg_current_logfile() function

"Karl O. Pinc" <kop(at)meme(dot)com> writes:
> On Sat, 4 Mar 2017 14:26:54 +0530
> Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>> On Fri, Mar 3, 2017 at 7:21 PM, Karl O. Pinc <kop(at)meme(dot)com> wrote:
>>> But if the code does not exit the loop then
>>> before calling elog(ERROR), shouldn't it
>>> also call FreeFile(fd)?

>> Hmm. Normally error recovery cleans up opened files, but since
>> logfile_open() directly calls fopen(), that's not going to work here.
>> So that does seem to be a problem.

> Should something different be done to open the file or is it
> enough to call FreeFile(fd) to clean up properly?

I would say that in at least 99.44% of cases, if you think you need to
do some cleanup action immediately before an elog(ERROR), that means
You're Doing It Wrong. That can only be a safe coding pattern in a
segment of code in which no called function does, *or ever will*, throw
elog(ERROR) itself. Straight-line code with no reason ever to call
anything else might qualify, but otherwise you're taking too much risk
of current or future breakage. You need some mechanism that will ensure
cleanup after any elog call anywhere, and the backend environment offers
lots of such mechanisms.

Without having actually looked at this patch, I would say that if it added
a direct call of fopen() to backend-side code, that was already the wrong
thing. Almost always, AllocateFile() would be a better choice, not only
because it's tied into transaction abort, but also because it knows how to
release virtual FDs in event of ENFILE/EMFILE errors. If there is some
convincing reason why you shouldn't use AllocateFile(), then a safe
cleanup pattern would be to have the fclose() in a PG_CATCH stanza.

(FWIW, I don't particularly agree with Michael's objection to "break"
after elog(ERROR). Our traditional coding style is to write such things
anyway, so as to avoid drawing complaints from compilers that don't know
that elog(ERROR) doesn't return. You could argue that that's an obsolete
reason, but I don't think I want to see it done some places and not
others. Consistency of coding style is a good thing.)

regards, tom lane