On Tue, Oct 1, 2013 at 2:08 PM, Andres Freund <andres@2ndquadrant.com> wrote:
>> As for the specific patch being discussed here. The read barrier is in
>> the wrong place and with the wrong comment, and the write side is
>> assuming that SpinLockAcquire() is a write barrier, which it isn't
>> documented to do at the moment.
>
> Lots of things we do pretty much already assume it is one - and I have a
> hard time imagining any spinlock implementation that isn't a write
> barrier. In fact it's the only reason we use spinlocks in quite some
> places.
> What we are missing is that it's not guaranteed to be a compiler barrier
> on all platforms :(. But that's imo a bug.
Agree on both counts.
>> The correct way to think of this is
>> that StartupXLOG() does a bunch of state modifications and then
>> advertises the fact that it's done by setting
>> xlogctl->SharedRecoveryInProgress = false; The state modifications
>> should better be visible to anyone seeing that last write, so you need
>> one write barrier between the state modifications and setting the
>> flag.
>
> SpinLockAcquire() should provide that.
Yes. It's notable that in this case it's a matter of correctness that
the global state modifications do *not* share the critical section
with the flag update. Otherwise the flag update may become visible
before the state updates.
>> So we need a read barrier somewhere *after* reading the flag in
>> RecoveryInProgress() and reading the shared memory structures, and in
>> theory a full barrier if we are going to be writing data. In practice
>> x86 is covered thanks to it's memory model, Power is covered thanks to
>> the control dependency and ARM would need a read barrier, but I don't
>> think any real ARM CPU does speculative stores as that would be
>> insane.
>
> Does there necessarily have to be a "visible" control dependency?
Unfortunately no, the compiler is quite free to hoist loads and even
stores out from the conditional block losing the control dependency.
:( It's quite unlikely to do so as it would be a very large code
motion and it probably has no reason to do it, but I don't see
anything that would disallow it. I wonder if we should just emit a
full fence there for platforms with weak memory models. Trying to
enforce the control dependency seems quite fragile.
Regards,
Ants Aasma
--
Cybertec Schönig & Schönig GmbH
Gröhrmühlgasse 26
A-2700 Wiener Neustadt
Web: http://www.postgresql-support.de
