Re: Is this non-volatile pointer access OK?

From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: Peter Geoghegan <peter(at)2ndquadrant(dot)com>
Cc: Daniel Farina <daniel(at)heroku(dot)com>, pgsql-hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: Is this non-volatile pointer access OK?
Date: 2012-09-03 15:45:54
Message-ID: 29770.1346687154@sss.pgh.pa.us
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

Peter Geoghegan <peter(at)2ndquadrant(dot)com> writes:
> On 3 September 2012 08:10, Daniel Farina <daniel(at)heroku(dot)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

Attachment Content-Type Size
volatile-ref-fixes.patch text/x-patch 1.5 KB

In response to

Browse pgsql-hackers by date

  From Date Subject
Next Message Magnus Hagander 2012-09-03 16:06:29 Re: [COMMITTERS] pgsql: Make a cut at a major-features list for 9.2.
Previous Message Bruce Momjian 2012-09-03 15:21:40 Re: Yet another failure mode in pg_upgrade