Tom Lane wrote:
> This patch implements Florian's idea about how to manage snapshot xmax
> without the ugly and performance-losing tactic of taking XidGenLock and
> ProcArrayLock at the same time. I had to do a couple of slightly klugy
> things to get bootstrap and prepared transactions to work, but on the
> whole it seems at least as clean as the code we have now. Comments?
I'm a bit late, but still a few comments
1) I wonder why you made XidCacheRemoveRunningXids take the latestXid as
an extra argument, though - it should be able to figure that out on
it's own I'd have thought. Is that just for consistency, or did I miss
something?
2) I don't see how either the childXids array or the XID cache in the
proc array could ever not be in ascending xid order. We do guarantee
that a child xid is always greater than it's parent. This guarantee
enables a few optimizations. First, as you say in the comments,
finding the largest xid when committing becomes trivial. But more
important, if we can assume that the proc array xid cache is always
sorted, we can get ride of the exclusive lock during subxact abort.
We will *always* remove the last n xids from the cache, so we just
need to reset subxids.nxids, and not touch the actual array at all.
(We might later set the now-unused entries to InvalidTransactionId,
but thats only cosmetic).
Regarding 2) Removing subxids from the proc array cannnot effect xmin
calculations, because the toplevel xid is always small than all it's
children. So we only need to care about snapshots themselves.
But they won't care much if they find an aborted sub xid in the
proc array or now. If not, the xid is neither running nor committed,
so it's assumes to have crashed. If it's in the snapshot, it's
treated as in-progress.
So I believe the code could be simplified a bit if we just made it a rule
that both childXids and the cache in the proc array are always in ascending
xid order, and we could get rid of the exclusive lock during subxact abort.
greetings, Florian Pflug
