Re: 2pc leaks fds

Hi,

On 2020-04-06 14:26:48 +0900, Michael Paquier wrote:
> 2PC shines with the code of xlogreader.c in this case because it keeps
> opening and closing XLogReaderState for a short amount of time. So it
> is not surprising to me to see this error only months after the fact
> because recovery or pg_waldump just use one XLogReaderState.

Well, it doesn't exactly signal that people (including me, up to just
now) are testing their changes all that carefully...

> From what I can see, the error is that the code only bothers closing
> WALOpenSegment->seg when switching to a new segment, but we need also
> to close it when finishing the business in XLogReaderFree().

Yea, I came to the same conclusion and locally fixed it the same way
(except having the close a bit earlier in XLogReaderFree()).

> diff --git a/src/backend/access/transam/xlogreader.c b/src/backend/access/transam/xlogreader.c
> index f3fea5132f..7e25e2050a 100644
> --- a/src/backend/access/transam/xlogreader.c
> +++ b/src/backend/access/transam/xlogreader.c
> @@ -144,6 +144,9 @@ XLogReaderFree(XLogReaderState *state)
> if (state->main_data)
> pfree(state->main_data);
>
> + if (state->seg.ws_file >= 0)
> + close(state->seg.ws_file);
> +
> pfree(state->errormsg_buf);
> if (state->readRecordBuf)
> pfree(state->readRecordBuf);

But I'm not sure it's quite the right idea. I'm not sure I fully
understand the design of 0dc8ead46, but it looks to me like it's
intended to allow users of the interface to have different ways of
opening files. If we just close() the fd that'd be a bit more limited.

OTOH, I think all but one (XLogPageRead()) of the current users of
XLogReader use WALRead(), which also close()s the fd (before calling the
WALSegmentOpen callback).

The XLogReader code flow has gotten quite complicated
:(. XLogReaderReadRecord()-> state->read_page() ->
logical_read_xlog_page etc -> WALRead() -> wal_segment_open callback etc.

There's been a fair bit of change, making the interface more generic /
powerful / reducing duplication, but not a lot of added / adapted
comments in the header...

Greetings,

Andres Freund