Small SSI issues - Mailing list pgsql-hackers

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
isno 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
necessaryto cover the whole range of 2^32 transactions, so at high    XIDs, say 2^32-1, doesn't it incorrectly think
thatlow 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
SerializableXactHashLockin 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
bitmapjust 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
someoneelse 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
thatit would  make any difference in performance. CheckForSerializableConflictIn()  might be called a lot, however, so
maybewe 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
thefunction  releases all conflicts and locks held by the transaction, and finally  the sxact struct itself containing
theflag. Also, isn't a  transaction that's already been marked for death the same as one that  has already rolled back,
forthe purposes of detecting conflicts?
 

* The HavePartialClearThrough/CanPartialClearThrough mechanism needs a  better explanation. The only explanation
currentlyis 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
zappedaway 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