Re: WAL segment file descriptor leak on read errors can PANIC the server

Hi,

> A failed WAL read leaks the open WAL segment file descriptor until the
> backend exits. Once the fd limit is reached, all later queries in that
> backend fail with "Too many open files". With a connection pooler the
> leaks add up across clients. There is no warning or log message for
> this. Reproducers are at [1] and [2].

I searched the archives and did not find any earlier report of this. It
should be fixed regardless.

> I think the fix is to register a memory context reset callback on the
> context the reader is allocated in. [...] Doing this in
> XLogReaderAllocate() covers every caller, present and future, instead
> of adding an error handler to each one.

I agree with the proposed fix.

Using your patch, I put Assert(state->seg.ws_file == -1) before the
close in the new callback and ran check-world on a cassert build. It
fires in two distinct places.

One is the logical decoding case, that is, errors mid-decode which means
XLogReaderFree() is never reached.

The other is the physical walsender, which brings me to this:

> The walsender does not have this issue because it closes the file in
> its own error cleanup.

This is not true in the normal exit case. StartReplication() never
calls XLogReaderFree(), so on a clean CopyDone the reader is discarded
with the segment file still open.

This is easy to see by logging in the new callback:

```
if (state->seg.ws_file != -1)
{
elog(LOG, "closing leaked WAL fd %d", state->seg.ws_file);
state->routine.segment_close(state);
}
```

Starting a pg_receivewal and stopping it with Ctrl-C, which ends
streaming cleanly rather than with an error:

```
LOG: closing leaked WAL fd 9
STATEMENT: START_REPLICATION SLOT "s1" 0/04000000 TIMELINE 1
```

So a segment file is still open when the reader goes away, on a path
with no error involved at all. Without your patch nothing closes it.

In terms of the patch, I was thinking about this allocation:

```
> + state->reset_cb = palloc_extended(sizeof(MemoryContextCallback),
> + MCXT_ALLOC_NO_OOM | MCXT_ALLOC_ZERO);
```

At first, I thought palloc_object() would do and be simpler, but keeping
MCXT_ALLOC_NO_OOM gives the better message, since callers report
errdetail("Failed while allocating a WAL reading processor."), and
keeping this consistent with the other allocations is probably best.
So I agree with what you have.

I do not see anything else that stands out in the patch.

--
Sami Imseih
Amazon Web Services (AWS)