Re: 2pc leaks fds

At Fri, 24 Apr 2020 11:48:46 -0400, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com> wrote in
> On 2020-Apr-24, Kyotaro Horiguchi wrote:
>
> > At Thu, 23 Apr 2020 19:16:03 -0400, Alvaro Herrera <alvherre(at)2ndquadrant(dot)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).

Sorry for the ambiguity, I didn't meant I minded that this conflicts
with my patch or I don't want this to be committed. It is easily
rebased on this patch. What I was anxious about is that the new
callback struct might be too flexible than required. So I "mildly"
objected, and I won't be dissapointed if this patch is committed.

> 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.

Right. I might be too much in detail, but it simplifies the call
tree. Anyway that is another discussion, though:)

> > 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.

It looks like as if the open/read/close-callbacks are generic and on
the same interface layer, but actually open-callback is dedicate to
WALRead and it is useless when the read-callback doesn't use
WALRead. What I was anxious about is that the open-callback is
uselessly exposing the secret of the read-callback.

> 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.

I meant concretely that we only have read- and cleanup- callbacks in
xlogreader state. The caller of XLogReaderAllocate specifies the
cleanup-callback that is to be used to clean up what the
reader-callback left behind, in the same manner with this patch does.
The only reason it is not named close-callback is that it is used only
when the xlogreader-state is destroyed. So I'm fine with
read/close-callbacks.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center