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

tglsfdc · tglsfdc · commit 61dd2ffaff1b · 2011-12-20T15:00:47.000-05:00
smgrdounlink takes care to not throw an ERROR if it fails to unlink something, but that caution was rendered useless by commit 3396000, 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
@@ -1325,8 +1325,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
@@ -4479,11 +4479,8 @@ xact_redo_commit(xl_xact_commit *xlrec, TransactionId xid, XLogRecPtr lsn)
 
 		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);
 	}
@@ -4584,11 +4581,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
@@ -325,11 +325,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
@@ -312,7 +312,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.
@@ -340,13 +346,10 @@ mdunlink(RelFileNode 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
 	{
@@ -369,6 +372,9 @@ mdunlink(RelFileNode 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);
 	}
 
 	/*
@@ -400,10 +406,6 @@ mdunlink(RelFileNode rnode, ForkNumber forkNum, bool isRedo)
 	}
 
 	pfree(path);
-
-	/* Register request to unlink first segment later */
-	if (!isRedo && forkNum == MAIN_FORKNUM)
-		register_unlink(rnode);
 }
 
 /*

Original file line number	Diff line number	Diff line change
`@@ -1325,8 +1325,7 @@ FinishPreparedTransaction(const char *gid, bool isCommit)`
`1325`	`1325`
`1326`	`1326`	`for (fork = 0; fork <= MAX_FORKNUM; fork++)`
`1327`	`1327`	`{`
`1328`		`- if (smgrexists(srel, fork))`
`1329`		`- smgrdounlink(srel, fork, false, false);`
	`1328`	`+ smgrdounlink(srel, fork, false, false);`
`1330`	`1329`	`}`
`1331`	`1330`	`smgrclose(srel);`
`1332`	`1331`	`}`
Original file line number	Diff line number	Diff line change
`@@ -4479,11 +4479,8 @@ xact_redo_commit(xl_xact_commit *xlrec, TransactionId xid, XLogRecPtr lsn)`
`4479`	`4479`
`4480`	`4480`	`for (fork = 0; fork <= MAX_FORKNUM; fork++)`
`4481`	`4481`	`{`
`4482`		`- if (smgrexists(srel, fork))`
`4483`		`- {`
`4484`		`- XLogDropRelation(xlrec->xnodes[i], fork);`
`4485`		`- smgrdounlink(srel, fork, false, true);`
`4486`		`- }`
	`4482`	`+ XLogDropRelation(xlrec->xnodes[i], fork);`
	`4483`	`+ smgrdounlink(srel, fork, false, true);`
`4487`	`4484`	`}`
`4488`	`4485`	`smgrclose(srel);`
`4489`	`4486`	`}`
`@@ -4584,11 +4581,8 @@ xact_redo_abort(xl_xact_abort *xlrec, TransactionId xid)`
`4584`	`4581`
`4585`	`4582`	`for (fork = 0; fork <= MAX_FORKNUM; fork++)`
`4586`	`4583`	`{`
`4587`		`- if (smgrexists(srel, fork))`
`4588`		`- {`
`4589`		`- XLogDropRelation(xlrec->xnodes[i], fork);`
`4590`		`- smgrdounlink(srel, fork, false, true);`
`4591`		`- }`
	`4584`	`+ XLogDropRelation(xlrec->xnodes[i], fork);`
	`4585`	`+ smgrdounlink(srel, fork, false, true);`
`4592`	`4586`	`}`
`4593`	`4587`	`smgrclose(srel);`
`4594`	`4588`	`}`
Original file line number	Diff line number	Diff line change
`@@ -325,11 +325,10 @@ smgrDoPendingDeletes(bool isCommit)`
`325`	`325`	`srel = smgropen(pending->relnode);`
`326`	`326`	`for (i = 0; i <= MAX_FORKNUM; i++)`
`327`	`327`	`{`
`328`		`- if (smgrexists(srel, i))`
`329`		`- smgrdounlink(srel,`
`330`		`- i,`
`331`		`- pending->isTemp,`
`332`		`- false);`
	`328`	`+ smgrdounlink(srel,`
	`329`	`+ i,`
	`330`	`+ pending->isTemp,`
	`331`	`+ false);`
`333`	`332`	`}`
`334`	`333`	`smgrclose(srel);`
`335`	`334`	`}`