Thread: pgsql: Do all accesses to shared buffer headers through

Log Message:
-----------
Do all accesses to shared buffer headers through volatile-qualified
pointers, to ensure that compilers won't rearrange accesses to occur
while we're not holding the buffer header spinlock.  It's probably
not necessary to mark volatile in every single place in bufmgr.c,
but better safe than sorry.  Per trouble report from Kevin Grittner.

Modified Files:
--------------
    pgsql/contrib/pg_buffercache:
        pg_buffercache_pages.c (r1.4 -> r1.5)

(http://developer.postgresql.org/cvsweb.cgi/pgsql/contrib/pg_buffercache/pg_buffercache_pages.c.diff?r1=1.4&r2=1.5)
    pgsql/src/backend/storage/buffer:
        bufmgr.c (r1.195 -> r1.196)
        (http://developer.postgresql.org/cvsweb.cgi/pgsql/src/backend/storage/buffer/bufmgr.c.diff?r1=1.195&r2=1.196)
        freelist.c (r1.52 -> r1.53)
        (http://developer.postgresql.org/cvsweb.cgi/pgsql/src/backend/storage/buffer/freelist.c.diff?r1=1.52&r2=1.53)
    pgsql/src/include/storage:
        buf_internals.h (r1.79 -> r1.80)
        (http://developer.postgresql.org/cvsweb.cgi/pgsql/src/include/storage/buf_internals.h.diff?r1=1.79&r2=1.80)

On Wed, 2005-12-10 at 13:45 -0300, Tom Lane wrote:
> Do all accesses to shared buffer headers through volatile-qualified
> pointers, to ensure that compilers won't rearrange accesses to occur
> while we're not holding the buffer header spinlock.

That is fairly error prone :-( Would it be possible to hide this in a
typedef?

-Neil

Neil Conway <neilc@samurai.com> writes:
> On Wed, 2005-12-10 at 13:45 -0300, Tom Lane wrote:
>> Do all accesses to shared buffer headers through volatile-qualified
>> pointers, to ensure that compilers won't rearrange accesses to occur
>> while we're not holding the buffer header spinlock.

> That is fairly error prone :-( Would it be possible to hide this in a
> typedef?

How would a typedef make it safer?  I see no particular difference
between omitting the "volatile" and choosing the wrong typedef.
Neither will happen if you follow the coding style of surrounding
routines (which is one reason I chose to make *all* the bufmgr routines
declare BufferDesc pointers as volatile, even though some of them
arguably didn't need to).  If you don't follow the coding style,
neither convention will catch you at it.

We do however have here a New Coding Rule that's good for all parts
of the backend: if you are accessing a spinlock-protected data structure
then you should be using a volatile-qualified pointer for it.  I had
at one time thought that volatile-qualifying the spinlock pointer should
be enough, on the grounds that compilers shouldn't move other stores
across a store to volatile.  But apparently gcc only promises not to
rearrange volatile stores with respect to each other.

            regards, tom lane

On Wed, 2005-12-10 at 18:54 -0400, Tom Lane wrote:
> How would a typedef make it safer?  I see no particular difference
> between omitting the "volatile" and choosing the wrong typedef.

IMHO it is notationally clearer to define a "BufferDescPtr" that
contains the "volatile" qualifier than to make sure that "volatile" is
used everywhere that it is needed -- obviously, neither approach is
fool-proof. But perhaps that's just me...

> We do however have here a New Coding Rule that's good for all parts
> of the backend: if you are accessing a spinlock-protected data structure
> then you should be using a volatile-qualified pointer for it.

I think this is worth documenting more clearly (I realize you added a
note in buf_internals.h, but perhaps a note in the spinlock headers
would be appropriate as well? The comment circa line 49 of s_lock.h
seems to need updating, for example.)

-Neil

Neil Conway <neilc@samurai.com> writes:
> IMHO it is notationally clearer to define a "BufferDescPtr" that
> contains the "volatile" qualifier than to make sure that "volatile" is
> used everywhere that it is needed -- obviously, neither approach is
> fool-proof. But perhaps that's just me...

Personally I dislike hiding type qualifiers like const and volatile
inside typedefs.  I agree it's a judgment call.

            regards, tom lane

Neil Conway <neilc@samurai.com> writes:
> On Wed, 2005-12-10 at 18:54 -0400, Tom Lane wrote:
>> We do however have here a New Coding Rule that's good for all parts
>> of the backend: if you are accessing a spinlock-protected data structure
>> then you should be using a volatile-qualified pointer for it.

> I think this is worth documenting more clearly

BTW, forgot to reply to this part, but: have at it.

            regards, tom lane

On Thu, 2005-13-10 at 00:11 -0400, Tom Lane wrote:
> BTW, forgot to reply to this part, but: have at it.

I've committed the attached patch; feel free to improve it if you're not
happy.

-Neil