While thinking about Heikki's 2PC patch, I've been getting annoyed about
how the SInvalLock is being overused. (The connection is that the
separate LWLock he proposes in his patch isn't going to work: addition
and removal of entries in the prepared-xact list has to be guarded by
SInvalLock, because it is not OK for a transaction to appear to exit
the InProgress state except while holding SInvalLock. See notes in
GetSnapshotData.)
The reason things got this way is that we've been loading more and more
functionality onto the array of PGPROC pointers that is, more or less
incidentally, maintained by sinval.c. I'm thinking that it'd be a good
idea to remove most of that stuff from sinval.c and put it into a
separate module that maintains its own array of PGPROC pointers with a
separate lock. There's no reason why, eg, GetSnapshotData needs to
conflict with transfer of SI inval messages, which is what SInvalLock
was originally designed to protect.
A small speed benefit could come from the fact that (AFAICS) there is
no need to maintain any particular ordering in this separate array. In
particular, deletion of an entry could be done by moving down the last
entry to fill the hole, so there need never be any gaps. That means
we'd simplify the looping logic from
for (index = 0; index < segP->lastBackend; index++) { SHMEM_OFFSET pOffset = stateP[index].procStruct;
if (pOffset != INVALID_OFFSET) { PGPROC *proc = (PGPROC *) MAKE_PTR(pOffset);
... do something useful here ... } }
to
for (index = 0; index < stateP->numBackends; index++) { PGPROC *proc = stateP->procPtr[index];
... do something useful here ... }
which ought to save at least a cycle or two.
Comments, objections? Any thoughts about what to call the new module
and lock? I was thinking maybe procarray.c and ProcArrayLock, but I'm
not terribly fond of that.
Portions of backend/storage/lmgr/proc.c perhaps ought to migrate into
this new file as well, but I haven't thought it through yet.
Note also that I'm thinking of making the shared memory prepared-XIDs
list for 2PC part of the responsibility of this module, rather than
being a separate file as Heikki has in his patch. Since they're going
to share a lock it doesn't seem very useful to keep them at arm's
length.
regards, tom lane
