Fix race between DROP TABLESPACE and checkpointing.

Commands like ALTER TABLE SET TABLESPACE may leave files for the next
checkpoint to clean up.  If such files are not removed by the time DROP
TABLESPACE is called, we request a checkpoint so that they are deleted.
However, there is presently a window before checkpoint start where new
unlink requests won't be scheduled until the following checkpoint.  This
means that the checkpoint forced by DROP TABLESPACE might not remove the
files we expect it to remove, and the following ERROR will be emitted:

	ERROR:  tablespace "mytblspc" is not empty

To fix, add a call to AbsorbSyncRequests() just before advancing the
unlink cycle counter.  This ensures that any unlink requests forwarded
prior to checkpoint start (i.e., when ckpt_started is incremented) will
be processed by the current checkpoint.  Since AbsorbSyncRequests()
performs memory allocations, it cannot be called within a critical
section, so we also need to move SyncPreCheckpoint() to before
CreateCheckPoint()'s critical section.

This is an old bug, so back-patch to all supported versions.

Author: Nathan Bossart <nathandbossart@gmail.com>
Reported-by: Nathan Bossart <nathandbossart@gmail.com>
Reviewed-by: Thomas Munro <thomas.munro@gmail.com>
Reviewed-by: Andres Freund <andres@anarazel.de>
Discussion: https://postgr.es/m/20220215235845.GA2665318%40nathanxps13

diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index c64febdb53d77ab591293abe08639b71b2211c56..8e8bdde7646f728326363da21c0b8721dde925e9 100644 (file)
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -8811,6 +8811,14 @@ CreateCheckPoint(int flags)
        MemSet(&CheckpointStats, 0, sizeof(CheckpointStats));
        CheckpointStats.ckpt_start_t = GetCurrentTimestamp();
 
+       /*
+        * Let smgr prepare for checkpoint; this has to happen outside the
+        * critical section and before we determine the REDO pointer.  Note that
+        * smgr must not do anything that'd have to be undone if we decide no
+        * checkpoint is needed.
+        */
+       smgrpreckpt();
+
        /*
         * Use a critical section to force system panic if we have trouble.
         */
@@ -8825,13 +8833,6 @@ CreateCheckPoint(int flags)
                LWLockRelease(ControlFileLock);
        }
 
-       /*
-        * Let smgr prepare for checkpoint; this has to happen before we determine
-        * the REDO pointer.  Note that smgr must not do anything that'd have to
-        * be undone if we decide no checkpoint is needed.
-        */
-       smgrpreckpt();
-
        /* Begin filling in the checkpoint WAL record */
        MemSet(&checkPoint, 0, sizeof(checkPoint));
        checkPoint.time = (pg_time_t) time(NULL);

diff --git a/src/backend/storage/smgr/md.c b/src/backend/storage/smgr/md.c
index 8c98a11435477e341d4100552d6d712d64e67b5b..bfce29371fbbf1aceef1cbfe1b9874eaaf615331 100644 (file)
--- a/src/backend/storage/smgr/md.c
+++ b/src/backend/storage/smgr/md.c
@@ -1385,7 +1385,9 @@ mdsync(void)
  * counter is incremented here.
  *
  * This must be called *before* the checkpoint REDO point is determined.
- * That ensures that we won't delete files too soon.
+ * That ensures that we won't delete files too soon.  Since this calls
+ * AbsorbFsyncRequests(), which performs memory allocations, it cannot be
+ * called within a critical section.
  *
  * Note that we can't do anything here that depends on the assumption
  * that the checkpoint will be completed.
@@ -1393,6 +1395,16 @@ mdsync(void)
 void
 mdpreckpt(void)
 {
+       /*
+        * Operations such as DROP TABLESPACE assume that the next checkpoint will
+        * process all recently forwarded unlink requests, but if they aren't
+        * absorbed prior to advancing the cycle counter, they won't be processed
+        * until a future checkpoint.  The following absorb ensures that any
+        * unlink requests forwarded before the checkpoint began will be processed
+        * in the current checkpoint.
+        */
+       AbsorbFsyncRequests();
+
        /*
         * Any unlink requests arriving after this point will be assigned the next
         * cycle counter, and won't be unlinked until next checkpoint.