Thread: Is this non-volatile pointer access OK?

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

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

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

diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index a7762eafcd0af2a6069a52d763e8fdbe6f65dafe..70b2e1cbeb8272b836f5b451f3c00568f4d8de16 100644
*** a/src/backend/access/transam/xlog.c
--- b/src/backend/access/transam/xlog.c
*************** RecoveryRestartPoint(const CheckPoint *c
*** 8158,8165 ****
       * work out the next time it wants to perform a restartpoint.
       */
      SpinLockAcquire(&xlogctl->info_lck);
!     XLogCtl->lastCheckPointRecPtr = ReadRecPtr;
!     memcpy(&XLogCtl->lastCheckPoint, checkPoint, sizeof(CheckPoint));
      SpinLockRelease(&xlogctl->info_lck);
  }

--- 8158,8165 ----
       * work out the next time it wants to perform a restartpoint.
       */
      SpinLockAcquire(&xlogctl->info_lck);
!     xlogctl->lastCheckPointRecPtr = ReadRecPtr;
!     xlogctl->lastCheckPoint = *checkPoint;
      SpinLockRelease(&xlogctl->info_lck);
  }

*************** CreateRestartPoint(int flags)
*** 8194,8200 ****
      /* Get a local copy of the last safe checkpoint record. */
      SpinLockAcquire(&xlogctl->info_lck);
      lastCheckPointRecPtr = xlogctl->lastCheckPointRecPtr;
!     memcpy(&lastCheckPoint, &XLogCtl->lastCheckPoint, sizeof(CheckPoint));
      SpinLockRelease(&xlogctl->info_lck);

      /*
--- 8194,8200 ----
      /* Get a local copy of the last safe checkpoint record. */
      SpinLockAcquire(&xlogctl->info_lck);
      lastCheckPointRecPtr = xlogctl->lastCheckPointRecPtr;
!     lastCheckPoint = xlogctl->lastCheckPoint;
      SpinLockRelease(&xlogctl->info_lck);

      /*

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