From: Antonin Houska <ah@cybertec.at>
To: Andres Freund <andres@anarazel.de>
cc: Michael Paquier <michael@paquier.xyz>, pgsql-hackers@postgresql.org,
    Alvaro Herrera <alvherre@2ndquadrant.com>
Subject: Re: 2pc leaks fds
In-reply-to: <20200408001249.2ma2x6s54xcegiu2@alap3.anarazel.de>
References: <20200406025651.fpzdb5yyb7qyhqko@alap3.anarazel.de>
 <20200406052648.GB2066@paquier.xyz>
 <20200406054449.ssmrzj4fav3ewvaq@alap3.anarazel.de> <26247.1586157152@antos>
 <20200408001249.2ma2x6s54xcegiu2@alap3.anarazel.de>
Comments: In-reply-to Andres Freund <andres@anarazel.de>
   message dated "Tue, 07 Apr 2020 17:12:49 -0700."
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <70538.1586332821.1@antos>
Content-Transfer-Encoding: quoted-printable
Date: Wed, 08 Apr 2020 10:00:21 +0200
Message-ID: <70539.1586332821@antos>
Precedence: bulk

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 seg=
ment*
> > file. The WALSegmentOpen callback could actually return file path inst=
ead 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 f=
ile).
> > =

> > 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" w=
ould be
> > more suitable than "preferred"):
> > =

> >  * BasicOpenFile() is the preferred way to open the segment file in ba=
ckend
> >  * 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