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@postgresql.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)

Heikki Linnakangas <hlinnaka@iki.fi> writes:

On 09/03/2023 20:51, Tom Lane wrote:

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.

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.

WFM. (I didn't study your back-branch patch.)

regards, tom lane

On 16/11/2023 01:08, Tom Lane wrote:

Heikki Linnakangas <hlinnaka@iki.fi> writes:

On 09/03/2023 20:51, Tom Lane wrote:

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.

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.

WFM. (I didn't study your back-branch patch.)

And here is a patch to implement that on master.

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

On Fri, Nov 17, 2023 at 12:22 AM Heikki Linnakangas <hlinnaka@iki.fi> wrote:

On 16/11/2023 01:08, Tom Lane wrote:

Heikki Linnakangas <hlinnaka@iki.fi> writes:

On 09/03/2023 20:51, Tom Lane wrote:

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.

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.

WFM. (I didn't study your back-branch patch.)

And here is a patch to implement that on master.

Rationale and code look good to me.

cfbot warns about WAIT_USE_WIN32:

[10:12:54.375] latch.c:889:2: error: ISO C90 forbids mixed
declarations and code [-Werror=declaration-after-statement]

Let's see...

WaitEvent *cur_event;

for (cur_event = set->events;

Maybe:

for (WaitEvent *cur_event = set->events;

20.11.2023 00:09, Thomas Munro wrote:

On Fri, Nov 17, 2023 at 12:22 AM Heikki Linnakangas <hlinnaka@iki.fi> wrote:

And here is a patch to implement that on master.

Rationale and code look good to me.

I can also confirm that the patches proposed (for master and back branches)
eliminate WES leakage as expected.

Thanks for the fix!

Maybe you would find appropriate to add the comment
/* Convenience wrappers over ResourceOwnerRemember/Forget */
above ResourceOwnerRememberWaitEventSet
just as it's added above ResourceOwnerRememberRelationRef,
ResourceOwnerRememberDSM, ResourceOwnerRememberFile, ...

(As a side note, this fix doesn't resolve the issue #17828 completely,
because that large number of handles might be also consumed
legally.)

Best regards,
Alexander

On 22/11/2023 15:00, Alexander Lakhin wrote:

I can also confirm that the patches proposed (for master and back branches)
eliminate WES leakage as expected.

Thanks for the fix!

Maybe you would find appropriate to add the comment
/* Convenience wrappers over ResourceOwnerRemember/Forget */
above ResourceOwnerRememberWaitEventSet
just as it's added above ResourceOwnerRememberRelationRef,
ResourceOwnerRememberDSM, ResourceOwnerRememberFile, ...

Added that and fixed the Windows warning that Thomas pointed out. Pushed
the ResourceOwner version to master, and PG_TRY-CATCH version to 14-16.

Thank you!

(As a side note, this fix doesn't resolve the issue #17828 completely,
because that large number of handles might be also consumed
legally.)

:-(

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