Avoid throwing ERROR during WAL replay of DROP TABLESPACE.

tglsfdc · tglsfdc · commit 07e36415e520 · 2012-02-06T14:44:10.000-05:00
Although we will not even issue an XLOG_TBLSPC_DROP WAL record unless
removal of the tablespace's directories succeeds, that does not guarantee
that the same operation will succeed during WAL replay.  Foreseeable
reasons for it to fail include temp files created in the tablespace by Hot
Standby backends, wrong directory permissions on a standby server, etc etc.
The original coding threw ERROR if replay failed to remove the directories,
but that is a serious overreaction.  Throwing an error aborts recovery,
and worse means that manual intervention will be needed to get the database
to start again, since otherwise the same error will recur on subsequent
attempts to replay the same WAL record.  And the consequence of failing to
remove the directories is only that some probably-small amount of disk
space is wasted, so it hardly seems justified to throw an error.
Accordingly, arrange to report such failures as LOG messages and keep going
when a failure occurs during replay.

Back-patch to 9.0 where Hot Standby was introduced.  In principle such
problems can occur in earlier releases, but Hot Standby increases the odds
of trouble significantly.  Given the lack of field reports of such issues,
I'm satisfied with patching back as far as the patch applies easily.
diff --git a/src/backend/commands/tablespace.c b/src/backend/commands/tablespace.c
@@ -626,9 +626,13 @@ create_tablespace_directories(const char *location, const Oid tablespaceoid)
 /*
  * destroy_tablespace_directories
  *
- * Attempt to remove filesystem infrastructure
+ * Attempt to remove filesystem infrastructure for the tablespace.
  *
- * 'redo' indicates we are redoing a drop from XLOG; okay if nothing there
+ * 'redo' indicates we are redoing a drop from XLOG; in that case we should
+ * not throw an ERROR for problems, just LOG them.  The worst consequence of
+ * not removing files here would be failure to release some disk space, which
+ * does not justify throwing an error that would require manual intervention
+ * to get the database running again.
  *
  * Returns TRUE if successful, FALSE if some subdirectory is not empty
  */
@@ -681,6 +685,16 @@ destroy_tablespace_directories(Oid tablespaceoid, bool redo)
 			pfree(linkloc_with_version_dir);
 			return true;
 		}
+		else if (redo)
+		{
+			/* in redo, just log other types of error */
+			ereport(LOG,
+					(errcode_for_file_access(),
+					 errmsg("could not open directory \"%s\": %m",
+							linkloc_with_version_dir)));
+			pfree(linkloc_with_version_dir);
+			return false;
+		}
 		/* else let ReadDir report the error */
 	}
 
@@ -694,7 +708,7 @@ destroy_tablespace_directories(Oid tablespaceoid, bool redo)
 		sprintf(subfile, "%s/%s", linkloc_with_version_dir, de->d_name);
 
 		/* This check is just to deliver a friendlier error message */
-		if (!directory_is_empty(subfile))
+		if (!redo && !directory_is_empty(subfile))
 		{
 			FreeDir(dirdesc);
 			pfree(subfile);
@@ -704,7 +718,7 @@ destroy_tablespace_directories(Oid tablespaceoid, bool redo)
 
 		/* remove empty directory */
 		if (rmdir(subfile) < 0)
-			ereport(ERROR,
+			ereport(redo ? LOG : ERROR,
 					(errcode_for_file_access(),
 					 errmsg("could not remove directory \"%s\": %m",
 							subfile)));
@@ -716,31 +730,38 @@ destroy_tablespace_directories(Oid tablespaceoid, bool redo)
 
 	/* remove version directory */
 	if (rmdir(linkloc_with_version_dir) < 0)
-		ereport(ERROR,
+	{
+		ereport(redo ? LOG : ERROR,
 				(errcode_for_file_access(),
 				 errmsg("could not remove directory \"%s\": %m",
 						linkloc_with_version_dir)));
+		pfree(linkloc_with_version_dir);
+		return false;
+	}
 
 	/*
 	 * Try to remove the symlink.  We must however deal with the possibility
 	 * that it's a directory instead of a symlink --- this could happen during
 	 * WAL replay (see TablespaceCreateDbspace), and it is also the case on
 	 * Windows where junction points lstat() as directories.
+	 *
+	 * Note: in the redo case, we'll return true if this final step fails;
+	 * there's no point in retrying it.
 	 */
 	linkloc = pstrdup(linkloc_with_version_dir);
 	get_parent_directory(linkloc);
 	if (lstat(linkloc, &st) == 0 && S_ISDIR(st.st_mode))
 	{
 		if (rmdir(linkloc) < 0)
-			ereport(ERROR,
+			ereport(redo ? LOG : ERROR,
 					(errcode_for_file_access(),
 					 errmsg("could not remove directory \"%s\": %m",
 							linkloc)));
 	}
 	else
 	{
 		if (unlink(linkloc) < 0)
-			ereport(ERROR,
+			ereport(redo ? LOG : ERROR,
 					(errcode_for_file_access(),
 					 errmsg("could not remove symbolic link \"%s\": %m",
 							linkloc)));
@@ -1413,30 +1434,38 @@ tblspc_redo(XLogRecPtr lsn, XLogRecord *record)
 		xl_tblspc_drop_rec *xlrec = (xl_tblspc_drop_rec *) XLogRecGetData(record);
 
 		/*
-		 * If we issued a WAL record for a drop tablespace it is because there
-		 * were no files in it at all. That means that no permanent objects
-		 * can exist in it at this point.
+		 * If we issued a WAL record for a drop tablespace it implies that
+		 * there were no files in it at all when the DROP was done. That means
+		 * that no permanent objects can exist in it at this point.
 		 *
 		 * It is possible for standby users to be using this tablespace as a
 		 * location for their temporary files, so if we fail to remove all
 		 * files then do conflict processing and try again, if currently
 		 * enabled.
+		 *
+		 * Other possible reasons for failure include bollixed file permissions
+		 * on a standby server when they were okay on the primary, etc etc.
+		 * There's not much we can do about that, so just remove what we can
+		 * and press on.
 		 */
 		if (!destroy_tablespace_directories(xlrec->ts_id, true))
 		{
 			ResolveRecoveryConflictWithTablespace(xlrec->ts_id);
 
 			/*
 			 * If we did recovery processing then hopefully the backends who
-			 * wrote temp files should have cleaned up and exited by now. So
-			 * lets recheck before we throw an error. If !process_conflicts
-			 * then this will just fail again.
+			 * wrote temp files should have cleaned up and exited by now.  So
+			 * retry before complaining.  If we fail again, this is just a LOG
+			 * condition, because it's not worth throwing an ERROR for (as
+			 * that would crash the database and require manual intervention
+			 * before we could get past this WAL record on restart).
 			 */
 			if (!destroy_tablespace_directories(xlrec->ts_id, true))
-				ereport(ERROR,
+				ereport(LOG,
 						(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
-						 errmsg("tablespace %u is not empty",
-								xlrec->ts_id)));
+						 errmsg("directories for tablespace %u could not be removed",
+								xlrec->ts_id),
+						 errhint("You can remove the directories manually if necessary.")));
 		}
 	}
 	else
@@ -1452,14 +1481,14 @@ tblspc_desc(StringInfo buf, uint8 xl_info, char *rec)
 	{
 		xl_tblspc_create_rec *xlrec = (xl_tblspc_create_rec *) rec;
 
-		appendStringInfo(buf, "create ts: %u \"%s\"",
+		appendStringInfo(buf, "create tablespace: %u \"%s\"",
 						 xlrec->ts_id, xlrec->ts_path);
 	}
 	else if (info == XLOG_TBLSPC_DROP)
 	{
 		xl_tblspc_drop_rec *xlrec = (xl_tblspc_drop_rec *) rec;
 
-		appendStringInfo(buf, "drop ts: %u", xlrec->ts_id);
+		appendStringInfo(buf, "drop tablespace: %u", xlrec->ts_id);
 	}
 	else
 		appendStringInfo(buf, "UNKNOWN");
