Skip to content
Snippets Groups Projects
Commit c37ec840 authored by Tom Lane's avatar Tom Lane
Browse files

Fix longstanding race condition in plancache.c.

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.
parent e3a7675e
No related branches found
No related tags found
No related merge requests found
...@@ -65,6 +65,14 @@ ...@@ -65,6 +65,14 @@
#include "utils/syscache.h" #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))
/* /*
* This is the head of the backend's list of "saved" CachedPlanSources (i.e., * This is the head of the backend's list of "saved" CachedPlanSources (i.e.,
* those that are in long-lived storage and are examined for sinval events). * those that are in long-lived storage and are examined for sinval events).
...@@ -352,9 +360,10 @@ CompleteCachedPlan(CachedPlanSource *plansource, ...@@ -352,9 +360,10 @@ CompleteCachedPlan(CachedPlanSource *plansource,
/* /*
* Use the planner machinery to extract dependencies. Data is saved in * Use the planner machinery to extract dependencies. Data is saved in
* query_context. (We assume that not a lot of extra cruft is created by * query_context. (We assume that not a lot of extra cruft is created by
* this call.) We can skip this for one-shot plans. * this call.) We can skip this for one-shot plans, and transaction
* control commands have no such dependencies anyway.
*/ */
if (!plansource->is_oneshot) if (!plansource->is_oneshot && !IsTransactionStmtPlan(plansource))
extract_query_dependencies((Node *) querytree_list, extract_query_dependencies((Node *) querytree_list,
&plansource->relationOids, &plansource->relationOids,
&plansource->invalItems); &plansource->invalItems);
...@@ -384,9 +393,12 @@ CompleteCachedPlan(CachedPlanSource *plansource, ...@@ -384,9 +393,12 @@ CompleteCachedPlan(CachedPlanSource *plansource,
/* /*
* Fetch current search_path into dedicated context, but do any * Fetch current search_path into dedicated context, but do any
* recalculation work required in caller's context. * recalculation work required in caller's context. We *must* skip this
* for transaction control commands, because this could result in catalog
* accesses.
*/ */
plansource->search_path = GetOverrideSearchPath(source_context); if (!IsTransactionStmtPlan(plansource))
plansource->search_path = GetOverrideSearchPath(source_context);
plansource->is_complete = true; plansource->is_complete = true;
plansource->is_valid = true; plansource->is_valid = true;
...@@ -537,9 +549,11 @@ RevalidateCachedQuery(CachedPlanSource *plansource) ...@@ -537,9 +549,11 @@ RevalidateCachedQuery(CachedPlanSource *plansource)
/* /*
* For one-shot plans, we do not support revalidation checking; it's * For one-shot plans, we do not support revalidation checking; it's
* assumed the query is parsed, planned, and executed in one transaction, * assumed the query is parsed, planned, and executed in one transaction,
* so that no lock re-acquisition is necessary. * so that no lock re-acquisition is necessary. Also, there is never
* any need to revalidate plans for transaction control commands (and
* we mustn't risk any catalog accesses when handling those).
*/ */
if (plansource->is_oneshot) if (plansource->is_oneshot || IsTransactionStmtPlan(plansource))
{ {
Assert(plansource->is_valid); Assert(plansource->is_valid);
return NIL; return NIL;
...@@ -859,7 +873,8 @@ BuildCachedPlan(CachedPlanSource *plansource, List *qlist, ...@@ -859,7 +873,8 @@ BuildCachedPlan(CachedPlanSource *plansource, List *qlist,
* compared to parse analysis + planning, I'm not going to contort the * compared to parse analysis + planning, I'm not going to contort the
* code enough to avoid that. * code enough to avoid that.
*/ */
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 that * If a snapshot is already set (the normal case), we can just use that
...@@ -894,7 +909,8 @@ BuildCachedPlan(CachedPlanSource *plansource, List *qlist, ...@@ -894,7 +909,8 @@ BuildCachedPlan(CachedPlanSource *plansource, List *qlist,
PopActiveSnapshot(); PopActiveSnapshot();
/* Now we can restore current search path */ /* Now we can restore current search path */
PopOverrideSearchPath(); if (plansource->search_path)
PopOverrideSearchPath();
/* /*
* Normally we make a dedicated memory context for the CachedPlan and its * Normally we make a dedicated memory context for the CachedPlan and its
...@@ -965,6 +981,9 @@ choose_custom_plan(CachedPlanSource *plansource, ParamListInfo boundParams) ...@@ -965,6 +981,9 @@ choose_custom_plan(CachedPlanSource *plansource, ParamListInfo boundParams)
/* Otherwise, never any point in a custom plan if there's no parameters */ /* Otherwise, never any point in a custom plan if there's no parameters */
if (boundParams == NULL) if (boundParams == NULL)
return false; return false;
/* ... nor for transaction control statements */
if (IsTransactionStmtPlan(plansource))
return false;
/* See if caller wants to force the decision */ /* See if caller wants to force the decision */
if (plansource->cursor_options & CURSOR_OPT_GENERIC_PLAN) if (plansource->cursor_options & CURSOR_OPT_GENERIC_PLAN)
...@@ -1267,7 +1286,8 @@ CopyCachedPlan(CachedPlanSource *plansource) ...@@ -1267,7 +1286,8 @@ CopyCachedPlan(CachedPlanSource *plansource)
newsource->resultDesc = CreateTupleDescCopy(plansource->resultDesc); newsource->resultDesc = CreateTupleDescCopy(plansource->resultDesc);
else else
newsource->resultDesc = NULL; newsource->resultDesc = NULL;
newsource->search_path = CopyOverrideSearchPath(plansource->search_path); if (plansource->search_path)
newsource->search_path = CopyOverrideSearchPath(plansource->search_path);
newsource->context = source_context; newsource->context = source_context;
querytree_context = AllocSetContextCreate(source_context, querytree_context = AllocSetContextCreate(source_context,
...@@ -1619,6 +1639,10 @@ PlanCacheRelCallback(Datum arg, Oid relid) ...@@ -1619,6 +1639,10 @@ PlanCacheRelCallback(Datum arg, Oid relid)
if (!plansource->is_valid) if (!plansource->is_valid)
continue; continue;
/* Never invalidate transaction control commands */
if (IsTransactionStmtPlan(plansource))
continue;
/* /*
* Check the dependency list for the rewritten querytree. * Check the dependency list for the rewritten querytree.
*/ */
...@@ -1683,6 +1707,10 @@ PlanCacheFuncCallback(Datum arg, int cacheid, uint32 hashvalue) ...@@ -1683,6 +1707,10 @@ PlanCacheFuncCallback(Datum arg, int cacheid, uint32 hashvalue)
if (!plansource->is_valid) if (!plansource->is_valid)
continue; continue;
/* Never invalidate transaction control commands */
if (IsTransactionStmtPlan(plansource))
continue;
/* /*
* Check the dependency list for the rewritten querytree. * Check the dependency list for the rewritten querytree.
*/ */
...@@ -1772,6 +1800,11 @@ ResetPlanCache(void) ...@@ -1772,6 +1800,11 @@ ResetPlanCache(void)
* We *must not* mark transaction control statements as invalid, * We *must not* mark transaction control statements as invalid,
* particularly not ROLLBACK, because they may need to be executed in * particularly not ROLLBACK, because they may need to be executed in
* aborted transactions when we can't revalidate them (cf bug #5269). * aborted transactions when we can't revalidate them (cf bug #5269).
*/
if (IsTransactionStmtPlan(plansource))
continue;
/*
* In general there is no point in invalidating utility statements * In general there is no point in invalidating utility statements
* since they have no plans anyway. So invalidate it only if it * since they have no plans anyway. So invalidate it only if it
* contains at least one non-utility statement, or contains a utility * contains at least one non-utility statement, or contains a utility
......
0% Loading or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment