Re: Yes, WaitLatch is vulnerable to weak-memory-ordering bugs

On Mon, Aug 8, 2011 at 11:28 AM, Peter Geoghegan <peter@2ndquadrant.com> wrote:
>> Maybe we should try to document the contract here
>> a little better; I think it's just that there must be a full memory
>> barrier (such as LWLockRelease) in both processes involved in the
>> interaction.
>
> Or, maybe we should think about pushing that into the latch
> implementation, while providing an interface that is easy to use
> correctly and hard to use incorrectly.

That would probably result in unnecessary double-synchronization in
many cases, though.  Memory barriers are expensive, and I think we'd
better adopt a policy of trying to figure out where they are truly
needed and using them only in those places.  Or to put that another
way, having spent the last couple of months trying to reduce our
synchronization overhead, I'm not real keen on adding more unless we
really need it.

>> I've been thinking about this too and actually went so far as to do
>> some research and put together something that I hope covers most of
>> the interesting cases.  The attached patch is pretty much entirely
>> untested, but reflects my present belief about how things ought to
>> work.
>
> That's interesting. Nice work.

Thanks!

> It seems likely that Windows will support ARM in some way in the next
> couple of years, so it's good that you're handling this using a win32
> abstraction where available. Of course, Itanic is currently supported
> on Windows, though not by us, and it is obviously never going to be
> worth pursuing such support. The point is that it generally isn't safe
> to assume that we'll only ever support Windows on x86 and x86_64.

Agreed.  I saw an MSDN article that referred to "architectures on
which Windows is supported".  It didn't say which those were, but I
figured I'd better not assume they were all platforms with strong
memory ordering.

> I'd like to see a #warning where you fall back on acquiring and
> releasing a spinlock, or at least a #warning if that in turn falls
> back on the semaphore-based spinlock implementation. Granted, that
> directive isn't in the C standard, but it's pretty portable in
> practice. If it breaks the build on some very esoteric platform, that
> may not be a bad thing - in fact, maybe you should use the portable
> #error directive to make sure it breaks. I'd rather have someone
> complain about that than lots more people complain about Postgres
> being inexplicably slow, or worse not complain and dismiss Postgres
> out of hand for their use-case.

I was thinking about whether and how we could expose that information.I was almost tempted to add a read-only GUC that
wouldoutput the 
details of what kind of synchronization we're using.  So the bad case
would look something like this:

spinlock=semaphore,memory_barrier=spinlock,read_barrier=spinlock,write_barrier=spinlock,compiler_barrier=spinlock

And the good case would look something like this:


spinlock=gcc-inline-asm,memory_barrier=gcc-inline-asm,read_barrier=gcc-inline-asm,write_barrier=gcc-inline-asm,compiler_barrier=gcc-inline-asm

And you can imagine other values, like compiler-intrinsic.  Of course,
jamming a whole CSV file into a GUC value is a bit unappealing.  Maybe
it would be better to have a system view with two columns,
synchronization_primitive and implementation.

Either way, I'd prefer this to a #warning, because that's only going
to be visible at compile-time, which means that when $PGCOMPANY gets
called in to figure out why the system is dog slow, it's going to take
a bit of forensics to figure out where the build came from and what
options are in use.  Is that a huge problem?  Maybe not, but it seems
like it would be a nice thing to make it easy for people to log into a
running system and immediately be able to determine which forms of
wizardry we're using there.

> By the way, I don't think that the comment "Won't work on Visual
> Studio 2003" is accurate. Everything you do for the
> defined(WIN32_ONLY_COMPILER) case is documented as working with 2003.

Oh, really?  I thought I ran across something that said otherwise, but
it might have been wrong, or maybe I got confused somewhere along the
line.  I haven't tested anything more than that this compiles without
erroring out on MacOS X, so the probability of errors and omissions is
not small.

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