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 d2fecb1ecb9171fc2c6dd2887d846c8a16779bb0..c5a2541807e148c29c790b0fe12a52c0389f4626 100644 (file)
--- a/src/backend/access/transam/twophase.c
+++ b/src/backend/access/transam/twophase.c
@@ -1366,8 +1366,7 @@ FinishPreparedTransaction(const char *gid, bool isCommit)
 
                for (fork = 0; fork <= MAX_FORKNUM; fork++)
                {
-                       if (smgrexists(srel, fork))
-                               smgrdounlink(srel, fork, false);
+                       smgrdounlink(srel, fork, false);
                }
                smgrclose(srel);
        }

diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c
index c383011b5f6538fa2b90fa5f7778da7ff59fa679..4177a6c5cf95d5e1a1b49b9f11c17098b2158529 100644 (file)
--- a/src/backend/access/transam/xact.c
+++ b/src/backend/access/transam/xact.c
@@ -4595,11 +4595,8 @@ xact_redo_commit_internal(TransactionId xid, XLogRecPtr lsn,
 
                for (fork = 0; fork <= MAX_FORKNUM; fork++)
                {
-                       if (smgrexists(srel, fork))
-                       {
-                               XLogDropRelation(xnodes[i], fork);
-                               smgrdounlink(srel, fork, true);
-                       }
+                       XLogDropRelation(xnodes[i], fork);
+                       smgrdounlink(srel, fork, true);
                }
                smgrclose(srel);
        }
@@ -4738,11 +4735,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, true);
-                       }
+                       XLogDropRelation(xlrec->xnodes[i], fork);
+                       smgrdounlink(srel, fork, true);
                }
                smgrclose(srel);
        }

diff --git a/src/backend/catalog/storage.c b/src/backend/catalog/storage.c
index fff933d403e7ca5e7b551b987e1684cd6a6e98a7..450e292f4013f9be5418f482654cd33c14c6d344 100644 (file)
--- a/src/backend/catalog/storage.c
+++ b/src/backend/catalog/storage.c
@@ -361,8 +361,7 @@ smgrDoPendingDeletes(bool isCommit)
                                srel = smgropen(pending->relnode, pending->backend);
                                for (i = 0; i <= MAX_FORKNUM; i++)
                                {
-                                       if (smgrexists(srel, i))
-                                               smgrdounlink(srel, i, false);
+                                       smgrdounlink(srel, i, false);
                                }
                                smgrclose(srel);
                        }

diff --git a/src/backend/storage/smgr/md.c b/src/backend/storage/smgr/md.c
index a761369d39c2f7aa4c71131d79ef6a0edc61b4ea..7150a00407e3b28dae1cca2e6063251f2c5b681c 100644 (file)
--- a/src/backend/storage/smgr/md.c
+++ b/src/backend/storage/smgr/md.c
@@ -323,7 +323,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.
@@ -351,13 +357,10 @@ mdunlink(RelFileNodeBackend rnode, ForkNumber forkNum, bool isRedo)
        if (isRedo || forkNum != MAIN_FORKNUM)
        {
                ret = unlink(path);
-               if (ret < 0)
-               {
-                       if (!isRedo || errno != ENOENT)
-                               ereport(WARNING,
-                                               (errcode_for_file_access(),
-                                                errmsg("could not remove file \"%s\": %m", path)));
-               }
+               if (ret < 0 && errno != ENOENT)
+                       ereport(WARNING,
+                                       (errcode_for_file_access(),
+                                        errmsg("could not remove file \"%s\": %m", path)));
        }
        else
        {
@@ -380,6 +383,9 @@ mdunlink(RelFileNodeBackend rnode, ForkNumber forkNum, bool isRedo)
                        ereport(WARNING,
                                        (errcode_for_file_access(),
                                         errmsg("could not truncate file \"%s\": %m", path)));
+
+               /* Register request to unlink first segment later */
+               register_unlink(rnode);
        }
 
        /*
@@ -411,10 +417,6 @@ mdunlink(RelFileNodeBackend rnode, ForkNumber forkNum, bool isRedo)
        }
 
        pfree(path);
-
-       /* Register request to unlink first segment later */
-       if (!isRedo && forkNum == MAIN_FORKNUM)
-               register_unlink(rnode);
 }
 
 /*