Horton

Thread

Commits

  1. Fix resource leak when a FDW's ForeignAsyncRequest function fails

  2. Use ResourceOwner to track WaitEventSets.

  1. WaitEventSet resource leakage

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-03-09T18:51:09Z

    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

  2. Re: WaitEventSet resource leakage

    Heikki Linnakangas <hlinnaka@iki.fi> — 2023-11-15T22:48:52Z

    (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)

  3. Re: WaitEventSet resource leakage

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-11-15T23:08:57Z

    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

  4. Re: WaitEventSet resource leakage

    Heikki Linnakangas <hlinnaka@iki.fi> — 2023-11-16T10:21:49Z

    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)

  5. Re: WaitEventSet resource leakage

    Thomas Munro <thomas.munro@gmail.com> — 2023-11-19T21:09:57Z

    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;

  6. Re: WaitEventSet resource leakage

    Alexander Law <exclusion@gmail.com> — 2023-11-22T13:00:00Z

    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

  7. Re: WaitEventSet resource leakage

    Heikki Linnakangas <hlinnaka@iki.fi> — 2023-11-23T11:35:16Z

    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)