reviewing the "Reduce sinval synchronization overhead" patch / b4fbe392f8ff6ff1a66b488eb7197eef9e1770a4

Hi,

I am reviewing this one year old change again before backporting it to 9.1.3 for 
production use.

ATM, I believe the code is correct, but I don't want to miss the change to spot 
possible errors, so please let me dump my brain on some points:

- IIUC, SIGetDataEntries() can return 0 when in fact there _are_ messages  because  stateP->hasMessages could come from
astale cache (iow there is no  read-membar used and because we return before acquiring SInvalReadLock (which  the patch
isall about in the first place), we don't get an implicit  read-membar from a lock op any more).
 
  What I can't judge on: Would this cause any harm? What are the consequences  of SIGetDataEntries returning 0 after
anotherprocess has posted a message  (looking at global temporal ordering)?
 
  I don't quite understand the significance of the respective comment in the  code that the incoherence should be
acceptablebecause the cached read can't  migrate to before a previous lock acquisition (which itself is clear).
 
  AcceptInvalidationMessages has a comment that it should be the first thing  to do in a transaction, and I am not sure
ifall the consumers have a  read-membar equivalent operation in place.
 
  How bad would a missed cache invalidation be? Should we have a read-membar  in SIGetDataEntries just to be safe?

Other notes on points which appear correct to me (really more a note to myself):

- stateP->hasMessages = false in SIGetDataEntries is membar'ed by  SpinLockAcquire(&vsegP->msgnumLock), so it shouldn't
happenthat  clearing hasMessages moves behind reading msgnumLock
 
  (in which case we could loose the hasMessages flag)

- but it can happen that hasMessages gets set when in fact there is  nothing to read (which is fine because we then
checkmaxMsgNum)
 

Nils