Date: Fri, 24 Apr 2020 11:48:46 -0400
From: Alvaro Herrera <alvherre@2ndquadrant.com>
To: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Cc: andres@anarazel.de, ah@cybertec.at, michael@paquier.xyz,
	pgsql-hackers@postgresql.org
Subject: Re: 2pc leaks fds
Message-ID: <20200424154846.GA28188@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20200424.153651.401853964209886273.horikyota.ntt@gmail.com>
User-Agent: Mutt/1.10.1 (2018-07-13)
Precedence: bulk

On 2020-Apr-24, Kyotaro Horiguchi wrote:

> At Thu, 23 Apr 2020 19:16:03 -0400, Alvaro Herrera <alvherre@2ndquadrant.com> wrote in 

> > Here's a first attempt at that.  The segment_open/close callbacks are
> > now given at XLogReaderAllocate time, and are passed the XLogReaderState
> > pointer.  I wrote a comment to explain that the page_read callback can
> > use WALRead() if it wishes to do so; but if it does, then segment_open
> > has to be provided.  segment_close is mandatory (since we call it at
> > XLogReaderFree).

> I modestly object to such many call-back functions.  FWIW I'm writing
> this with [1] in my mind.
> [1] https://www.postgresql.org/message-id/20200422.101246.331162888498679491.horikyota.ntt%40gmail.com

Hmm.  Looking at your 0001, I think there's nothing in that patch that's
not compatible with my proposed API change.

0002 is a completely different story of course; but that patch is a
radical change of spirit for xlogreader, in the sense that it's no
longer a "reader", but rather just an interpreter of bytes from a WAL
byte sequence into WAL records; and shifts the responsibility of the
actual reading to the caller.  That's why xlogreader no longer receives
the page_read callback (and why it doesn't need the segment_open,
segment_close callbacks).

I have to admit that until today I hadn't realized that that's what your
patch series was doing.  I'm not familiar with how you intend to
implement WAL encryption on top of this, but on first blush I'm not
liking this proposed design too much.

> An open-callback is bound to a read-callback. A close-callback is
> bound to the way the read-callback opens a segment (or the
> open-callback).  I'm afraid that only adding "cleanup" callback might
> be sufficient.

Well, the complaint is that the current layering is weird, in that there
are two levels at which we pass callbacks: one is XLogReaderAllocate,
where you specify the page_read callback; and the other layer is inside
the page_read callback, if that layer uses the WALRead auxiliary
function.  The thing that my patch is doing is pass all three callbacks
at the XLogReaderAllocate level.  So when xlogreader drills down to
read_page, xlogreader already has the segment_open callback handy if it
needs it.  Conceptually, this seems more sensible.

I think a "cleanup" callback might also be sensible in general terms,
but we have a problem with the specifics -- namely that the "state" that
we need to clean up (the file descriptor of the open segment) is part of
xlogreader's state.  And we obviously cannot remove the FD from
XLogReaderState, because when we need the FD to do things with it to
obtain data from the file.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services