Обсуждение: Facing a problem with SPI

<span style="font-family: courier new,monospace;">Hi all,<br /><br />    I am facing a problem with the SPI (spi.c). I
haveimplemented short-lived virtual indexes using SubTransactions. The Index Adviser, when creating virtual indexes,
createsthem inside a SubTransaction, and when it is done with its advisory stuff, it rolls back the SubTransaction.
Thisachieves two objectives: <br /><br />    (1) Easy cleanup of dependencies/changes in the catalog.<br />    (2) The
virtualindexes are not visible to other backends. This eliminates the possibility of other backends (not running in
Advisemode) seeing these indexes, as these don't have any data in them. <br /><br />    So all the index
creation/deletionis done between a pair of BeginInternalSubTransaction() (BIST()) and
RollbackAndReleaseCurrentSubTransaction()(RARCST()). This all works well when we are EXPLAINing query in special advise
mode.<br /><br />    Now, based on this, I wanted to generate advise for every query that comes for planning, be it
frompsql or pl/pgsql or anywhere else. So I made a function call to the index_adviser() from within pg_plan_query()
afterthe planner() call. <br /><br />    The problem I am facing is that that the queries being planned by pl/pgsql,
comeafter a call to SPI_connect() and SPI_Prepare(). SPI_prepare() switches to _SPI_current->execCxt memory context
using_SPI_begin_call(). <br /><br />    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
executionmemory context of the surrounding SPI (which happens to be pl/pgsql's SPI). This, in effect, frees all the
memoryallocated by pl/pgsql for the prepare stage; and the next time the pl/pgsql's data-structures are referenced, it
causesa crash. The code in question is: <br /><br />void<br />AtEOSubXact_SPI(bool isCommit, SubTransactionId
mySubid)<br/>{<br />    bool        found = false;<br /><br />    while (_SPI_connected >= 0)<br />    {<br
/>       _SPI_connection *connection = &(_SPI_stack[_SPI_connected]); <br /><br />        if
(connection->connectSubid!= mySubid)<br />            break;              /* couldn't be any underneath it either
*/<br/><br />        ......<br />        ......<br />    /*<br />     * If we are aborting a subtransaction and there
isan open SPI context <br />     * surrounding the subxact, clean up to prevent memory leakage.<br />     */<br />   
if(_SPI_current && !isCommit)<br />    {<br />        /* free Executor memory the same as _SPI_end_call would
do*/<br />        MemoryContextResetAndDeleteChildren(_SPI_current->execCxt); <br />        ^^^^^^^^^^^<br />       
/*throw away any partially created tuple-table */<br />        SPI_freetuptable(_SPI_current->tuptable);<br
/>       _SPI_current->tuptable = NULL;<br />    }<br />}<br /><br />    The code is doing what the comments say,
buthow I wish it didn't. This code assumes that the latest BIST() came from a SPI invoking module, which the Index
Adviseris not! <br /><br />    I haven't been able to come up with a proof, but I think this situation can be
simulated/reproducedusing some pl/* language that we support.<br /><br />    pl/pgsql is safe, since it doesn't allow
SAVEPOINT/ROLLBACKTO SAVEPOINT inside it's code blocks; but if some pl/xxx allows SAVEPOINTS, I think it can be
reproducedusing something like: <br /><br />    SAVEPOINT xxx            ( should call BIST() )<br />        SAVEPOINT
yyy       ( should call BIST() )<br />        Rollback (to yyy)    ( should call RARCST() ) will free pl/xxx exec
context<br/>    Rollback (to xxx)        ( should call RARCST() ) <br /><br />    Probably just one pair, instead of
twonested 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.<br /><br />    I hope I have understood the issue correctly, or have I missed
something?Is there a way to work around this? <br /><br />Best regards,<br clear="all" style="font-family: courier
new,monospace;"/></span><br style="font-family: courier new,monospace;" /><span style="font-family: courier
new,monospace;">--</span><br style="font-family: courier new,monospace;" /><span style="font-family: courier
new,monospace;">gurjeet[.singh]@EnterpriseDB.com</span><brstyle="font-family: courier new,monospace;" /><span
style="font-family:courier new,monospace;">singh.gurjeet@{ gmail | hotmail | yahoo }.com </span> 

<span style="font-family: courier new,monospace;">    Just noticed that this was committed by Tom on 21/Nov with this
logmessage:</span><br style="font-family: courier new,monospace;" /><br style="font-family: courier new,monospace;"
/><spanstyle="font-family: courier new,monospace;"><snip></span><br style="font-family: courier new,monospace;"
/><spanstyle="font-family: courier new,monospace;">Prevent intratransaction memory leak when a subtransaction is
aborted</span><br style="font-family: courier new,monospace;" /><span style="font-family: courier new,monospace;">in
themiddle of executing a SPI query.  This doesn't entirely fix the</span><br style="font-family: courier
new,monospace;"/><span style="font-family: courier new,monospace;">problem of memory leakage in plpgsql exception
handling,but it should</span><br style="font-family: courier new,monospace;" /><span style="font-family: courier
new,monospace;">get rid of the lion's share of leakage.<br /></snip><br /><br />Can the memory leaks be avoided
insome other way?<br /><br />Best regards,<br /><br />-- </span><br style="font-family: courier new,monospace;" /><span
style="font-family:courier new,monospace;"> gurjeet[.singh]@EnterpriseDB.com</span><br style="font-family: courier
new,monospace;"/><span style="font-family: courier new,monospace;">singh.gurjeet@{ gmail | hotmail | yahoo }.com
</span><brstyle="font-family: courier new,monospace;" /><br style="font-family: courier new,monospace;" /><div
style="font-family:courier new,monospace;"><span class="gmail_quote">On 12/3/06, <b class="gmail_sendername">Gurjeet
Singh</b> <<a href="mailto:singh.gurjeet@gmail.com">singh.gurjeet@gmail.com</a>> wrote:</span><blockquote
class="gmail_quote"style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;"> Hi
all,<br/><br />    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
donewith its advisory stuff, it rolls back the SubTransaction. This achieves two objectives: <br /><br />    (1) Easy
cleanupof dependencies/changes in the catalog.<br />    (2) The virtual indexes are not visible to other backends. This
eliminatesthe possibility of other backends (not running in Advise mode) seeing these indexes, as these don't have any
datain them. <br /><br />    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
specialadvise mode. <br /><br />    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. <br /><br />    The problem I am facing is that that the queries being planned
bypl/pgsql, come after a call to SPI_connect() and SPI_Prepare(). SPI_prepare() switches to _SPI_current->execCxt
memorycontext using _SPI_begin_call(). <br /><br />    Everything goes well until the index_adviser() calls RARCST() to
rollbackit's SubTransaction. RARCST() ultimately ends up calling AtEOSubXact_SPI(), which, just before returning,
deletesthe execution memory context of the surrounding SPI (which happens to be pl/pgsql's SPI). This, in effect, frees
allthe 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: <br /><br />void<br />AtEOSubXact_SPI(bool isCommit,
SubTransactionIdmySubid)<br />{<br />    bool        found = false;<br /><br />    while (_SPI_connected >= 0)<br
/>   {<br />        _SPI_connection *connection = &(_SPI_stack[_SPI_connected]); <br /><br />        if
(connection->connectSubid!= mySubid)<br />            break;              /* couldn't be any underneath it either
*/<br/><br />        ......<br />        ......<br />    /*<br />     * If we are aborting a subtransaction and there
isan open SPI context <br />     * surrounding the subxact, clean up to prevent memory leakage.<br />     */<br />   
if(_SPI_current && !isCommit)<br />    {<br />        /* free Executor memory the same as _SPI_end_call would
do*/<br />        MemoryContextResetAndDeleteChildren(_SPI_current->execCxt); <br />        ^^^^^^^^^^^<br />       
/*throw away any partially created tuple-table */<br />        SPI_freetuptable(_SPI_current->tuptable);<br
/>       _SPI_current->tuptable = NULL;<br />    }<br />}<br /><br />    The code is doing what the comments say,
buthow I wish it didn't. This code assumes that the latest BIST() came from a SPI invoking module, which the Index
Adviseris not! <br /><br />    I haven't been able to come up with a proof, but I think this situation can be
simulated/reproducedusing some pl/* language that we support.<br /><br />    pl/pgsql is safe, since it doesn't allow
SAVEPOINT/ROLLBACKTO SAVEPOINT inside it's code blocks; but if some pl/xxx allows SAVEPOINTS, I think it can be
reproducedusing something like: <br /><br />    SAVEPOINT xxx            ( should call BIST() )<br />        SAVEPOINT
yyy       ( should call BIST() )<br />        Rollback (to yyy)    ( should call RARCST() ) will free pl/xxx exec
context<br/>    Rollback (to xxx)        ( should call RARCST() ) <br /><br />    Probably just one pair, instead of
twonested 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.<br /><br />    I hope I have understood the issue correctly, or have I missed
something?Is there a way to work around this? <br /><br />Best regards,<br clear="all" /><span class="sg"><br />-- <br
/>gurjeet[.singh]@EnterpriseDB.com<br />singh.gurjeet@{ gmail | hotmail | yahoo }.com </span></blockquote></div> 

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@2ndquadrant.com> wrote:


--
gurjeet[.singh]@ EnterpriseDB.com
singh.gurjeet@{ gmail | hotmail | yahoo }.com

<span style="font-family: courier new,monospace;">Another question.<br /><br />There are two global variables:<br /><br
/>CurrentResourceOwner(CRO) and CurrentTransactionState (CTS). CTS has a member curTransactionOwner(cTO). Now the
questionis: <br /><br />is the following condition always supposed to be true?<br /><br
/>CurrentResourceOwner==CurrentTransactionState->curTransactionOwner<br/><br />    While in executor, CRO->name
is"Portal". Then I do the BeginInternalSubTransaction() (BIST()); it pushes the current (sub)transaction on a stack and
andalong with it, it sets the CTS as it's parent. CTS->cTO->name is "TopTransaction". And a new resource-owner is
assignedto the new SubTransaction by AtSubStart_ResourceOwner(). <br /><br />    After the adviser is done with it's
work,it calls RollbackAndReleaseCurrentSubTransaction() (RARCST()), which restores the global CRO to the previously
pushedSubTransaction'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.
<br/><br />    So, is the above mentioned condition supposed to hold always? (I am assuming not). If not, then we
shouldremember the CurrentResourceOwner across BIST() and RARCST() calls!<br /><br />    Sorry to be pestering you guys
withseemingly trivial stuff, but you would also agree that it becomes very difficult to understand the code when there
areso many globals and the relationship between them is not very clear. <br /><br />Regards,<br /></span><span
style="font-family:courier new,monospace;"><br style="font-family: courier new,monospace;" /></span><span
style="font-family:courier new,monospace;">-- </span><br style="font-family: courier new,monospace;" /><span
style="font-family:courier new,monospace;">gurjeet[.singh]@EnterpriseDB.com</span><br style="font-family: courier
new,monospace;"/><span style="font-family: courier new,monospace;">singh.gurjeet@{ gmail | hotmail | yahoo }.com
</span>

"Gurjeet Singh" <singh.gurjeet@gmail.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@sss.pgh.pa.us> wrote:
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[.singh]@EnterpriseDB.com
singh.gurjeet@{ gmail | hotmail | yahoo }.com