Thread: WaitEventSet resource leakage
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.
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.
regards, tom lane
[1] https://www.postgresql.org/message-id/423731.1678381075%40sss.pgh.pa.us
(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)
Attachment
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)
Attachment
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)