Re: 2pc leaks fds

At Thu, 23 Apr 2020 19:16:03 -0400, Alvaro Herrera <alvherre@2ndquadrant.com> wrote in 
> On 2020-Apr-22, Andres Freund wrote:
> 
> > I'm in favor of doing so. Not necessarily primarily to avoid repeated
> > API changes, but because I don't think the v13 changes went in the quite
> > right direction.
> > 
> > ISTM that we should:
> > - have the three callbacks you mention above
> > - change WALSegmentOpen to also get the XLogReaderState
> > - add private state to WALOpenSegment, so it can be used even when not
> >   accessing data in files / when one needs more information to close the
> >   file.
> > - disambiguate between WALOpenSegment (struct describing an open
> >   segment) and WALSegmentOpen (callback to open a segment) (note that
> >   the read page callback uses a *CB naming, why not follow?)
> 
> 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).
> 
> Of the half a dozen cases that exist, three are slightly weird:
> 
> * Physical walsender does not use a xlogreader at all.  I think we could
>   beat that code up so that it does.  But for the moment I just cons up
>   a fake xlogreader, which only has the segment_open pointer set up, so
>   that it can call WALRead.
> 
> * main xlog.c uses an xlogreader with XLogPageRead(), which does not use
>   WALRead.  Therefore it does not pass open_segment.  It does not use
>   xlogreader->seg.ws_file either.  Eventually we may want to beat this
>   one up also.
> 
> * pg_rewind has its own page read callback, SimpleXLogPageRead, which
>   does all the required opening and closing.  I don't think it'd be an
>   improvement to force this to use segment_open.  Oddly enough, it calls
>   itself "simple" but is unique in having the ability to read files from
>   the wal archive.
> 
> All tests are passing for me.

I modestly object to such many call-back functions.  FWIW I'm writing
this with [1] in my mind.

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.

[1] https://www.postgresql.org/message-id/20200422.101246.331162888498679491.horikyota.ntt%40gmail.com

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center