Re: Plugging fd leaks (was Re: Switching timeline over streaming replication)

On 26.11.2012 14:53, Amit Kapila wrote:
> On Friday, November 23, 2012 7:03 PM Heikki Linnakangas
>> 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));

Thanks. Added that and committed.

I didn't dare to backpatch this, even though it could be fairly easily
backpatched. The leaks exist in older versions too, but since they're
extremely rare (zero complaints from the field and it's been like that
forever), I didn't want to take the risk. Maybe later, after this has
had more testing in master.

>> 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 ended up calling the functions OpenTransientFile and
CloseTransientFile. Windows has a library function called "OpenFile", so
that was a pretty bad choice after all.

- Heikki