On Mon, Mar 18, 2013 at 10:09 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Boszormenyi Zoltan <zb@cybertec.at> writes:
>> How about the attached patch over current GIT? In other words,
>> why I am wrong with this idea?
>
> Because it's wrong. Removing "volatile" means that the compiler is
> permitted to optimize away stores (and fetches!) on the basis of their
> being unnecessary according to straight-line analysis of the code.
> Write barriers don't fix that, they only say that stores that the
> compiler chooses to issue at all have to be ordered a certain way.
I don't think this is correct. The read and write barriers as
implemented are designed to function as compiler barriers also, just
as they do in the Linux kernel and every other piece of software I've
found that implements anything remotely like this, with the lone
exception of PostgreSQL. In PostgreSQL, spinlock acquisition and
release are defined as CPU barriers but not a compiler barrier, and
this necessitates extensive use of volatile all over the code base
which would be unnecessary if we did this the way it's done in Linux
and elsewhere.
However, Zoltan's patch probably isn't right either. First, a write
barrier isn't ever the correct solution when there's only one process
involved - which is the case here, because the relevant variables are
in backend-private memory. A compiler barrier - which is generally
far cheaper - might be the right thing, though. However, the position
of the barriers in his proposed patch is suspect. As implemented,
he's proposing to force each change to alarm_enabled to be scheduled
by the compiler before any other writes to memory are completed. But
there's no guard against the compiler sliding the change backward,
only forward. Now maybe that doesn't matter, if we're only concerned
about the timing of updates of alarm_enabled relative to other updates
of that same variable. But if there are multiple variables involved,
then it matters.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company