Re: sinval synchronization considered harmful

On Wed, Jul 27, 2011 at 12:55 PM, Noah Misch <noah(at)2ndquadrant(dot)com> wrote:
> This approach would work if a spinlock release constrained the global stores
> timeline.  It makes a weaker guarantee: all stores preceding the lock release
> in program order will precede it globally.  Consequently, no processor will
> observe the spinlock to be available without also observing all stores made by
> the last holding processor before that processor released it.  That guarantee
> is not enough for this sequence of events:
>
> Backend 0:
>        add a message for table "a"
>        store 5 => maxMsgNum
>        store true => timeToPayAttention
>        LWLockRelease(SInvalWriteLock)
> <plenty of time passes>
> Backend 2:
>        LOCK TABLE a;
>        load timeToPayAttention => true
> Backend 1:
>        add a message for table "b"
>        store 6 => maxMsgNum
>        SpinLockRelease(&vsegP->msgnumLock);
>        store true => timeToPayAttention
> Backend 2:
>        store false => timeToPayAttention
>        LWLockAcquire(SInvalReadLock, LW_SHARED)
>        load maxMsgNum => 5 [!]

Eh, how can this possibly happen? You have to hold msgNumLock to to
set maxMsgNum and msgNumLock to read maxMsgNum. If that's not enough
to guarantee that we never read a stale value, what is?

> I think a benchmark is in order, something like 900 idle connections and 80
> connections running small transactions that create a few temporary tables.  If
> that shows no statistically significant regression, then we're probably fine
> here.  I'm not sure what result to expect, honestly.

That's setting the bar pretty high. I don't mind doing the
experiment, but I'm not sure that's the case we should be optimizing
for.

> What did you think of making the message number a uint64, removing wraparound
> handling, and retaining SISeg.msgnumLock for 32-bit only?  You could isolate the
> variant logic in READ_MSGNUM and WRITE_MSGNUM macros.

Well, what you really need to know is whether the platform has 8-byte
atomic stores, which doesn't seem trivial to figure out, plus you need
a memory fence. If that's the only method of fixing this problem we
can agree on, I'm willing to work on it, but an
architecture-independent fix would be nicer.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company