Horton

Thread

Commits

  1. Fix snapshot reference leak if lo_export fails.

  1. Snapshot leak warning with lo_export in subtransaction

    Heikki Linnakangas <hlinnaka@iki.fi> — 2021-09-27T10:55:00Z

    Hi,

Andrew B reported this warning off-list:

postgres=# SELECT lo_create(41174);
  lo_create
-----------
      41174
(1 row)

postgres=# DO $$
BEGIN
   PERFORM lo_export(41174, '/invalid/path');
EXCEPTION
   WHEN others THEN RAISE NOTICE 'error: %', sqlerrm;
END;
$$;
NOTICE:  error: could not create server file "/invalid/path": No such 
file or directory
WARNING:  Snapshot reference leak: Snapshot 0x5634afd61cb8 still referenced
DO

The code in be_lo_export does this:

> 
> 	CreateFSContext();
> 
> 	/*
> 	 * open the inversion object (no need to test for failure)
> 	 */
> 	lobj = inv_open(lobjId, INV_READ, fscxt);
> 
> 	/*
> 	 * open the file to be written to
> 	 *
> 	...

And inv_open does this:

>     /* OK to create a descriptor */
>     retval = (LargeObjectDesc *) MemoryContextAlloc(mcxt,
> 													sizeof(LargeObjectDesc));
>     retval->id = lobjId;
>     retval->subid = GetCurrentSubTransactionId();
>     retval->offset = 0;
>     retval->flags = descflags;
> 
>     /*
>      * We must register the snapshot in TopTransaction's resowner, because it
>      * must stay alive until the LO is closed rather than until the current
>      * portal shuts down.  Do this last to avoid uselessly leaking the
>      * snapshot if an error is thrown above.
>      */
>     if (snapshot)
>         snapshot = RegisterSnapshotOnOwner(snapshot,
>                                            TopTransactionResourceOwner);
>     retval->snapshot = snapshot;

So this is pretty clear-cut: if opening the file fails, the snapshot 
reference is leaked in TopTransactionResourceOwner. Similarly, the 
LargeObjectDesc is leaked in 'fscxt', which is a subcontext of 
TopMemoryContext, but that doesn't generate a warning.

I propose the attached patch to fix that. With the patch, we use 
CurrentMemoryContext and no resource owner for transient 
LargeObjectDescs that are opened and closed in the same function call.

This should be backpatched to all supported versions. This adds an 
argument to 'inv_open' function, but I don't think there are extensions 
that use the inv_*() functions directly. inv_api.c relies on 
close_lo_relation() being called at the end of transaction, so I think 
an extension would find it hard to use those functions correctly, anyway.

- Heikki

  2. Re: Snapshot leak warning with lo_export in subtransaction

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2021-09-27T12:33:23Z

    On 2021-Sep-27, Heikki Linnakangas wrote:

> This should be backpatched to all supported versions. This adds an argument
> to 'inv_open' function, but I don't think there are extensions that use the
> inv_*() functions directly. inv_api.c relies on close_lo_relation() being
> called at the end of transaction, so I think an extension would find it hard
> to use those functions correctly, anyway.

Hmm, but you could also use a new value in its existing 'flags' argument
instead of changing ABI.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
Voy a acabar con todos los humanos / con los humanos yo acabaré
voy a acabar con todos (bis) / con todos los humanos acabaré ¡acabaré! (Bender)

  3. Re: Snapshot leak warning with lo_export in subtransaction

    Heikki Linnakangas <hlinnaka@iki.fi> — 2021-10-18T09:59:56Z

    On 27/09/2021 15:33, Alvaro Herrera wrote:
> On 2021-Sep-27, Heikki Linnakangas wrote:
> 
>> This should be backpatched to all supported versions. This adds an argument
>> to 'inv_open' function, but I don't think there are extensions that use the
>> inv_*() functions directly. inv_api.c relies on close_lo_relation() being
>> called at the end of transaction, so I think an extension would find it hard
>> to use those functions correctly, anyway.
> 
> Hmm, but you could also use a new value in its existing 'flags' argument
> instead of changing ABI.

I tried that, but didn't like the result. It conflated the user-visible 
INV_READ/WRITE flags with the new internal-only flag.

Thinking about this some more, I came up with the attached. It moves the 
responsibility of registering the snapshot from inv_api.c to the caller. 
With that change, there's no need for a new option to inv_open(). The 
division of labor between be-fsstubs.c and inv_api.c has always been a 
bit blurry, I think that this makes it slightly more clear.

With this change, if there is an existing extension out there that calls 
inv_open(), the returned desc will be short-lived. As I said above, I 
think that's a reasonable assumption because dealing with a long-lived 
LO desc correctly would require the caller to jump through some hoops 
even before this change.

- Heikki

  4. Re: Snapshot leak warning with lo_export in subtransaction

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2021-10-18T14:40:35Z

    On 2021-Oct-18, Heikki Linnakangas wrote:

> I tried that, but didn't like the result. It conflated the user-visible
> INV_READ/WRITE flags with the new internal-only flag.

True -- I also noticed this and had written in my earlier reply that we
could perhaps define this flag internally, not exposing it to the user.
But that seemed weird from the API definition perspective, so I removed
that phrase before sending.

> Thinking about this some more, I came up with the attached. It moves the
> responsibility of registering the snapshot from inv_api.c to the caller.

I like this patch much better.

> With that change, there's no need for a new option to inv_open(). The
> division of labor between be-fsstubs.c and inv_api.c has always been a bit
> blurry, I think that this makes it slightly more clear.

Agreed.

-- 
Álvaro Herrera              Valdivia, Chile  —  https://www.EnterpriseDB.com/

  5. Re: Snapshot leak warning with lo_export in subtransaction

    Heikki Linnakangas <hlinnaka@iki.fi> — 2021-11-03T10:39:18Z

    On 18/10/2021 17:40, Alvaro Herrera wrote:
> On 2021-Oct-18, Heikki Linnakangas wrote:
>> Thinking about this some more, I came up with the attached. It moves the
>> responsibility of registering the snapshot from inv_api.c to the caller.
> 
> I like this patch much better.

Pushed! Thanks for the review, and thanks for the report, Andrew.

- Heikki