Avoid crashing when we have problems unlinking files post-commit.

smgrdounlink takes care to not throw an ERROR if it fails to unlink
something, but that caution was rendered useless by commit
3396000684b41e7e9467d1abc67152b39e697035, which put an smgrexists call in
front of it; smgrexists *does* throw error if anything looks funny, such
as getting a permissions error from trying to open the file.  If that
happens post-commit, you get a PANIC, and what's worse the same logic
appears in the WAL replay code, so the database even fails to restart.

Restore the intended behavior by removing the smgrexists call --- it isn't
accomplishing anything that we can't do better by adjusting mdunlink's
ideas of whether it ought to warn about ENOENT or not.

Per report from Joseph Shraibman of unrecoverable crash after trying to
drop a table whose FSM fork had somehow gotten chmod'd to 000 permissions.
Backpatch to 8.4, where the bogus coding was introduced.

diff --git a/src/backend/access/transam/twophase.c b/src/backend/access/transam/twophase.c
index 0d94697084b316f9eac96a784f36eb0103e26ff5..3be0a38cb27c24f5013028cfd62f8fc6faa6345f 100644 (file)
--- a/src/backend/access/transam/twophase.c
+++ b/src/backend/access/transam/twophase.c
@@ -1271,8 +1271,7 @@ FinishPreparedTransaction(const char *gid, bool isCommit)
 
                for (fork = 0; fork <= MAX_FORKNUM; fork++)
                {
-                       if (smgrexists(srel, fork))
-                               smgrdounlink(srel, fork, false, false);
+                       smgrdounlink(srel, fork, false, false);
                }
                smgrclose(srel);
        }

diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c
index cc20929fae6d67d10a8f38ca41322ad5b1de0c80..f2c5c1692713f0794377c190c6cfdad9d984b273 100644 (file)
--- a/src/backend/access/transam/xact.c
+++ b/src/backend/access/transam/xact.c
@@ -4270,11 +4270,8 @@ xact_redo_commit(xl_xact_commit *xlrec, TransactionId xid)
 
                for (fork = 0; fork <= MAX_FORKNUM; fork++)
                {
-                       if (smgrexists(srel, fork))
-                       {
-                               XLogDropRelation(xlrec->xnodes[i], fork);
-                               smgrdounlink(srel, fork, false, true);
-                       }
+                       XLogDropRelation(xlrec->xnodes[i], fork);
+                       smgrdounlink(srel, fork, false, true);
                }
                smgrclose(srel);
        }
@@ -4313,11 +4310,8 @@ xact_redo_abort(xl_xact_abort *xlrec, TransactionId xid)
 
                for (fork = 0; fork <= MAX_FORKNUM; fork++)
                {
-                       if (smgrexists(srel, fork))
-                       {
-                               XLogDropRelation(xlrec->xnodes[i], fork);
-                               smgrdounlink(srel, fork, false, true);
-                       }
+                       XLogDropRelation(xlrec->xnodes[i], fork);
+                       smgrdounlink(srel, fork, false, true);
                }
                smgrclose(srel);
        }

diff --git a/src/backend/catalog/storage.c b/src/backend/catalog/storage.c
index 67236ddd81f97a51f3f56c8b418f7d70f7204bf5..326e76f283e08430e0aa440987b50b2d962b8bf3 100644 (file)
--- a/src/backend/catalog/storage.c
+++ b/src/backend/catalog/storage.c
@@ -278,11 +278,10 @@ smgrDoPendingDeletes(bool isCommit)
                                srel = smgropen(pending->relnode);
                                for (i = 0; i <= MAX_FORKNUM; i++)
                                {
-                                       if (smgrexists(srel, i))
-                                               smgrdounlink(srel,
-                                                                        i,
-                                                                        pending->isTemp,
-                                                                        false);
+                                       smgrdounlink(srel,
+                                                                i,
+                                                                pending->isTemp,
+                                                                false);
                                }
                                smgrclose(srel);
                        }

diff --git a/src/backend/storage/smgr/md.c b/src/backend/storage/smgr/md.c
index 0c4861d6dbb6a8d4efd0ab859c234478e128e8ee..e69c7d3bb8a5b63033a3a53941103544f6e458f2 100644 (file)
--- a/src/backend/storage/smgr/md.c
+++ b/src/backend/storage/smgr/md.c
@@ -310,7 +310,13 @@ mdcreate(SMgrRelation reln, ForkNumber forkNum, bool isRedo)
  * number until it's safe, because relfilenode assignment skips over any
  * existing file.
  *
- * If isRedo is true, it's okay for the relation to be already gone.
+ * All the above applies only to the relation's main fork; other forks can
+ * just be removed immediately, since they are not needed to prevent the
+ * relfilenode number from being recycled.  Also, we do not carefully
+ * track whether other forks have been created or not, but just attempt to
+ * unlink them unconditionally; so we should never complain about ENOENT.
+ *
+ * If isRedo is true, it's unsurprising for the relation to be already gone.
  * Also, we should remove the file immediately instead of queuing a request
  * for later, since during redo there's no possibility of creating a
  * conflicting relation.
@@ -355,18 +361,15 @@ mdunlink(RelFileNode rnode, ForkNumber forkNum, bool isRedo)
                else
                        ret = -1;
        }
-       if (ret < 0)
-       {
-               if (!isRedo || errno != ENOENT)
-                       ereport(WARNING,
-                                       (errcode_for_file_access(),
-                                        errmsg("could not remove relation %s: %m", path)));
-       }
+       if (ret < 0 && errno != ENOENT)
+               ereport(WARNING,
+                               (errcode_for_file_access(),
+                                errmsg("could not remove relation %s: %m", path)));
 
        /*
         * Delete any additional segments.
         */
-       else
+       if (ret >= 0)
        {
                char       *segpath = (char *) palloc(strlen(path) + 12);
                BlockNumber segno;