On Wed, Nov 6, 2013 at 4:54 PM, Andres Freund <andres@2ndquadrant.com> wrote:
> FWIW, I find the requirement for atomic_init() really, really
> annoying. Not that that will change anything ;)
Me too, but I guess this allows them to emulate atomics on platforms
that don't have native support. Whether that is a good idea is a
separate question.
>> However I'm mildly supportive on having a separate type for variables
>> accessed by atomics. It can result in some unnecessary code churn, but
>> in general if an atomic access to a variable is added, then all other
>> accesses to it need to be audited for memory barriers, even if they
>> were previously correctly synchronized by a lock.
>
> Ok, that's what I am writing right now.
>
>> I guess the best approach for deciding would be to try to convert a
>> couple of the existing unlocked accesses to the API and see what the
>> patch looks like.
>
> I don't think there really exist any interesting ones? I am using my
> lwlock reimplementation as a testbed so far.
BufferDesc management in src/backend/storage/buffer/bufmgr.c.
The seqlock like thing used to provide consistent reads from
PgBackendStatus src/backend/postmaster/pgstat.c (this code looks like
it's broken on weakly ordered machines)
The unlocked reads that are done from various procarray variables.
The XLogCtl accesses in xlog.c.
IMO the volatile keyword should be confined to the code actually
implementing atomics, locking. The "use volatile pointer to prevent
code rearrangement" comment is evil. If there are data races in the
code, they need comments pointing this out and probably memory
barriers too. If there are no data races, then the volatile
declaration is useless and a pessimization. Currently it's more of a
catch all "here be dragons" declaration for data structures.
Regards,
Ants Aasma
--
Cybertec Schönig & Schönig GmbH
Gröhrmühlgasse 26
A-2700 Wiener Neustadt
Web: http://www.postgresql-support.de