Re: WaitEventSet resource leakage

(Alexander just reminded me of this off-list)

On 09/03/2023 20:51, Tom Lane wrote:
> In [1] I wrote:
>
>> PG Bug reporting form <noreply(at)postgresql(dot)org> writes:
>>> The following script:
>>> [ leaks a file descriptor per error ]
>>
>> Yeah, at least on platforms where WaitEventSets own kernel file
>> descriptors. I don't think it's postgres_fdw's fault though,
>> but that of ExecAppendAsyncEventWait, which is ignoring the
>> possibility of failing partway through. It looks like it'd be
>> sufficient to add a PG_CATCH or PG_FINALLY block there to make
>> sure the WaitEventSet is disposed of properly --- fortunately,
>> it doesn't need to have any longer lifespan than that one
>> function.

Here's a patch to do that. For back branches.

> After further thought that seems like a pretty ad-hoc solution.
> We probably can do no better in the back branches, but shouldn't
> we start treating WaitEventSets as ResourceOwner-managed resources?
> Otherwise, transient WaitEventSets are going to be a permanent
> source of headaches.

Agreed. The current signature of CurrentWaitEventSet is:

WaitEventSet *
CreateWaitEventSet(MemoryContext context, int nevents)

Passing MemoryContext makes little sense when the WaitEventSet also
holds file descriptors. With anything other than TopMemoryContext, you
need to arrange for proper cleanup with PG_TRY-PG_CATCH or by avoiding
ereport() calls. And once you've arrange for cleanup, the memory context
doesn't matter much anymore.

Let's change it so that it's always allocated in TopMemoryContext, but
pass a ResourceOwner instead:

WaitEventSet *
CreateWaitEventSet(ResourceOwner owner, int nevents)

And use owner == NULL to mean session lifetime.

--
Heikki Linnakangas
Neon (https://neon.tech)