Re: Memory leak with CALL to Procedure with COMMIT.

From: Peter Eisentraut <peter(dot)eisentraut(at)2ndquadrant(dot)com>
To: "Jonathan S(dot) Katz" <jkatz(at)postgresql(dot)org>, Michael Paquier <michael(at)paquier(dot)xyz>
Cc: Prabhat Sahu <prabhat(dot)sahu(at)enterprisedb(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: Memory leak with CALL to Procedure with COMMIT.
Date: 2018-08-16 15:39:26
Message-ID: 2daf4ac0-3376-673c-3858-2fc191a010b9@2ndquadrant.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

On 14/08/2018 15:35, Peter Eisentraut wrote:
> The commit that started this is
>
> commit 59a85323d9d5927a852939fa93f09d142c72c91a
> Author: Peter Eisentraut <peter_e(at)gmx(dot)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

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Tom Lane 2018-08-16 15:41:30 Re: C99 compliance for src/port/snprintf.c
Previous Message Robert Haas 2018-08-16 15:36:58 Re: A slightly misleading comment in GetNewObjectId()