Re: Facing a problem with SPI

Hi all,

I am facing a problem with the SPI (spi.c). I have implemented
short-lived virtual indexes using SubTransactions. The Index Adviser, when
creating virtual indexes, creates them inside a SubTransaction, and when it
is done with its advisory stuff, it rolls back the SubTransaction. This
achieves two objectives:

(1) Easy cleanup of dependencies/changes in the catalog.
(2) The virtual indexes are not visible to other backends. This
eliminates the possibility of other backends (not running in Advise mode)
seeing these indexes, as these don't have any data in them.

So all the index creation/deletion is done between a pair of
BeginInternalSubTransaction() (BIST()) and
RollbackAndReleaseCurrentSubTransaction() (RARCST()). This all works well
when we are EXPLAINing query in special advise mode.

Now, based on this, I wanted to generate advise for every query that
comes for planning, be it from psql or pl/pgsql or anywhere else. So I made
a function call to the index_adviser() from within pg_plan_query() after the
planner() call.

The problem I am facing is that that the queries being planned by
pl/pgsql, come after a call to SPI_connect() and SPI_Prepare().
SPI_prepare() switches to _SPI_current->execCxt memory context using
_SPI_begin_call().

Everything goes well until the index_adviser() calls RARCST() to
rollback it's SubTransaction. RARCST() ultimately ends up calling
AtEOSubXact_SPI(), which, just before returning, deletes the execution
memory context of the surrounding SPI (which happens to be pl/pgsql's SPI).
This, in effect, frees all the memory allocated by pl/pgsql for the prepare
stage; and the next time the pl/pgsql's data-structures are referenced, it
causes a crash. The code in question is:

void
AtEOSubXact_SPI(bool isCommit, SubTransactionId mySubid)
{
bool found = false;

while (_SPI_connected >= 0)
{
_SPI_connection *connection = &(_SPI_stack[_SPI_connected]);

if (connection->connectSubid != mySubid)
break; /* couldn't be any underneath it either */

......
......
/*
* If we are aborting a subtransaction and there is an open SPI context
* surrounding the subxact, clean up to prevent memory leakage.
*/
if (_SPI_current && !isCommit)
{
/* free Executor memory the same as _SPI_end_call would do */
MemoryContextResetAndDeleteChildren(_SPI_current->execCxt);
^^^^^^^^^^^
/* throw away any partially created tuple-table */
SPI_freetuptable(_SPI_current->tuptable);
_SPI_current->tuptable = NULL;
}
}

The code is doing what the comments say, but how I wish it didn't. This
code assumes that the latest BIST() came from a SPI invoking module, which
the Index Adviser is not!

I haven't been able to come up with a proof, but I think this situation
can be simulated/reproduced using some pl/* language that we support.

pl/pgsql is safe, since it doesn't allow SAVEPOINT/ROLLBACK TO SAVEPOINT
inside it's code blocks; but if some pl/xxx allows SAVEPOINTS, I think it
can be reproduced using something like:

SAVEPOINT xxx ( should call BIST() )
SAVEPOINT yyy ( should call BIST() )
Rollback (to yyy) ( should call RARCST() ) will free pl/xxx exec
context
Rollback (to xxx) ( should call RARCST() )

Probably just one pair, instead of two nested ones, should reproduce it.
I tried to reproduce it using pl/pgsql's exception block (there we use
BIST() and RARCST()); but couldn't succeed.

I hope I have understood the issue correctly, or have I missed
something? Is there a way to work around this?

Best regards,

--
gurjeet[(dot)singh](at)EnterpriseDB(dot)com
singh(dot)gurjeet(at){ gmail | hotmail | yahoo }.com

Just noticed that this was committed by Tom on 21/Nov with this log
message:

<snip>
Prevent intratransaction memory leak when a subtransaction is aborted
in the middle of executing a SPI query. This doesn't entirely fix the
problem of memory leakage in plpgsql exception handling, but it should
get rid of the lion's share of leakage.
</snip>

Can the memory leaks be avoided in some other way?

Best regards,

--
gurjeet[(dot)singh](at)EnterpriseDB(dot)com
singh(dot)gurjeet(at){ gmail | hotmail | yahoo }.com

On 12/3/06, Gurjeet Singh <singh(dot)gurjeet(at)gmail(dot)com> wrote:
>
> Hi all,
>
> I am facing a problem with the SPI (spi.c). I have implemented
> short-lived virtual indexes using SubTransactions. The Index Adviser, when
> creating virtual indexes, creates them inside a SubTransaction, and when it
> is done with its advisory stuff, it rolls back the SubTransaction. This
> achieves two objectives:
>
> (1) Easy cleanup of dependencies/changes in the catalog.
> (2) The virtual indexes are not visible to other backends. This
> eliminates the possibility of other backends (not running in Advise mode)
> seeing these indexes, as these don't have any data in them.
>
> So all the index creation/deletion is done between a pair of
> BeginInternalSubTransaction() (BIST()) and
> RollbackAndReleaseCurrentSubTransaction() (RARCST()). This all works well
> when we are EXPLAINing query in special advise mode.
>
> Now, based on this, I wanted to generate advise for every query that
> comes for planning, be it from psql or pl/pgsql or anywhere else. So I made
> a function call to the index_adviser() from within pg_plan_query() after the
> planner() call.
>
> The problem I am facing is that that the queries being planned by
> pl/pgsql, come after a call to SPI_connect() and SPI_Prepare().
> SPI_prepare() switches to _SPI_current->execCxt memory context using
> _SPI_begin_call().
>
> Everything goes well until the index_adviser() calls RARCST() to
> rollback it's SubTransaction. RARCST() ultimately ends up calling
> AtEOSubXact_SPI(), which, just before returning, deletes the execution
> memory context of the surrounding SPI (which happens to be pl/pgsql's SPI).
> This, in effect, frees all the memory allocated by pl/pgsql for the prepare
> stage; and the next time the pl/pgsql's data-structures are referenced, it
> causes a crash. The code in question is:
>
> void
> AtEOSubXact_SPI(bool isCommit, SubTransactionId mySubid)
> {
> bool found = false;
>
> while (_SPI_connected >= 0)
> {
> _SPI_connection *connection = &(_SPI_stack[_SPI_connected]);
>
> if (connection->connectSubid != mySubid)
> break; /* couldn't be any underneath it either */
>
> ......
> ......
> /*
> * If we are aborting a subtransaction and there is an open SPI
> context
> * surrounding the subxact, clean up to prevent memory leakage.
> */
> if (_SPI_current && !isCommit)
> {
> /* free Executor memory the same as _SPI_end_call would do */
> MemoryContextResetAndDeleteChildren(_SPI_current->execCxt);
> ^^^^^^^^^^^
> /* throw away any partially created tuple-table */
> SPI_freetuptable(_SPI_current->tuptable);
> _SPI_current->tuptable = NULL;
> }
> }
>
> The code is doing what the comments say, but how I wish it didn't.
> This code assumes that the latest BIST() came from a SPI invoking module,
> which the Index Adviser is not!
>
> I haven't been able to come up with a proof, but I think this
> situation can be simulated/reproduced using some pl/* language that we
> support.
>
> pl/pgsql is safe, since it doesn't allow SAVEPOINT/ROLLBACK TO
> SAVEPOINT inside it's code blocks; but if some pl/xxx allows SAVEPOINTS, I
> think it can be reproduced using something like:
>
> SAVEPOINT xxx ( should call BIST() )
> SAVEPOINT yyy ( should call BIST() )
> Rollback (to yyy) ( should call RARCST() ) will free pl/xxx
> exec context
> Rollback (to xxx) ( should call RARCST() )
>
> Probably just one pair, instead of two nested ones, should reproduce
> it. I tried to reproduce it using pl/pgsql's exception block (there we use
> BIST() and RARCST()); but couldn't succeed.
>
> I hope I have understood the issue correctly, or have I missed
> something? Is there a way to work around this?
>
> Best regards,
>
> --
> gurjeet[(dot)singh](at)EnterpriseDB(dot)com
> singh(dot)gurjeet(at){ gmail | hotmail | yahoo }.com

On Sun, 2006-12-03 at 17:55 +0530, Gurjeet Singh wrote:

> Can the memory leaks be avoided in some other way?

Possibly, but it's not likely to happen just to help you out here
unfortuantely.

There's another way, I'm sure

--
Simon Riggs
EnterpriseDB http://www.enterprisedb.com

On 12/4/06, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
>
> On Sun, 2006-12-03 at 17:55 +0530, Gurjeet Singh wrote:
>
> > Can the memory leaks be avoided in some other way?
>
> Possibly, but it's not likely to happen just to help you out here
> unfortuantely.

That's sad... well, I have devised a workaround, and I am sure that even
that isn't pretty enough to be liked by everyone.

There's another way, I'm sure
>
> --
> Simon Riggs
> EnterpriseDB http://www.enterprisedb.com
>
>
>

--
gurjeet[(dot)singh](at)EnterpriseDB(dot)com
singh(dot)gurjeet(at){ gmail | hotmail | yahoo }.com

Another question.

There are two global variables:

CurrentResourceOwner (CRO) and CurrentTransactionState (CTS). CTS has a
member curTransactionOwner(cTO). Now the question is:

is the following condition always supposed to be true?

CurrentResourceOwner==CurrentTransactionState->curTransactionOwner

While in executor, CRO->name is "Portal". Then I do the
BeginInternalSubTransaction() (BIST()); it pushes the current
(sub)transaction on a stack and and along with it, it sets the CTS as it's
parent. CTS->cTO->name is "TopTransaction". And a new resource-owner is
assigned to the new SubTransaction by AtSubStart_ResourceOwner().

After the adviser is done with it's work, it calls
RollbackAndReleaseCurrentSubTransaction() (RARCST()), which restores the
global CRO to the previously pushed SubTransaction's parent; this is
"TopTransaction". Now, we have lost record of which was the ResourceOwner
before BIST() and erroneously restored "TopTransaction" to be the CRO. This
is causing some diffcult to track ERROR conditions.

So, is the above mentioned condition supposed to hold always? (I am
assuming not). If not, then we should remember the CurrentResourceOwner
across BIST() and RARCST() calls!

Sorry to be pestering you guys with seemingly trivial stuff, but you
would also agree that it becomes very difficult to understand the code when
there are so many globals and the relationship between them is not very
clear.

Regards,

--
gurjeet[(dot)singh](at)EnterpriseDB(dot)com
singh(dot)gurjeet(at){ gmail | hotmail | yahoo }.com

"Gurjeet Singh" <singh(dot)gurjeet(at)gmail(dot)com> writes:
> is the following condition always supposed to be true?
> CurrentResourceOwner==CurrentTransactionState->curTransactionOwner

No.

> If not, then we should remember the CurrentResourceOwner
> across BIST() and RARCST() calls!

As indeed the current callers of them do...

regards, tom lane

On 12/4/06, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>
> > we should remember the CurrentResourceOwner
> > across BIST() and RARCST() calls!
>
> As indeed the current callers of them do...
>

Thanks. I missed that piece of code in pl/pgsql exception handling!!!

So, if I use kludge the code like:

oldResourceOwner = CurrentResourceOwner;
BIST();
... do stuff ...
RARCST();
CurrentResourceOwner = oldResourceOwner;

it would be a standard way of doing it.

Best regards,

--
gurjeet[(dot)singh](at)EnterpriseDB(dot)com
singh(dot)gurjeet(at){ gmail | hotmail | yahoo }.com