Fix two race conditions between the pending unlink mechanism that was put in

place to prevent reusing relation OIDs before next checkpoint, and DROP
DATABASE. First, if a database was dropped, bgwriter would still try to unlink
the files that the rmtree() call by the DROP DATABASE command has already
deleted, or is just about to delete. Second, if a database is dropped, and
another database is created with the same OID, bgwriter would in the worst
case delete a relation in the new database that happened to get the same OID
as a dropped relation in the old database.

To fix these race conditions:
- make rmtree() ignore ENOENT errors. This fixes the 1st race condition.
- make ForgetDatabaseFsyncRequests forget unlink requests as well.
- force checkpoint on in dropdb on all platforms

Since ForgetDatabaseFsyncRequests() is asynchronous, the 2nd change isn't
enough on its own to fix the problem of dropping and creating a database with
same OID, but forcing a checkpoint on DROP DATABASE makes it sufficient.

Per Tom Lane's bug report and proposal. Backpatch to 8.3.

diff --git a/src/backend/commands/dbcommands.c b/src/backend/commands/dbcommands.c
index 8b99fd11ba43c38d0f5da09cf50da2bf13f99519..9596bf4808158fd7a2665a6d508b1e8bbceb13e1 100644 (file)
--- a/src/backend/commands/dbcommands.c
+++ b/src/backend/commands/dbcommands.c
@@ -13,7 +13,7 @@
  *
  *
  * IDENTIFICATION
- *   $PostgreSQL: pgsql/src/backend/commands/dbcommands.c,v 1.204.2.1 2008/04/16 23:59:51 tgl Exp $
+ *   $PostgreSQL: pgsql/src/backend/commands/dbcommands.c,v 1.204.2.2 2008/04/18 06:48:50 heikki Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -714,18 +714,20 @@ dropdb(const char *dbname, bool missing_ok)
    pgstat_drop_database(db_id);
 
    /*
-    * Tell bgwriter to forget any pending fsync requests for files in the
-    * database; else it'll fail at next checkpoint.
+    * Tell bgwriter to forget any pending fsync and unlink requests for files
+    * in the database; else the fsyncs will fail at next checkpoint, or worse,
+    * it will delete files that belong to a newly created database with the
+    * same OID.
     */
    ForgetDatabaseFsyncRequests(db_id);
 
    /*
-    * On Windows, force a checkpoint so that the bgwriter doesn't hold any
-    * open files, which would cause rmdir() to fail.
+    * Force a checkpoint to make sure the bgwriter has received the message
+    * sent by ForgetDatabaseFsyncRequests. On Windows, this also ensures that
+    * the bgwriter doesn't hold any open files, which would cause rmdir() to
+    * fail.
     */
-#ifdef WIN32
    RequestCheckpoint(CHECKPOINT_IMMEDIATE | CHECKPOINT_FORCE | CHECKPOINT_WAIT);
-#endif
 
    /*
     * Remove all tablespace subdirs belonging to the database.

diff --git a/src/backend/storage/smgr/md.c b/src/backend/storage/smgr/md.c
index 543574be400309e48f78fce33355880f5929c2df..9b01fbdb2d0432cc9878332747624bbbadc763b8 100644 (file)
--- a/src/backend/storage/smgr/md.c
+++ b/src/backend/storage/smgr/md.c
@@ -8,7 +8,7 @@
  *
  *
  * IDENTIFICATION
- *   $PostgreSQL: pgsql/src/backend/storage/smgr/md.c,v 1.135 2008/01/01 19:45:52 momjian Exp $
+ *   $PostgreSQL: pgsql/src/backend/storage/smgr/md.c,v 1.135.2.1 2008/04/18 06:48:50 heikki Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -1196,8 +1196,11 @@ mdpostckpt(void)
        if (unlink(path) < 0)
        {
            /*
-            * ENOENT shouldn't happen either, but it doesn't really matter
-            * because we would've deleted it now anyway.
+            * There's a race condition, when the database is dropped at the
+            * same time that we process the pending unlink requests. If the
+            * DROP DATABASE deletes the file before we do, we will get ENOENT
+            * here. rmtree() also has to ignore ENOENT errors, to deal with
+            * the possibility that we delete the file first.
             */
            if (errno != ENOENT)
                ereport(WARNING,
@@ -1321,7 +1324,11 @@ RememberFsyncRequest(RelFileNode rnode, BlockNumber segno)
        /* Remove any pending requests for the entire database */
        HASH_SEQ_STATUS hstat;
        PendingOperationEntry *entry;
+       ListCell   *cell, 
+                  *prev,
+                  *next;
 
+       /* Remove fsync requests */
        hash_seq_init(&hstat, pendingOpsTable);
        while ((entry = (PendingOperationEntry *) hash_seq_search(&hstat)) != NULL)
        {
@@ -1331,6 +1338,22 @@ RememberFsyncRequest(RelFileNode rnode, BlockNumber segno)
                entry->canceled = true;
            }
        }
+   
+       /* Remove unlink requests */
+       prev = NULL;
+       for (cell = list_head(pendingUnlinks); cell; cell = next)
+       {
+           PendingUnlinkEntry *entry = (PendingUnlinkEntry *) lfirst(cell);
+
+           next = lnext(cell);
+           if (entry->rnode.dbNode == rnode.dbNode) 
+           {
+               pendingUnlinks = list_delete_cell(pendingUnlinks, cell, prev);
+               pfree(entry);
+           }
+           else
+               prev = cell;
+       }
    }
    else if (segno == UNLINK_RELATION_REQUEST)
    {
@@ -1386,7 +1409,7 @@ RememberFsyncRequest(RelFileNode rnode, BlockNumber segno)
 }
 
 /*
- * ForgetRelationFsyncRequests -- ensure any fsyncs for a rel are forgotten
+ * ForgetRelationFsyncRequests -- forget any fsyncs for a rel
  */
 void
 ForgetRelationFsyncRequests(RelFileNode rnode)
@@ -1419,7 +1442,7 @@ ForgetRelationFsyncRequests(RelFileNode rnode)
 }
 
 /*
- * ForgetDatabaseFsyncRequests -- ensure any fsyncs for a DB are forgotten
+ * ForgetDatabaseFsyncRequests -- forget any fsyncs and unlinks for a DB
  */
 void
 ForgetDatabaseFsyncRequests(Oid dbid)

diff --git a/src/port/dirmod.c b/src/port/dirmod.c
index e9f5c42ab912119b6936af1486244023488c3f6e..0138be89d4ec488ba41bef292ebff5ca1f182c5d 100644 (file)
--- a/src/port/dirmod.c
+++ b/src/port/dirmod.c
@@ -10,7 +10,7 @@
  * Win32 (NT, Win2k, XP).  replace() doesn't work on Win95/98/Me.
  *
  * IDENTIFICATION
- *   $PostgreSQL: pgsql/src/port/dirmod.c,v 1.51.2.2 2008/04/11 23:59:49 tgl Exp $
+ *   $PostgreSQL: pgsql/src/port/dirmod.c,v 1.51.2.3 2008/04/18 06:48:50 heikki Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -406,8 +406,24 @@ rmtree(char *path, bool rmtopdir)
    {
        snprintf(filepath, MAXPGPATH, "%s/%s", path, *filename);
 
+       /*
+        * It's ok if the file is not there anymore; we were just about to
+        * delete it anyway.
+        *
+        * This is not an academic possibility. One scenario where this
+        * happens is when bgwriter has a pending unlink request for a file
+        * in a database that's being dropped. In dropdb(), we call
+        * ForgetDatabaseFsyncRequests() to flush out any such pending unlink
+        * requests, but because that's asynchronous, it's not guaranteed
+        * that the bgwriter receives the message in time.
+        */
        if (lstat(filepath, &statbuf) != 0)
-           goto report_and_fail;
+       {
+           if (errno != ENOENT)
+               goto report_and_fail;
+           else
+               continue;
+       }
 
        if (S_ISDIR(statbuf.st_mode))
        {
@@ -422,7 +438,10 @@ rmtree(char *path, bool rmtopdir)
        else
        {
            if (unlink(filepath) != 0)
-               goto report_and_fail;
+           {
+               if (errno != ENOENT)
+                   goto report_and_fail;
+           }
        }
    }