Thread: SPI: Correct way to rollback a subtransaction?

I want to update one row from possibly several backends. In case
of SERIALIZABLE transactions, the update command may fail.  To
hide it from calling transaction, I use a subtransaction and try
to catch and hide the error.

With help of plpgsql source, I wrote following code that _seems_
to work.  But I have no idea if it's the correct way to do it:

/* store old state */MemoryContext oldcontext = CurrentMemoryContext;ResourceOwner oldowner = CurrentResourceOwner;
BeginInternalSubTransaction(NULL);res = SPI_connect();if (res < 0)    elog(ERROR, "cannot connect to SPI");
PG_TRY();{    res = SPI_execute("update one row", false, 0);    SPI_finish();
ReleaseCurrentSubTransaction();}PG_CATCH();{   SPI_finish();    RollbackAndReleaseCurrentSubTransaction();
FlushErrorState();   res = -1; /* remember failure */}PG_END_TRY(); 
/* restore old state */MemoryContextSwitchTo(oldcontext);CurrentResourceOwner = oldowner;


I am suspicious about the ..SubTransaction and SPI_* nesting
and resetting the error state.  Can anyone look if it's correct?

Goal of the exercise is to have 8-byte transaction ID's and snapshots
externally available.  (Port of xxid module from Slony).  Above code
does the update of the 'epoch' table that has only one row.

--
marko

"Marko Kreen" <markokr@gmail.com> writes:
>     BeginInternalSubTransaction(NULL);
>     res = SPI_connect();
>     if (res < 0)
>         elog(ERROR, "cannot connect to SPI");

>     PG_TRY();
>     {
>         res = SPI_execute("update one row", false, 0);
>         SPI_finish();
>         ReleaseCurrentSubTransaction();
>     }
>     PG_CATCH();
>     {
>         SPI_finish();
>         RollbackAndReleaseCurrentSubTransaction();
>         FlushErrorState();
>         res = -1; /* remember failure */
>     }
>     PG_END_TRY();


This seems like a pretty bad idea: if the SPI_connect fails you lose
control without having unwound the subtransaction.  That's unlikely,
but still wrong.  I think you could do this as
BeginInternalSubTransaction(NULL);PG_TRY();{    res = SPI_connect();    if (res < 0)        elog(ERROR, "cannot connect
toSPI");    res = SPI_execute("update one row", false, 0);    SPI_finish();
ReleaseCurrentSubTransaction();}PG_CATCH();{   /* we expect rollback to clean up inner SPI call */
RollbackAndReleaseCurrentSubTransaction();   FlushErrorState();    res = -1; /* remember failure */}PG_END_TRY();
 

Check the abort-subtrans path but I think it gets you out of the nested
SPI call.  (Because pl_exec.c wants to preserve an already-opened SPI
call, it has to go out of its way to undo this via SPI_restore_connection.
I *think* you don't need that here but am too lazy to check for sure.
Anyway it'll be good practice for you to figure it out for yourself ;-))
        regards, tom lane

On 2/20/06, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> "Marko Kreen" <markokr@gmail.com> writes:
> >       BeginInternalSubTransaction(NULL);
> >       res = SPI_connect();
> >       if (res < 0)
> >               elog(ERROR, "cannot connect to SPI");

> This seems like a pretty bad idea: if the SPI_connect fails you lose
> control without having unwound the subtransaction.  That's unlikely,
> but still wrong.

But if I want the error to reach upper transaction?  SPI_connect
failure does not seem a 'expected' situation to me.

Or will the started subtransaction corrupt some state?

>         PG_CATCH();
>         {
>                 /* we expect rollback to clean up inner SPI call */
>                 RollbackAndReleaseCurrentSubTransaction();
>                 FlushErrorState();
>                 res = -1; /* remember failure */
>         }
>         PG_END_TRY();
>
> Check the abort-subtrans path but I think it gets you out of the nested
> SPI call.  (Because pl_exec.c wants to preserve an already-opened SPI
> call, it has to go out of its way to undo this via SPI_restore_connection.
> I *think* you don't need that here but am too lazy to check for sure.
> Anyway it'll be good practice for you to figure it out for yourself ;-))

Thank you for hints.  The RollbackAndReleaseCurrentSubTransaction()
seems to call AbortSubTransaction->AtEOSubXact_SPI() only if the
transaction is TBLOCK_SUBINPROGRESS,  As SERIALIZABLE seems to
thow simple elog(ERROR, ...) [executor/execMain.c], and error
handling also does not seem to touch transaction state, it seems
calling SPI_finish() is not needed.

Correct?  (Yes, I'm newbie in core code...)

--
marko

"Marko Kreen" <markokr@gmail.com> writes:
> On 2/20/06, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> This seems like a pretty bad idea: if the SPI_connect fails you lose
>> control without having unwound the subtransaction.  That's unlikely,
>> but still wrong.

> But if I want the error to reach upper transaction?  SPI_connect
> failure does not seem a 'expected' situation to me.

In that case you should put the SPI_connect and later SPI_finish
*outside* the subtransaction and TRY block.  And you'll need
SPI_restore_connection I think.  This structure would be exactly
parallel to the way pl_exec.c does it.
        regards, tom lane

On 2/21/06, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> "Marko Kreen" <markokr@gmail.com> writes:
> > On 2/20/06, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >> This seems like a pretty bad idea: if the SPI_connect fails you lose
> >> control without having unwound the subtransaction.  That's unlikely,
> >> but still wrong.
>
> > But if I want the error to reach upper transaction?  SPI_connect
> > failure does not seem a 'expected' situation to me.
>
> In that case you should put the SPI_connect and later SPI_finish
> *outside* the subtransaction and TRY block.  And you'll need
> SPI_restore_connection I think.  This structure would be exactly
> parallel to the way pl_exec.c does it.

It does not seem worth the complexity, I rather go with the simple
approach and put it inside TRY block then.

--
marko