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 |
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 |