Re: Is this non-volatile pointer access OK? - Mailing list pgsql-hackers

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

      /*