Re: 2pc leaks fds

At Thu, 23 Apr 2020 19:16:03 -0400, Alvaro Herrera <alvherre(at)2ndquadrant(dot)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