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: <20200406054449.ssmrzj4fav3ewvaq@alap3.anarazel.de>
References: <20200406025651.fpzdb5yyb7qyhqko@alap3.anarazel.de>
 <20200406052648.GB2066@paquier.xyz>
 <20200406054449.ssmrzj4fav3ewvaq@alap3.anarazel.de>
Comments: In-reply-to Andres Freund <andres@anarazel.de>
   message dated "Sun, 05 Apr 2020 22:44:49 -0700."
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <26246.1586157152.1@antos>
Date: Mon, 06 Apr 2020 09:12:32 +0200
Message-ID: <26247.1586157152@antos>
Precedence: bulk

Andres Freund <andres@anarazel.de> wrote:

> > From what I can see, the error is that the code only bothers closing
> > WALOpenSegment->seg when switching to a new segment, but we need also
> > to close it when finishing the business in XLogReaderFree().
> 
> Yea, I came to the same conclusion and locally fixed it the same way
> (except having the close a bit earlier in XLogReaderFree()).

It's still not quite clear to me why the problem starts to appear after
0dc8ead46.  This patch does not remove any close() call from XLogReaderFree().

> But I'm not sure it's quite the right idea. I'm not sure I fully
> understand the design of 0dc8ead46, but it looks to me like it's
> intended to allow users of the interface to have different ways of
> opening files. If we just close() the fd that'd be a bit more limited.

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);

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