Fix failure to guarantee that a checkpoint will write out pg_clog updates

for transaction commits that occurred just before the checkpoint.  This is
an EXTREMELY serious bug --- kudos to Satoshi Okada for creating a
reproducible test case to prove its existence.

diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c
index 4a765998749de19e967894671fbfb4f11e9b0deb..b057a3d0909828c83511200031de3d8f798a834a 100644 (file)
--- a/src/backend/access/transam/xact.c
+++ b/src/backend/access/transam/xact.c
@@ -503,6 +503,7 @@ RecordTransactionCommit(void)
        if (MyXactMadeXLogEntry || MyXactMadeTempRelUpdate)
        {
                TransactionId xid = GetCurrentTransactionId();
+               bool            madeTCentries;
                XLogRecPtr      recptr;
 
                /* Tell bufmgr and smgr to prepare for commit */
@@ -511,12 +512,29 @@ RecordTransactionCommit(void)
                START_CRIT_SECTION();
 
                /*
-                * We only need to log the commit in xlog if the transaction made
-                * any transaction-controlled XLOG entries.  (Otherwise, its XID
-                * appears nowhere in permanent storage, so no one else will ever
-                * care if it committed.)
+                * If our transaction made any transaction-controlled XLOG entries,
+                * we need to lock out checkpoint start between writing our XLOG
+                * record and updating pg_clog.  Otherwise it is possible for the
+                * checkpoint to set REDO after the XLOG record but fail to flush the
+                * pg_clog update to disk, leading to loss of the transaction commit
+                * if we crash a little later.  Slightly klugy fix for problem
+                * discovered 2004-08-10.
+                *
+                * (If it made no transaction-controlled XLOG entries, its XID
+                * appears nowhere in permanent storage, so no one else will ever care
+                * if it committed; so it doesn't matter if we lose the commit flag.)
+                *
+                * Note we only need a shared lock.
                 */
-               if (MyLastRecPtr.xrecoff != 0)
+               madeTCentries = (MyLastRecPtr.xrecoff != 0);
+               if (madeTCentries)
+                       LWLockAcquire(CheckpointStartLock, LW_SHARED);
+
+               /*
+                * We only need to log the commit in XLOG if the transaction made
+                * any transaction-controlled XLOG entries.
+                */
+               if (madeTCentries)
                {
                        /* Need to emit a commit record */
                        XLogRecData rdata;
@@ -585,6 +603,10 @@ RecordTransactionCommit(void)
                if (MyLastRecPtr.xrecoff != 0 || MyXactMadeTempRelUpdate)
                        TransactionIdCommit(xid);
 
+               /* Unlock checkpoint lock if we acquired it */
+               if (madeTCentries)
+                       LWLockRelease(CheckpointStartLock);
+
                END_CRIT_SECTION();
        }
 
@@ -698,6 +720,8 @@ RecordTransactionAbort(void)
                 * care if it committed.)  We do not flush XLOG to disk in any
                 * case, since the default assumption after a crash would be that
                 * we aborted, anyway.
+                * For the same reason, we don't need to worry about interlocking
+                * against checkpoint start.
                 */
                if (MyLastRecPtr.xrecoff != 0)
                {

diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 97d2e9a2332f258971af448a0e46988a6ea2666e..5fb188e67e4bca118773650f0c80be6104404905 100644 (file)
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -3159,6 +3159,15 @@ CreateCheckPoint(bool shutdown, bool force)
        checkPoint.ThisStartUpID = ThisStartUpID;
        checkPoint.time = time(NULL);
 
+       /*
+        * We must hold CheckpointStartLock while determining the checkpoint
+        * REDO pointer.  This ensures that any concurrent transaction commits
+        * will be either not yet logged, or logged and recorded in pg_clog.
+        * See notes in RecordTransactionCommit().
+        */
+       LWLockAcquire(CheckpointStartLock, LW_EXCLUSIVE);
+
+       /* And we need WALInsertLock too */
        LWLockAcquire(WALInsertLock, LW_EXCLUSIVE);
 
        /*
@@ -3191,6 +3200,7 @@ CreateCheckPoint(bool shutdown, bool force)
                        ControlFile->checkPointCopy.redo.xrecoff)
                {
                        LWLockRelease(WALInsertLock);
+                       LWLockRelease(CheckpointStartLock);
                        LWLockRelease(CheckpointLock);
                        END_CRIT_SECTION();
                        return;
@@ -3258,11 +3268,13 @@ CreateCheckPoint(bool shutdown, bool force)
 #endif
 
        /*
-        * Now we can release insert lock, allowing other xacts to proceed
-        * even while we are flushing disk buffers.
+        * Now we can release insert lock and checkpoint start lock, allowing
+        * other xacts to proceed even while we are flushing disk buffers.
         */
        LWLockRelease(WALInsertLock);
 
+       LWLockRelease(CheckpointStartLock);
+
        /*
         * Get the other info we need for the checkpoint record.
         */

diff --git a/src/include/storage/lwlock.h b/src/include/storage/lwlock.h
index 353eb43d4b58c7f208479d6b6c8ea749aed017c2..c88244aba52679433c21a06a5d984a25a1c7df1b 100644 (file)
--- a/src/include/storage/lwlock.h
+++ b/src/include/storage/lwlock.h
@@ -37,6 +37,7 @@ typedef enum LWLockId
        WALWriteLock,
        ControlFileLock,
        CheckpointLock,
+       CheckpointStartLock,
        RelCacheInitLock,
 
        NumFixedLWLocks,                        /* must be last except for