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<hlinnakan...@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. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
