Re: 2pc leaks fds

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.

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