Thread

1. Is this non-volatile pointer access OK?

   Daniel Farina <daniel@heroku.com> — 2012-09-03T07:10:58Z

   http://doxygen.postgresql.org/xlog_8c_source.html#l08197
   
   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.
   
   I found this while scouring the code trying to figure out Bug #6291,
   which is the (to my latest knowledge) is when the epoch is not
   incremented (sometimes) when passing wraparound.
   
   -- 
   fdr
   
   
   

2. Re: Is this non-volatile pointer access OK?

   Peter Geoghegan <peter@2ndquadrant.com> — 2012-09-03T10:14:58Z

   On 3 September 2012 08:10, Daniel Farina <daniel@heroku.com> wrote:
   > http://doxygen.postgresql.org/xlog_8c_source.html#l08197
   >
   > 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.
   
   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.
   
   I count 98 SpinLockAcquire() call sites (of which only a minority use
   this convention, which is mostly within xlog.c, I think). Is it worth
   instituting an alternative convention to make this kind of misuse more
   obvious? This went unnoticed since February 2009.
   
   -- 
   Peter Geoghegan       http://www.2ndQuadrant.com/
   PostgreSQL Development, 24x7 Support, Training and Services
   
   
   

3. Re: Is this non-volatile pointer access OK?

   Tom Lane <tgl@sss.pgh.pa.us> — 2012-09-03T15:45:54Z

   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
   
   

4. Re: Is this non-volatile pointer access OK?

   Peter Eisentraut <peter_e@gmx.net> — 2012-09-04T01:21:11Z

   On Mon, 2012-09-03 at 11:14 +0100, Peter Geoghegan wrote:
   > 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.
   
   With some inventive macro magic, you could probably make this safer.
   I'm thinking something along the lines of replacing
   
   SpinLockAcquire(&xlogctl->info_lck);
   
   with
   
   SpinLockAcquire(XLogCtl, info_lck);
   
   which expands to
   
   {
       volatile typeof(XLogCtl) *XLogCtl_volatile = XLogCtl;
       void *XLogCtl = NULL;  // compiler error or crash at run time if used
       OldSpinLockAcquire(XLogCtl_volatile->info_lock);
       ...
   
   and then something corresponding for SpinLockRelease.
   
   This will likely only work with modern compilers, but it could give you
   some amount of static checking against this problem.