Fix longstanding race condition in plancache.c.

tglsfdc · tglsfdc · commit dd9c6ff468fb · 2013-04-20T16:59:36.000-04:00
When creating or manipulating a cached plan for a transaction control
command (particularly ROLLBACK), we must not perform any catalog accesses,
since we might be in an aborted transaction.  However, plancache.c busily
saved or examined the search_path for every cached plan.  If we were
unlucky enough to do this at a moment where the path's expansion into
schema OIDs wasn't already cached, we'd do some catalog accesses; and with
some more bad luck such as an ill-timed signal arrival, that could lead to
crashes or Assert failures, as exhibited in bug #8095 from Nachiket Vaidya.
Fortunately, there's no real need to consider the search path for such
commands, so we can just skip the relevant steps when the subject statement
is a TransactionStmt.  This is somewhat related to bug #5269, though the
failure happens during initial cached-plan creation rather than
revalidation.

This bug has been there since the plan cache was invented, so back-patch
to all supported branches.
diff --git a/src/backend/utils/cache/plancache.c b/src/backend/utils/cache/plancache.c
@@ -61,6 +61,14 @@
 #include "utils/syscache.h"
 
 
+/*
+ * We must skip "overhead" operations that involve database access when the
+ * cached plan's subject statement is a transaction control command.
+ */
+#define IsTransactionStmtPlan(plansource)  \
+	((plansource)->raw_parse_tree && \
+	 IsA((plansource)->raw_parse_tree, TransactionStmt))
+
 static List *cached_plans_list = NIL;
 
 static void StoreCachedPlan(CachedPlanSource *plansource, List *stmt_list,
@@ -136,9 +144,13 @@ CreateCachedPlan(Node *raw_parse_tree,
 
 	/*
 	 * Fetch current search_path into new context, but do any recalculation
-	 * work required in caller's context.
+	 * work required in caller's context.  Skip this for a transaction control
+	 * command, since we won't need it and can't risk catalog access.
 	 */
-	search_path = GetOverrideSearchPath(source_context);
+	if (raw_parse_tree && IsA(raw_parse_tree, TransactionStmt))
+		search_path = NULL;
+	else
+		search_path = GetOverrideSearchPath(source_context);
 
 	/*
 	 * Create and fill the CachedPlanSource struct within the new context.
@@ -229,9 +241,13 @@ FastCreateCachedPlan(Node *raw_parse_tree,
 
 	/*
 	 * Fetch current search_path into given context, but do any recalculation
-	 * work required in caller's context.
+	 * work required in caller's context.  Skip this for a transaction control
+	 * command, since we won't need it and can't risk catalog access.
 	 */
-	search_path = GetOverrideSearchPath(context);
+	if (raw_parse_tree && IsA(raw_parse_tree, TransactionStmt))
+		search_path = NULL;
+	else
+		search_path = GetOverrideSearchPath(context);
 
 	/*
 	 * Create and fill the CachedPlanSource struct within the given context.
@@ -517,7 +533,8 @@ RevalidateCachedPlan(CachedPlanSource *plansource, bool useResOwner)
 		 *
 		 * (XXX is there anything else we really need to restore?)
 		 */
-		PushOverrideSearchPath(plansource->search_path);
+		if (plansource->search_path)
+			PushOverrideSearchPath(plansource->search_path);
 
 		/*
 		 * If a snapshot is already set (the normal case), we can just use
@@ -601,7 +618,8 @@ RevalidateCachedPlan(CachedPlanSource *plansource, bool useResOwner)
 			PopActiveSnapshot();
 
 		/* Now we can restore current search path */
-		PopOverrideSearchPath();
+		if (plansource->search_path)
+			PopOverrideSearchPath();
 
 		/*
 		 * Store the plans into the plancache entry, advancing the generation
