On Thu, Jun 14, 2012 at 3:10 PM, Gurjeet Singh <singh.gurjeet@gmail.com> wrote:
>> Well, according to the comments for AllocateFile:
>>
>> * fd.c will automatically close all files opened with AllocateFile at
>> * transaction commit or abort; this prevents FD leakage if a routine
>> * that calls AllocateFile is terminated prematurely by ereport(ERROR).
>
> I bet anyone else looking at this code, who is not in the know, will trip
> over this again.
>
> Another problem with that code block is that it will throw "could not read"
> even though read has succeeded but FreeFile() failed.
>
> I say we break it into two blocks, one to handle read error, and then close
> the file separately. Also, either make sure FreeFile() is called in all code
> paths, or not call FreeFile() at all and reference to the comment above
> AllocateFile().
>
> Patch attached.
I agree with breaking it into two blocks, but I don't agree that the
comments need to recapitulate how AllocateFile works. Also, you had
the same primary content for both comments, and you incorrectly
reversed the sense of the FreeFile() test.
Committed with those changes.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company