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 a stale cache (iow there is no
read-membar used and because we return before acquiring SInvalReadLock (which
the patch is all 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 another process 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 acceptable because 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 if all 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 happen that
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 check maxMsgNum)

Nils