Small SSI issues

It makes wonders to take a couple of months break from looking at a
piece of code, and then review it in detail again. It's like a whole new
pair of eyes :-).

Here's a bunch of small issues that I spotted:

* The oldserxid code is broken for non-default BLCKSZ.
o The warning will come either too early or too late
o There is no safeguard against actually wrapping around the SLRU,
just the warning
o I'm suspicious of the OldSerXidPagePrecedesLogically() logic with
32k BLCKSZ. In that case, OLDSERXID_MAX_PAGE is a larger than
necessary to cover the whole range of 2^32 transactions, so at high
XIDs, say 2^32-1, doesn't it incorrectly think that low XIDs, say
10, are in the past, not the future?

We've discussed these SLRU issues already, but still..

> /*
> * Keep a pointer to the currently-running serializable transaction (if any)
> * for quick reference.
> * TODO SSI: Remove volatile qualifier and the then-unnecessary casts?
> */
> static volatile SERIALIZABLEXACT *MySerializableXact = InvalidSerializableXact;

* So, should we remove it? In most places where contents of
MySerializableXact are modified, we're holding
SerializableXactHashLock in exclusive mode. However, there's two
exceptions:

o GetSafeSnapshot() modifies MySerializableXact->flags without any
lock. It also inspects MySerializableXact->possibleUnsafeConflicts
without a lock. What if somone sets some other flag in the flags
bitmap just when GetSafeSnapshot() is about to set
SXACT_FLAG_DEFERRABLE_WAITING? One of the flags might be lost :-(.

o CheckForSerializableConflictIn() sets SXACT_FLAG_DID_WRITE without
holding a lock. The same danger is here if someone else tries to
set some other flag concurrently.

I think we should simply acquire SerializableXactHashLock in
GetSafeSnapshot(). It shouldn't be so much of a hotspot that it would
make any difference in performance. CheckForSerializableConflictIn()
might be called a lot, however, so maybe we need to check if the flag
is set first, and only acquire the lock and set it if it's not set
already.

* Is the SXACT_FLAG_ROLLED_BACK flag necessary? It's only set in
ReleasePredicateLocks() for a fleeting moment while the function
releases all conflicts and locks held by the transaction, and finally
the sxact struct itself containing the flag. Also, isn't a
transaction that's already been marked for death the same as one that
has already rolled back, for the purposes of detecting conflicts?

* The HavePartialClearThrough/CanPartialClearThrough mechanism needs a
better explanation. The only explanation currently is this:

> if (--(PredXact->WritableSxactCount) == 0)
> {
> /*
> * Release predicate locks and rw-conflicts in for all committed
> * transactions. There are no longer any transactions which might
> * conflict with the locks and no chance for new transactions to
> * overlap. Similarly, existing conflicts in can't cause pivots,
> * and any conflicts in which could have completed a dangerous
> * structure would already have caused a rollback, so any
> * remaining ones must be benign.
> */
> PredXact->CanPartialClearThrough = PredXact->LastSxactCommitSeqNo;
> }

If I understand that correctly, any predicate lock belonging to any
already committed transaction can be safely zapped away at that
instant. We don't do it immediately, because it might be expensive.
Instead, we set CanPartialClearThrough, and do it lazily in
ClearOldPredicateLocks(). But what is the purpose of
HavePartialClearThrough?

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com