Thread: Memory leak with CALL to Procedure with COMMIT.
Hi Hackers,
While testing with PG procedure, I found a memory leak on HEAD, with below steps:
postgres=# CREATE OR REPLACE PROCEDURE proc1(v1 INOUT INT)
AS $$
BEGIN
commit;
END; $$ LANGUAGE plpgsql;
CREATE PROCEDURE
postgres=# call proc1(10);
WARNING: Snapshot reference leak: Snapshot 0x23678e8 still referenced
v1
----
10
(1 row)
--
With Regards,
Prabhat Kumar Sahu
Skype ID: prabhat.sahu1984
EnterpriseDB Corporation
The Postgres Database Company
On Mon, Jul 23, 2018 at 12:19:12PM +0530, Prabhat Sahu wrote: > While testing with PG procedure, I found a memory leak on HEAD, with below > steps: > > postgres=# CREATE OR REPLACE PROCEDURE proc1(v1 INOUT INT) > AS $$ > BEGIN > commit; > END; $$ LANGUAGE plpgsql; > CREATE PROCEDURE > > postgres=# call proc1(10); > WARNING: Snapshot reference leak: Snapshot 0x23678e8 still referenced > v1 > ---- > 10 > (1 row) I can reproduce this issue on HEAD and v11, so an open item is added. Peter, could you look at it? -- Michael
Attachment
> On Jul 23, 2018, at 3:06 AM, Michael Paquier <michael@paquier.xyz> wrote:
>
> On Mon, Jul 23, 2018 at 12:19:12PM +0530, Prabhat Sahu wrote:
>> While testing with PG procedure, I found a memory leak on HEAD, with below
>> steps:
>>
>> postgres=# CREATE OR REPLACE PROCEDURE proc1(v1 INOUT INT)
>> AS $$
>> BEGIN
>> commit;
>> END; $$ LANGUAGE plpgsql;
>> CREATE PROCEDURE
>>
>> postgres=# call proc1(10);
>> WARNING: Snapshot reference leak: Snapshot 0x23678e8 still referenced
>> v1
>> ----
>> 10
>> (1 row)
>
> I can reproduce this issue on HEAD and v11, so an open item is added.
> Peter, could you look at it?
I tested and was able to reproduce this on head. I also tried a few other other
and was able to reproduce it when the procedure contained a few read-only
statements prior to commit, where the argument passed in was designated "INOUT."
Scenarios 1 & 2 show the leak whereas 3 & 4 do not.
/** Scenario 1: Original scenario */
CREATE OR REPLACE PROCEDURE proc1(v1 INOUT INT)
AS $$
BEGIN
commit;
END; $$ LANGUAGE plpgsql;
CALL proc1(10);
WARNING: Snapshot reference leak: Snapshot 0x7f9519826d18 still referenced
CONTEXT: PL/pgSQL function proc1(integer) line 3 at COMMIT
v1
----
10
(1 row)
/** Scenario 2: call "perform" prior to the commit */
CREATE OR REPLACE PROCEDURE proc2(v1 INOUT INT)
AS $$
BEGIN
PERFORM v1;
COMMIT;
END; $$ LANGUAGE plpgsql;
CALL proc2(10);
WARNING: Snapshot reference leak: Snapshot 0x7f9519826d18 still referenced
CONTEXT: PL/pgSQL function proc2(integer) line 4 at COMMIT
v1
----
10
(1 row)
/** Scenario 3: argument is only IN */
CREATE OR REPLACE PROCEDURE proc3(v1 IN INT)
AS $$
BEGIN
PERFORM v1;
COMMIT;
END; $$ LANGUAGE plpgsql;
CALL proc3(10);
CALL
/** Scenario 4: Same as #2 but with a ROLLBACK */
CREATE OR REPLACE PROCEDURE proc4(v1 INOUT INT)
AS $$
BEGIN
PERFORM v1;
ROLLBACK;
END; $$ LANGUAGE plpgsql;
CALL proc4(10);
CALL
Jonathan
Attachment
The commit that started this is
commit 59a85323d9d5927a852939fa93f09d142c72c91a
Author: Peter Eisentraut <peter_e@gmx.net>
Date: Mon Jul 9 13:58:08 2018
Add UtilityReturnsTuples() support for CALL
This ensures that prepared statements for CALL can return tuples.
The change whether a statement returns tuples or not causes some
different paths being taking deep in the portal code that set snapshot
in different ways. I haven't fully understood them yet. I plan to post
more tomorrow.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 14/08/2018 15:35, Peter Eisentraut wrote: > The commit that started this is > > commit 59a85323d9d5927a852939fa93f09d142c72c91a > Author: Peter Eisentraut <peter_e@gmx.net> > Date: Mon Jul 9 13:58:08 2018 > > Add UtilityReturnsTuples() support for CALL > > This ensures that prepared statements for CALL can return tuples. > > The change whether a statement returns tuples or not causes some > different paths being taking deep in the portal code that set snapshot > in different ways. I haven't fully understood them yet. The problem arises with the combination of CALL with output parameters and doing a COMMIT inside the procedure. When a CALL has output parameters, the portal uses the strategy PORTAL_UTIL_SELECT instead of PORTAL_MULTI_QUERY. Using PORTAL_UTIL_SELECT causes the portal's snapshot to be registered with the current resource owner (portal->holdSnapshot). I'm not sure why this is done for one kind of portal strategy but not the other. Then, when the COMMIT inside the procedure is run, the current resource owner is released and it complains that a snapshot is still registered there. There are, technically, three ways to fix this: silence the warning, unregister the snapshot explicitly, or don't register the snapshot to begin with. Silencing the warning, other than by just deleting it, might require passing in some more context information into ResourceOwnerRelease(). (The existing isTopLevel isn't quite the right thing.) Unregistering the snapshot explicitly looks tricky because in SPI_commit() or thereabouts we have no context information about which snapshot might have been registered where. Not registering the snapshot to begin with seems dubious, but see my question above about why this is not done in the case of PORTAL_MULTI_QUERY. Thoughts? -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 2018-Aug-16, Peter Eisentraut wrote: > There are, technically, three ways to fix this: silence the warning, > unregister the snapshot explicitly, or don't register the snapshot to > begin with. > > Silencing the warning, other than by just deleting it, might require > passing in some more context information into ResourceOwnerRelease(). > (The existing isTopLevel isn't quite the right thing.) > > Unregistering the snapshot explicitly looks tricky because in > SPI_commit() or thereabouts we have no context information about which > snapshot might have been registered where. Hmm, this got me thinking whether the current resource owner setup for a procedure is appropriate. Maybe the problem is that resowners are still thought of in terms of transactions plus portals, so that if transactions are done then everything is over; maybe we need to teach them that procedures can outlive transactions, so you'd have a resowner that's global to the procedure and then each transaction resowner is a child of that one? -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> Hmm, this got me thinking whether the current resource owner setup for a
> procedure is appropriate. Maybe the problem is that resowners are still
> thought of in terms of transactions plus portals, so that if
> transactions are done then everything is over; maybe we need to teach
> them that procedures can outlive transactions, so you'd have a resowner
> that's global to the procedure and then each transaction resowner is a
> child of that one?
The procedure still has to be running inside a query, and therefore
inside a portal, so the portal's resowner ought to be sufficiently
long-lived for any resources that ought to be procedure-lifetime.
So I doubt we need any more resowners. It's certainly possible that
something somewhere is assigning a particular resource to the wrong
resowner, of course.
regards, tom lane
Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
> The problem arises with the combination of CALL with output parameters
> and doing a COMMIT inside the procedure.
> When a CALL has output parameters, the portal uses the strategy
> PORTAL_UTIL_SELECT instead of PORTAL_MULTI_QUERY. Using
> PORTAL_UTIL_SELECT causes the portal's snapshot to be registered with
> the current resource owner (portal->holdSnapshot). I'm not sure why
> this is done for one kind of portal strategy but not the other.
I'm a bit confused by that statement. AFAICS, for both PortalRunUtility
and PortalRunMulti, setHoldSnapshot is only passed as true by
FillPortalStore, so registering the snapshot should happen (or not) the
same way for either portal execution strategy. What scenarios are you
comparing here, exactly?
In the long run where we want to think about allowing multiple rowsets to
be returned out of a procedure, it's fairly likely that PORTAL_UTIL_SELECT
isn't going to work anyway. Maybe we should be thinking about inventing a
different portal execution strategy for CALL. But I'm not sure we need
that just yet.
regards, tom lane
On 16/08/2018 19:26, Tom Lane wrote: >> When a CALL has output parameters, the portal uses the strategy >> PORTAL_UTIL_SELECT instead of PORTAL_MULTI_QUERY. Using >> PORTAL_UTIL_SELECT causes the portal's snapshot to be registered with >> the current resource owner (portal->holdSnapshot). I'm not sure why >> this is done for one kind of portal strategy but not the other. > I'm a bit confused by that statement. AFAICS, for both PortalRunUtility > and PortalRunMulti, setHoldSnapshot is only passed as true by > FillPortalStore, so registering the snapshot should happen (or not) the > same way for either portal execution strategy. What scenarios are you > comparing here, exactly? The call to PortalRunMulti() in FillPortalStore() is for PORTAL_ONE_RETURNING and PORTAL_ONE_MOD_WITH, not for PORTAL_MULTI_QUERY. For PORTAL_MULTI_QUERY, FillPortalStore() isn't called; PortalRun() calls PortalRunMulti() directly with setHoldSnapshot = false. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
I think I've found a reasonable fix for this. The problem arises with the combination of CALL with output parameters and doing a COMMIT inside the procedure. When a CALL has output parameters, the portal uses the strategy PORTAL_UTIL_SELECT instead of PORTAL_MULTI_QUERY. Using PORTAL_UTIL_SELECT causes the portal's snapshot to be registered with the current resource owner (portal->holdSnapshot); see 9ee1cf04ab6bcefe03a11837b53f29ca9dc24c7a for the reason. Normally, PortalDrop() unregisters the snapshot. If not, then ResourceOwnerRelease() will print a warning about a snapshot leak on transaction commit. A transaction commit normally drops all portals (PreCommit_Portals()), except the active portal. So in case of the active portal, we need to manually release the snapshot to avoid the warning. PreCommit_Portals() already contains some code that deals specially with the active portal versus resource owners, so it seems reasonable to add there. I think this problem could theoretically apply to any multiple-transaction utility command. For example, if we had a variant of VACUUM that returned tuples, it would produce the same warning. (This patch would fix that as well.) -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Attachment
> On Aug 23, 2018, at 9:34 AM, Peter Eisentraut <peter.eisentraut@2ndquadrant.com> wrote: > > I think I've found a reasonable fix for this. > > The problem arises with the combination of CALL with output parameters > and doing a COMMIT inside the procedure. When a CALL has output > parameters, the portal uses the strategy PORTAL_UTIL_SELECT instead of > PORTAL_MULTI_QUERY. Using PORTAL_UTIL_SELECT causes the portal's > snapshot to be registered with the current resource > owner (portal->holdSnapshot); see > 9ee1cf04ab6bcefe03a11837b53f29ca9dc24c7a for the reason. > > Normally, PortalDrop() unregisters the snapshot. If not, then > ResourceOwnerRelease() will print a warning about a snapshot leak on > transaction commit. A transaction commit normally drops all > portals (PreCommit_Portals()), except the active portal. So in case of > the active portal, we need to manually release the snapshot to avoid the > warning. > > PreCommit_Portals() already contains some code that deals specially with > the active portal versus resource owners, so it seems reasonable to add > there. > > I think this problem could theoretically apply to any > multiple-transaction utility command. For example, if we had a variant > of VACUUM that returned tuples, it would produce the same warning. > (This patch would fix that as well.) Applied the patch against head. make check passed, all the tests I ran earlier in this thread passed as well. Reviewed the code - looks to match above reasoning, and comments are clear. From my perspective it looks good, but happy to hear from someone more familiar than me with this area of the code. Thanks, Jonathan
Attachment
On 23/08/2018 17:35, Jonathan S. Katz wrote: > Applied the patch against head. make check passed, all the tests I ran > earlier in this thread passed as well. > > Reviewed the code - looks to match above reasoning, and comments > are clear. > > From my perspective it looks good, but happy to hear from someone > more familiar than me with this area of the code. committed -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
> On Aug 27, 2018, at 5:13 PM, Peter Eisentraut <peter.eisentraut@2ndquadrant.com> wrote: > > On 23/08/2018 17:35, Jonathan S. Katz wrote: >> Applied the patch against head. make check passed, all the tests I ran >> earlier in this thread passed as well. >> >> Reviewed the code - looks to match above reasoning, and comments >> are clear. >> >> From my perspective it looks good, but happy to hear from someone >> more familiar than me with this area of the code. > > committed Awesome, thank you! I’ve removed this from the Open Items list. Jonathan