Re: 2pc leaks fds

Andres Freund <andres@anarazel.de> wrote:

> On 2020-04-06 09:12:32 +0200, Antonin Houska wrote:
> > Andres Freund <andres@anarazel.de> wrote:
> > It should have allowed users to have different ways to *locate the segment*
> > file. The WALSegmentOpen callback could actually return file path instead of
> > the file descriptor and let WALRead() perform the opening/closing, but then
> > the WALRead function would need to be aware whether it is executing in backend
> > or in frontend (so it can use the correct function to open/close the file).
> > 
> > I was aware of the problem that the correct function should be used to open
> > the file and that's why this comment was added (although "mandatory" would be
> > more suitable than "preferred"):
> > 
> >  * BasicOpenFile() is the preferred way to open the segment file in backend
> >  * code, whereas open(2) should be used in frontend.
> >  */
> > typedef int (*WALSegmentOpen) (XLogSegNo nextSegNo, WALSegmentContext *segcxt,
> > 							   TimeLineID *tli_p);
> 
> I don't think that BasicOpenFile() really solves anything here? If
> anything it *exascerbates* the problem, because it will trigger all of
> the "virtual file descriptors" for already opened Files to close() the
> underlying OS FDs.  So not even a fully cached table can be seqscanned,
> because that tries to check the file size...

Specifically for 2PC, isn't it better to close some OS-level FD of an
unrelated table scan and then succeed than to ERROR immediately? Anyway,
0dc8ead46 hasn't changed this.

I at least admit that the comment should not recommend particular function,
and that WALRead() should call the appropriate function to close the file,
rather than always calling close().

Can we just pass the existing FD to the callback as an additional argument,
and let the callback close it?

-- 
Antonin Houska
Web: https://www.cybertec-postgresql.com