Re: Is this non-volatile pointer access OK?

Peter Geoghegan <peter@2ndquadrant.com> writes:
> On 3 September 2012 08:10, Daniel Farina <daniel@heroku.com> wrote:
>> On line 8197 of xlog.c:
>> 
>> 08194     /* Get a local copy of the last safe checkpoint record. */
>> 08195     SpinLockAcquire(&xlogctl->info_lck);
>> 08196     lastCheckPointRecPtr = xlogctl->lastCheckPointRecPtr;
>> 08197     memcpy(&lastCheckPoint, &XLogCtl->lastCheckPoint, sizeof(CheckPoint));
>> 08198     SpinLockRelease(&xlogctl->info_lck);
>> 
>> Note the use of capital XLogCtl->lastCheckPoint, which is not the
>> volatile pointer.

> That looks like a bug to me.

The problem with s/XLogCtl/xlogctl/ there is that then the compiler
warns about passing a volatile pointer to memcpy.  I seem to recall
we discussed this once before and decided to leave it alone.

I experimented just now with replacing the memcpy with struct
assignment, here and in the other place where xlog.c does this
(see attached patch).  I don't get a complaint from my versions of
gcc, although it's not entirely clear why not, since I doubt the
assembly code for struct assignment is any more atomic than memcpy
would be.  According to
http://archives.postgresql.org/pgsql-patches/2007-07/msg00025.php
g++ *does* complain about that.

Anyway, since we're already depending on struct assignment for
XLogRecPtr (in the back branches anyway), I don't see any very good
reason not to depend on it for struct CheckPoint as well, and so
propose that we apply the attached.

> Come to think of it, the whole convention of using a lower-case
> variant of the original pointer variable name seems like a foot-gun,
> given the harmful and indeed very subtle consequences of making this
> error.

Yes.  The right way to fix this would be for the compiler to not ever
move assignments across a SpinLockAcquire or SpinLockRelease.  Do you
have a bulletproof method for guaranteeing that?

			regards, tom lane