Remove CheckpointLock.

Up until now, we've held this lock when performing a checkpoint or
restartpoint, but commit 076a055acf3c55314de267c62b03191586d79cf6 back
in 2004 and commit 7e48b77b1cebb9a43f9fdd6b17128a0ba36132f9 from 2009,
taken together, have removed all need for this. In the present code,
there's only ever one process entitled to attempt a checkpoint: either
the checkpointer, during normal operation, or the postmaster, during
single-user operation. So, we don't need the lock.

One possible concern in making this change is that it means that
a substantial amount of code where HOLD_INTERRUPTS() was previously
in effect due to the preceding LWLockAcquire() will now be
running without that. This could mean that ProcessInterrupts()
gets called in places from which it didn't before. However, this
seems unlikely to do very much, because the checkpointer doesn't
have any signal mapped to die(), so it's not clear how,
for example, ProcDiePending = true could happen in the first
place. Similarly with ClientConnectionLost and recovery conflicts.

Also, if there are any such problems, we might want to fix them
rather than reverting this, since running lots of code with
interrupt handling suspended is generally bad.

Patch by me, per an inquiry by Amul Sul. Review by Tom Lane
and Michael Paquier.

Discussion: http://postgr.es/m/CAAJ_b97XnBBfYeSREDJorFsyoD1sHgqnNuCi=02mNQBUMnA=FA@mail.gmail.com

diff --git a/doc/src/sgml/monitoring.sgml b/doc/src/sgml/monitoring.sgml
index f05140dd4240037fec02c4c3118629180914627c..9496f76b1fb0e3c7799b0246af194caa5213dc6f 100644 (file)
--- a/doc/src/sgml/monitoring.sgml
+++ b/doc/src/sgml/monitoring.sgml
@@ -1880,10 +1880,6 @@ postgres   27093  0.0  0.0  30096  2752 ?        Ss   11:34   0:00 postgres: ser
       <entry>Waiting to associate a data block with a buffer in the buffer
        pool.</entry>
      </row>
-     <row>
-      <entry><literal>Checkpoint</literal></entry>
-      <entry>Waiting to begin a checkpoint.</entry>
-     </row>
      <row>
       <entry><literal>CheckpointerComm</literal></entry>
       <entry>Waiting to manage fsync requests.</entry>

diff --git a/src/backend/access/heap/rewriteheap.c b/src/backend/access/heap/rewriteheap.c
index 29ffe4067042c1d9b2290e0534af77d01c7fe81f..fcaad9ba0b7f7519a88b280f05352f8c77b5f390 100644 (file)
--- a/src/backend/access/heap/rewriteheap.c
+++ b/src/backend/access/heap/rewriteheap.c
@@ -1256,8 +1256,8 @@ CheckPointLogicalRewriteHeap(void)
 
            /*
             * The file cannot vanish due to concurrency since this function
-            * is the only one removing logical mappings and it's run while
-            * CheckpointLock is held exclusively.
+            * is the only one removing logical mappings and only one
+            * checkpoint can be in progress at a time.
             */
            if (fd < 0)
                ereport(ERROR,

diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 470e113b3319932d49c1281c763be54322c6dccf..cc007b8963ef34b750c6e12a3a40194c594205bc 100644 (file)
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -430,10 +430,6 @@ static XLogRecPtr RedoStartLSN = InvalidXLogRecPtr;
  * ControlFileLock: must be held to read/update control file or create
  * new log file.
  *
- * CheckpointLock: must be held to do a checkpoint or restartpoint (ensures
- * only one checkpointer at a time; currently, with all checkpoints done by
- * the checkpointer, this is just pro forma).
- *
  *----------
  */
 
@@ -8864,14 +8860,6 @@ CreateCheckPoint(int flags)
     */
    InitXLogInsert();
 
-   /*
-    * Acquire CheckpointLock to ensure only one checkpoint happens at a time.
-    * (This is just pro forma, since in the present system structure there is
-    * only one process that is allowed to issue checkpoints at any given
-    * time.)
-    */
-   LWLockAcquire(CheckpointLock, LW_EXCLUSIVE);
-
    /*
     * Prepare to accumulate statistics.
     *
@@ -8941,7 +8929,6 @@ CreateCheckPoint(int flags)
        if (last_important_lsn == ControlFile->checkPoint)
        {
            WALInsertLockRelease();
-           LWLockRelease(CheckpointLock);
            END_CRIT_SECTION();
            ereport(DEBUG1,
                    (errmsg("checkpoint skipped because system is idle")));
@@ -9241,15 +9228,12 @@ CreateCheckPoint(int flags)
                                     CheckpointStats.ckpt_segs_added,
                                     CheckpointStats.ckpt_segs_removed,
                                     CheckpointStats.ckpt_segs_recycled);
-
-   LWLockRelease(CheckpointLock);
 }
 
 /*
  * Mark the end of recovery in WAL though without running a full checkpoint.
  * We can expect that a restartpoint is likely to be in progress as we
- * do this, though we are unwilling to wait for it to complete. So be
- * careful to avoid taking the CheckpointLock anywhere here.
+ * do this, though we are unwilling to wait for it to complete.
  *
  * CreateRestartPoint() allows for the case where recovery may end before
  * the restartpoint completes so there is no concern of concurrent behaviour.
@@ -9399,12 +9383,6 @@ CreateRestartPoint(int flags)
    XLogSegNo   _logSegNo;
    TimestampTz xtime;
 
-   /*
-    * Acquire CheckpointLock to ensure only one restartpoint or checkpoint
-    * happens at a time.
-    */
-   LWLockAcquire(CheckpointLock, LW_EXCLUSIVE);
-
    /* Get a local copy of the last safe checkpoint record. */
    SpinLockAcquire(&XLogCtl->info_lck);
    lastCheckPointRecPtr = XLogCtl->lastCheckPointRecPtr;
@@ -9420,7 +9398,6 @@ CreateRestartPoint(int flags)
    {
        ereport(DEBUG2,
                (errmsg("skipping restartpoint, recovery has already ended")));
-       LWLockRelease(CheckpointLock);
        return false;
    }
 
@@ -9455,7 +9432,6 @@ CreateRestartPoint(int flags)
            UpdateControlFile();
            LWLockRelease(ControlFileLock);
        }
-       LWLockRelease(CheckpointLock);
        return false;
    }
 
@@ -9621,8 +9597,6 @@ CreateRestartPoint(int flags)
             xtime ? errdetail("Last completed transaction was at log time %s.",
                               timestamptz_to_str(xtime)) : 0));
 
-   LWLockRelease(CheckpointLock);
-
    /*
     * Finally, execute archive_cleanup_command, if any.
     */

diff --git a/src/backend/replication/logical/origin.c b/src/backend/replication/logical/origin.c
index 77781d059dddeb5250b9e1bb6c437c1c78aa2e27..9bd761a426223805f6a92d0ebde701810cd01f7e 100644 (file)
--- a/src/backend/replication/logical/origin.c
+++ b/src/backend/replication/logical/origin.c
@@ -559,8 +559,8 @@ CheckPointReplicationOrigin(void)
                        tmppath)));
 
    /*
-    * no other backend can perform this at the same time, we're protected by
-    * CheckpointLock.
+    * no other backend can perform this at the same time; only one
+    * checkpoint can happen at a time.
     */
    tmpfd = OpenTransientFile(tmppath,
                              O_CREAT | O_EXCL | O_WRONLY | PG_BINARY);

diff --git a/src/backend/storage/lmgr/lwlocknames.txt b/src/backend/storage/lmgr/lwlocknames.txt
index 774292fd942774b87f11dbeebb76f6c5e64f02f2..6c7cf6c2956613df1dab332189ed89195afdba21 100644 (file)
--- a/src/backend/storage/lmgr/lwlocknames.txt
+++ b/src/backend/storage/lmgr/lwlocknames.txt
@@ -15,7 +15,7 @@ SInvalWriteLock                       6
 WALBufMappingLock                  7
 WALWriteLock                       8
 ControlFileLock                        9
-CheckpointLock                     10
+# 10 was CheckpointLock
 XactSLRULock                       11
 SubtransSLRULock                   12
 MultiXactGenLock                   13