Thread: Re: Nested xact status?

Cc'ed to hackers ...

On Sat, Jul 24, 2004 at 02:10:49PM -0400, Tom Lane wrote:
> > I think I am done with this part.  Please review and apply it.  The
> > attached patch is probably the same I posted to patches some days ago,
> > only updated with the latest commits.
> 
> It looks pretty good, but one thing I don't much like is the static
> AbortToLevel variable.  I am thinking it would be better to have a
> transaction state value that indicates "abort pending", and to have
> the Rollback command mark every transaction up to the savepoint with
> that state value, and then the transaction-end code responds to that.
> Any particular reason you didn't do it that way?

A previous discarded patch used the static var as means of setting an
abort point when a function was called.  The idea was to close all
subtransactions opened within the function, so that the user wouldn't be
presented with subtransactions that the function failed to close.  I
think this is still needed, of course.

I thought about adding something to the transaction state, but the
"abort pending" state didn't come to my mind.  Yes, I think it's
cleaner.

> Would we potentially be losing any needed state?  (ISTM all the
> transactions that could need to change would be in TRANS_INPROGRESS
> states, and so shifting them all to TRANS_PENDING_ABORT shouldn't lose
> any information.  But it would likely be trickier if we tried to do it
> as a TBLOCK state.)

Note that we don't allow to start subtransactions in aborted state, so
all intermediate subtransactions have to be in (TBLOCK) in-progress
state anyway.  (The only reason StartAbortedSubtransaction() survived in
this patch is to allow a subtransaction to error out while in
TBLOCK_BEGIN state -- not sure what should really happen here.)

I don't see a lot of value in using TRANS states as control mechanism;
rather they are there to make sure we only make valid transaction calls.


> I will look at this.  One reason I'd like to have the "abort pending"
> xact state is that I think we may need to be able to control rollback
> from outside xact.c, and it'll probably be cleaner with that.

Cool.  Thank you very much.

One thing I noticed is that there are several ways to call a function --
as a rangevar, through SPI, and as "normal" functions in the executor.
Most likely all of them need some kind of handling.

-- 
Alvaro Herrera (<alvherre[a]dcc.uchile.cl>)
"No deja de ser humillante para una persona de ingenio saber
que no hay tonto que no le pueda enseñar algo." (Jean B. Say)

Alvaro Herrera <alvherre@dcc.uchile.cl> writes:
> A previous discarded patch used the static var as means of setting an
> abort point when a function was called.  The idea was to close all
> subtransactions opened within the function, so that the user wouldn't be
> presented with subtransactions that the function failed to close.  I
> think this is still needed, of course.

I think that functions should not have the ability to do random things
to the subtransaction state, and one of the things that would be right
out is exiting at a different subtransaction nest level than they
started in.  It is not practical for xact.c as such to enforce this;
it has to be the responsibility of the functions.

In the case of plpgsql this will be probably be enforced by not offering
direct access to begin/commit/savepoint/rollback at all.  IIRC there is
a try/catch-like construct in Oracle pl/sql, and I think what we want to
do is offer that syntax as the interface to creating and rolling back
subtransactions.  In other words, TRY does a SAVEPOINT, successful exit
from the TRY block does a RELEASE, and any error inside the TRY block
does a ROLLBACK AND RELEASE then transfers control to the CATCH code.

We probably want to do something roughly similar in SPI as well, ie,
convert the use of savepoints into an implementation detail inside an
API that lets you catch errors in SPI commands you execute.  I do not
want people doing SPI_exec("savepoint") or SPI_exec("rollback") because
I don't think it makes any sense.

BTW, whether or not you think that ROLLBACK AND RELEASE is needed at the
SQL level, it is definitely needed at the implementation level.  We
don't want useless creation and destruction of a subtransaction every
time a plpgsql function catches an error.
        regards, tom lane