Re: Plugging fd leaks (was Re: Switching timeline over streaming replication) - Mailing list pgsql-hackers

On Friday, November 23, 2012 7:03 PM Heikki Linnakangas
> On 15.11.2012 17:16, Heikki Linnakangas wrote:
> > On 15.11.2012 16:55, Tom Lane wrote:
> >> Heikki Linnakangas<hlinnakangas@vmware.com> writes:
> >>> This is a fairly general issue, actually. Looking around, I can see
> >>> at least two similar cases in existing code, with BasicOpenFile,
> >>> where we will leak file descriptors on error:
> >>
> >> Um, don't we automatically clean those up during transaction abort?
> >
> > Not the ones allocated with PathNameOpenFile or BasicOpenFile. Files
> > allocated with AllocateFile() and OpenTemporaryFile() are cleaned up
> > at abort.
> >
> >> If we don't, we ought to think about that, not about cluttering
> >> calling code with certain-to-be-inadequate cleanup in error cases.
> >
> > Agreed. Cleaning up at end-of-xact won't help walsender or other
> > non-backend processes, though, because they don't do transactions. But
> > a top-level ResourceOwner that's reset in the sigsetjmp() cleanup
> > routine would work.
> 
> This is what I came up with. It adds a new function, OpenFile, that
> returns a raw file descriptor like BasicOpenFile, but the file
> descriptor is associated with the current subtransaction and
> automatically closed at end-of-xact, in the same way that AllocateFile
> and AllocateDir work. In other words, OpenFile is to open() what
> AllocateFile is to fopen(). BasicOpenFile is unchanged, it returns a raw
> fd and it's solely the caller's responsibility to close it, but many of
> the places that used to call BasicOpenFile now use the safer OpenFile
> function instead.
> 
> This patch plugs three existing fd (or virtual fd) leaks:
> 
> 1. copy_file() - fixed by by using OpenFile instead of BasicOpenFile 2.
> XLogFileLinit() - fixed by adding close() calls to the error cases.
> Can't use OpenFile here because the fd is supposed to persist over
> transaction boundaries.
> 3. lo_import/lo_export - fixed by using OpenFile instead of
> PathNameOpenFile.

I have gone through the patch and find it okay except for one minor
suggestion
1. Can we put below log in OpenFile as well 
+        DO_DB(elog(LOG, "CloseFile: Allocated %d", numAllocatedDescs));
> One thing I'm not too fond of is the naming. I'm calling the new
> functions OpenFile and CloseFile. There's some danger of confusion
> there, as the function to close a virtual file opened with
> PathNameOpenFile is called FileClose. OpenFile is really the same kind
> of operation as AllocateFile and AllocateDir, but returns an unbuffered
> fd. So it would be nice if it was called AllocateSomething, too. But
> AllocateFile is already taken. And I don't much like the Allocate*
> naming for these anyway, you really would expect the name to contain
> "open".

OpenFileInTrans
OpenTransactionAwareFile

In anycase OpenFile is also okay.


I have one usecase in feature (SQL Command to edit postgresql.conf) very
similar to OpenFile/CloseFile, but I want that when CloseFile is called from
abort, it should remove(unlink) the file as well and during open it has to
retry few times if open is not success.
I have following options: 
1. Extend OpenFile/CloseFile or PathNameOpenFile
2. Write new functions similar to OpenFile/CloseFile, something like
OpenConfLockFile/CloseConfLockFile
3. Use OpenFile/CloseFile  and handle my specific case with PG_TRY ..
PG_CATCH

Any suggestions?

With Regards,
Amit Kapila.