Thread: Cleaning up recovery from subtransaction start failure

BTW, I'm not going to make the lock release/XactLockTableWait fix just yet,
because exhaustion of shared memory provides an easy test case for a
problem that I want to fix first.  What I noticed while testing the
reported case is that you get

WARNING:  out of shared memory
CONTEXT:  PL/pgSQL function "do_standard_mgc" line 2 at block variables initialization

(this is ShmemAlloc choking)

ERROR:  out of shared memory
HINT:  You may need to increase max_locks_per_transaction.
CONTEXT:  PL/pgSQL function "do_standard_mgc" line 2 at block variables initialization

(this is LockAcquire choking as a result)

WARNING:  StartAbortedSubTransaction while in START state

(this is xact.c trying to cope with the resultant error exit)

and after that we are stuck inside an aborted subtransaction:

(gdb) p *CurrentTransactionState
$2 = {transactionIdData = 0, name = 0x0, savepointLevel = 0, commandId = 8414, state = TRANS_ABORT, blockState =
TBLOCK_SUBABORT,nestingLevel = 2, curTransactionContext = 0x4011b6d0, curTransactionOwner = 0x4017b1f8, childXids =
0x0,currentUser = 0, prevXactReadOnly = 0 '\000', parent = 0x40001130}
 
(gdb) p *CurrentTransactionState->parent
$3 = {transactionIdData = 78724, name = 0x0, savepointLevel = 0, commandId = 8414, state = TRANS_INPROGRESS, blockState
=TBLOCK_STARTED, nestingLevel = 1, curTransactionContext = 0x400bc6b8, curTransactionOwner = 0x40069d58, childXids =
0x400c3030,currentUser = 0, prevXactReadOnly = 0 '\000', parent = 0x0}
 
(gdb)

which means that I have to type "abort;" to get the system back into a
usable state.  Since I did not type "begin;" first, that is clearly a
bug.

I don't actually like StartAbortedSubTransaction at all --- ISTM that if
you get a failure trying to enter a subxact, it's better *not* to enter
the subxact and instead to treat the error as putting the calling xact
in abort state.

Also, I found a problem a couple days ago with errors occurring during
transaction commit, for instance a sequence likebegin;set constraints all deferred;insert into foo () -- assume this
violatesa deferred FK constraintsavepoint a;commit;
 

The problem is that the COMMIT command only changed the state of the
topmost transaction record (the savepoint's subxact) which we then pop
off the stack during commit.  After that we run deferred triggers.
If we get an error at that stage, there is nothing left on the state
stack to remind us that the user had typed commit ... and so guess what,
we stay inside the aborted transaction, and the user has the same
problem of needing to type abort when he shouldn't.

I put in what I think is a correct fix for that one here:
http://developer.postgresql.org/cvsweb.cgi/pgsql-server/src/backend/access/transam/xact.c.diff?r1=1.186;r2=1.187
(ignore the renamings of trigger functions in the same patch).
Basically the idea is that COMMIT should mark the *entire* state stack
before we start to pop anything, and then we will still remember what we
were doing if we get an error after the first pop.

Since then I have been thinking that we should get rid of some of the
subxact-abort-related states in favor of handling abort cases similarly.
I don't have a test case to prove that there's a problem, but in general
errors during error aborts are a real threat that has to be considered.

Thoughts?
        regards, tom lane

I wrote:
> I don't actually like StartAbortedSubTransaction at all --- ISTM that if
> you get a failure trying to enter a subxact, it's better *not* to enter
> the subxact and instead to treat the error as putting the calling xact
> in abort state.

Actually, the more I think about this the more I like the idea I just
mentioned in the other thread, which is to not assign a subxact an XID
right away.  The advantage is that acquiring an XID and a lock on it
are definitely operations that could fail (cf original problem), and
it becomes very painful to ensure that xact.c's internal state stays
legal in event of a failure there, if we insist that that happens during
subxact start.

I've just looked through the modules that use GetCurrentTransactionId
to label their internal state, and without exception they'd be perfectly
happy with an ersatz substitute that only distinguishes among
subtransactions of the current top transaction.  So here is what I am
thinking:

* New datatype SubTransactionId == uint32 (just for clarity of code)

* Special values InvalidSubTransactionId == 0 and TopSubTransactionId == 1; we initialize the counter for each main
transactionso that subxacts get IDs beginning at 2.
 

* Subtransaction start looks about like this:
subxactid = ++SubTransactionIdCounter;if (subxactid == InvalidSubTransactionId){    // oops, wrapped around
--SubTransactionIdCounter;   elog(ERROR, "out of subxact xids");}
 
subxactstate = palloc(sizeof(TransactionStateData));
// initialize subxact block and link it into state stack
// call modules that need start-of-subxact calls

Note that if we get an out-of-memory failure in the palloc, we have not
done anything except waste a SubTransactionId value, so there is no
recovery problem there.  Once the palloc succeeds, no error is possible
between there and having a fully-set-up subxact.  Errors in the "call
modules" phase, if any, can be treated as errors occuring inside an
already-established subxact.  Or we could choose to abort and pop the
subxact, if you like the theory that failure during SAVEPOINT should
not create a broken subxact; this isn't too hard since we know we have
gotten to a valid stack state that abort can cope with, which was
definitely not guaranteed before.

The places that currently call GetCurrentTransactionId for labeling
internal state would all call a new GetCurrentSubTransactionId routine.
Other than that and renaming/retyping their state fields, they'd not
need much change in most cases.

The remaining calls of GetCurrentTransactionId would mostly be the ones
in heapam.c that are labeling tuples about to be written to disk.

One minor point is that GetCurrentTransactionId would now become a
routine with a nonzero prospect of failure.  We'd want to be sure that
it is not called inside critical sections (since ERROR -> PANIC inside
such sections), unless we are certain a current XID has been assigned.
This would require only trivial code rearrangements in heapam.c, but
there is a call in XLogInsert that is a bigger hazard.  I believe all
calls of XLogInsert are *already* inside critical sections, and so if
any of them are in code paths where no XID has been assigned, we have
a hard-to-spot risk of system panic.  But there probably isn't any good
reason to be emitting an XLOG record without having assigned an XID.
I think I would fix this by not having XLogInsert do its own call,
but requiring callers to pass in the XID to use.  It will be easy to
make the callers do GetCurrentTransactionId before they enter their
critical sections.

Bottom line: I'm now leaning towards doing this for 8.0, since it would
make for a useful improvement in robustness, quite aside from any
performance gains from eliminating unnecessary XID assignments.

Comments?
        regards, tom lane

On Mon, Sep 13, 2004 at 08:40:46PM -0400, Tom Lane wrote:

> The remaining calls of GetCurrentTransactionId would mostly be the ones
> in heapam.c that are labeling tuples about to be written to disk.
>
> One minor point is that GetCurrentTransactionId would now become a
> routine with a nonzero prospect of failure.

Just to be sure I understand your proposal: the idea is that a
subtransaction would not have a TransactionId assigned right away, but
instead the first call to GetCurrentTransactionId in the subxact would
assign it (and call SubTransSetParent and XactLockTableInsert).  Is this
right?

> Bottom line: I'm now leaning towards doing this for 8.0, since it would
> make for a useful improvement in robustness, quite aside from any
> performance gains from eliminating unnecessary XID assignments.

I agree it's a very interesting robustness improvement.  Also let me
point out explicitly that it would reduce the wastage of pg_clog and
pg_subtrans space by a possibly big margin, reduce the number of locks
taken by XactLockTableInsert(), and reduce the recently talked about
problems of transaction Id wraparound.  The savings could be
considerable, especially where high usage of pl/pgsql exceptions are
used.  IMHO it definitely has to be in 8.0, or we risk too much in Xid
wraparound, especially when lots of people is starting to use Pg on high
volume, high update environments.

-- 
Alvaro Herrera (<alvherre[a]dcc.uchile.cl>)
FOO MANE PADME HUM

Alvaro Herrera <alvherre@dcc.uchile.cl> writes:
> Just to be sure I understand your proposal: the idea is that a
> subtransaction would not have a TransactionId assigned right away, but
> instead the first call to GetCurrentTransactionId in the subxact would
> assign it (and call SubTransSetParent and XactLockTableInsert).  Is this
> right?

Right.  The theory being that a subxact that hasn't yet done anything
to global database state need not own an XID.  We had optimized on the
assumption that most subxacts would eventually need an XID, but in
hindsight I'm not sure why we thought that must be so.  In any case,
I can't see any significant performance cost associated with postponing
the assignment, so why not?

> IMHO it definitely has to be in 8.0, or we risk too much in Xid
> wraparound, especially when lots of people is starting to use Pg on high
> volume, high update environments.

I'm leaning the same way.  I'll work on this tomorrow unless someone
raises a good objection before then.
        regards, tom lane