Re: Patch pg_is_in_backup()

On Thu, Jun 14, 2012 at 2:02 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:

> On Thu, Jun 14, 2012 at 1:48 PM, Gurjeet Singh <singh(dot)gurjeet(at)gmail(dot)com>
> wrote:
> > On Thu, Jun 14, 2012 at 1:29 PM, Robert Haas <robertmhaas(at)gmail(dot)com>
> wrote:
> >> 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 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.

Regards,
--
Gurjeet Singh
EnterpriseDB Corporation
The Enterprise PostgreSQL Company