Re: Facing a problem with SPI - Mailing list pgsql-hackers

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[.singh]@EnterpriseDB.com
singh.gurjeet@{ gmail | hotmail | yahoo }.com