Re: 2pc leaks fds

I have tested with and without the commit from Andres using the pgbench
script (below) provided in the initial email.

pgbench -n -s 500 -c 4 -j 4 -T 100000 -P1 -f pgbench-write-2pc.sql

I am not getting the leak anymore, it seems to be holding up pretty well.

On Wed, Apr 8, 2020 at 12:59 PM Antonin Houska <ah(at)cybertec(dot)at> wrote:

> Andres Freund <andres(at)anarazel(dot)de> wrote:
>
> > On 2020-04-06 09:12:32 +0200, Antonin Houska wrote:
> > > Andres Freund <andres(at)anarazel(dot)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
>
>
>

--
Highgo Software (Canada/China/Pakistan)
URL : http://www.highgo.ca
ADDR: 10318 WHALLEY BLVD, Surrey, BC
EMAIL: mailto: ahsan(dot)hadi(at)highgo(dot)ca