Re: Patch pg_is_in_backup()

On Thu, Jun 14, 2012 at 1:48 PM, Gurjeet Singh <singh.gurjeet@gmail.com> wrote:
> On Thu, Jun 14, 2012 at 1:29 PM, Robert Haas <robertmhaas@gmail.com> wrote:
>>
>> On Thu, Jun 14, 2012 at 6:06 AM, Gabriele Bartolini
>> <gabriele.bartolini@2ndquadrant.it> wrote:
>> >    thank you very much for your patience (and thank you Marco for
>> > supporting
>> > me). I apologise for the delay.
>> >
>> >    I have retested the updated patch and it works fine with me. It is
>> > "ready
>> > for committer" for me.
>>
>> Committed.
>
>
> A minor gripe:
>
> +     /*
> +     * Close the backup label file.
> +     */
> +     if (ferror(lfp) || FreeFile(lfp)) {
> +            ereport(ERROR,
> +                (errcode_for_file_access(),
> +                errmsg("could not read file \"%s\": %m",
> +                             BACKUP_LABEL_FILE)));
> +     }
> +
>
> If ferror(lfp) returns false, wouldn't we miss the FreeFile() and leak a
> file pointer?

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
leakageif a routine* that calls AllocateFile is terminated prematurely by ereport(ERROR). 

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company