Peter Geoghegan <pe...@2ndquadrant.com> writes: > On 3 September 2012 08:10, Daniel Farina <dan...@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); /*
-- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
