Re: nested xacts and phantom Xids

Alvaro Herrera <alvherre(at)dcc(dot)uchile(dot)cl> writes:
> Here I present the nested transactions patch and the phantom Xids patch
> that goes with it.

I took a very preliminary look through the nested-xacts patch, too.

> Missing: rollback of SET CONSTRAINTS and GUC vars.

There's a good deal more than that missing :-(. Here are the modules or
actions that are called in CommitTransaction and/or AbortTransaction
that have not yet been touched by the patch:

shared-inval message queue processing (inval.c)
lo_commit
localbuf.c (refcounts need fixed same as bufmgr)
eoxactcallbacks API needs extension
GUC
namespace (temp namespace cleanup)
files (close/drop temp files)
reset current userid during xact abort
password file updating
SetReindexProcessing disable (can these be nested??)

Of these the one that I think most urgently needs attention is inval.c;
that is complicated code already and figuring out what should happen
for subtrans commit/abort is not trivial. The others look relatively
straightforward to fix, if tedious.

Given patches for inval.c and guc.c, I would say that the patch is
functionally close enough to done that we could commit to including
it in 7.5 --- the other stuff could be wrapped up post-feature-freeze.

BUT (you knew there was a but coming, right?) ... I am really really
disturbed about the potential performance hit from this patch.
I don't think we want to have a nested-transaction feature that imposes
significant overhead on people who aren't actually using nested
transactions. As I looked through the patch I found quite a few places
that would impose such overhead.

The main thing that's bothering me is that TransactionIdIsInProgress
has been turned from a relatively cheap test (just scan the in-memory
PGPROC array) into an exceedingly expensive one. It's gratutitously
badly coded (for N open main transactions it does N searches of the
subtrans tree, when one would do), but I don't really want it going to
the subtrans tree at all during typical cases. We had talked about
keeping a limited number of active subtrans IDs in the PGPROC array,
and I think we're really going to need to do that to buy back
performance here.

(BTW, what is the intended behavior of TransactionIdIsInProgress for
a committed or aborted subtransaction whose parent is still open?
If it has to recognize aborted subtranses as still in progress then
things get very messy, but I think it probably doesn't have to.)

I'm quite unhappy with the wide use of SubTransXidsHaveCommonAncestor to
replace TransactionIdEquals in tqual.c, too. This turns very cheap tests
into very expensive ones --- whether or not you use subtransactions ---
and unfortunately tqual.c is coded on the assumption that these tests
are cheap. We can *not* afford to repeat SubTransXidsHaveCommonAncestor
for every open main transaction every time we check a tuple. In many
ways this is the same problem as with TransactionIdIsInProgress, and
it probably needs a similar solution.

Also I found it annoying that XactLockTableWait waits for a top xact
even when target subxact has already aborted; possibly this is even a
recipe for deadlock. Perhaps should iterate up the tree one level at a
time, waiting for each? Would mean that subxacts have to acquire a
lock on their xid that other people can wait for, but this is not
necessarily bad. (In fact, if we do that then I don't think
XactLockTableWait has to change at all.)

So I think there are several must-fix performance issues as well before
we can make a decision to accept this for 7.5.

I'd feel better about this if we had another month, but with ten days
before feature freeze it's gonna be touch and go.

regards, tom lane