]> granicus.if.org Git - postgresql/commitdiff
Fix failure to ensure that a snapshot is available to datatype input functions
authorTom Lane <tgl@sss.pgh.pa.us>
Sat, 13 Dec 2008 02:00:30 +0000 (02:00 +0000)
committerTom Lane <tgl@sss.pgh.pa.us>
Sat, 13 Dec 2008 02:00:30 +0000 (02:00 +0000)
when they are invoked by the parser.  We had been setting up a snapshot at
plan time but really it needs to be done earlier, before parse analysis.
Per report from Dmitry Koterov.

Also fix two related problems discovered while poking at this one:
exec_bind_message called datatype input functions without establishing a
snapshot, and SET CONSTRAINTS IMMEDIATE could call trigger functions without
establishing a snapshot.

Backpatch to 8.2.  The underlying problem goes much further back, but it is
masked in 8.1 and before because we didn't attempt to invoke domain check
constraints within datatype input.  It would only be exposed if a C-language
datatype input function used the snapshot; which evidently none do, or we'd
have heard complaints sooner.  Since this code has changed a lot over time,
a back-patch is hardly risk-free, and so I'm disinclined to patch further
than absolutely necessary.

src/backend/commands/trigger.c
src/backend/parser/analyze.c
src/backend/tcop/postgres.c
src/backend/utils/cache/plancache.c
src/include/parser/analyze.h

index 33da6632cae66c88fe3861bff6bb8207b53dbac7..96ec841f847269ee1d60e7767a8a93f718a117a9 100644 (file)
@@ -7,7 +7,7 @@
  * Portions Copyright (c) 1994, Regents of the University of California
  *
  * IDENTIFICATION
- *       $PostgreSQL: pgsql/src/backend/commands/trigger.c,v 1.227.2.1 2008/10/25 03:32:44 tgl Exp $
+ *       $PostgreSQL: pgsql/src/backend/commands/trigger.c,v 1.227.2.2 2008/12/13 02:00:29 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -3469,19 +3469,51 @@ AfterTriggerSetState(ConstraintsSetStmt *stmt)
        if (!stmt->deferred)
        {
                AfterTriggerEventList *events = &afterTriggers->events;
+               Snapshot        saveActiveSnapshot = ActiveSnapshot;
 
-               while (afterTriggerMarkEvents(events, NULL, true))
+               /* PG_TRY to ensure previous ActiveSnapshot is restored on error */
+               PG_TRY();
                {
-                       CommandId       firing_id = afterTriggers->firing_counter++;
+                       Snapshot        mySnapshot = NULL;
 
-                       /*
-                        * We can delete fired events if we are at top transaction level,
-                        * but we'd better not if inside a subtransaction, since the
-                        * subtransaction could later get rolled back.
-                        */
-                       afterTriggerInvokeEvents(-1, firing_id, NULL,
-                                                                        !IsSubTransaction());
+                       while (afterTriggerMarkEvents(events, NULL, true))
+                       {
+                               CommandId       firing_id = afterTriggers->firing_counter++;
+
+                               /*
+                                * Make sure a snapshot has been established in case trigger
+                                * functions need one.  Note that we avoid setting a snapshot
+                                * if we don't find at least one trigger that has to be fired
+                                * now.  This is so that BEGIN; SET CONSTRAINTS ...; SET
+                                * TRANSACTION ISOLATION LEVEL SERIALIZABLE; ... works
+                                * properly.  (If we are at the start of a transaction it's
+                                * not possible for any trigger events to be queued yet.)
+                                */
+                               if (mySnapshot == NULL)
+                               {
+                                       mySnapshot = CopySnapshot(GetTransactionSnapshot());
+                                       ActiveSnapshot = mySnapshot;
+                               }
+
+                               /*
+                                * We can delete fired events if we are at top transaction level,
+                                * but we'd better not if inside a subtransaction, since the
+                                * subtransaction could later get rolled back.
+                                */
+                               afterTriggerInvokeEvents(-1, firing_id, NULL,
+                                                                                !IsSubTransaction());
+                       }
+
+                       if (mySnapshot)
+                               FreeSnapshot(mySnapshot);
+               }
+               PG_CATCH();
+               {
+                       ActiveSnapshot = saveActiveSnapshot;
+                       PG_RE_THROW();
                }
+               PG_END_TRY();
+               ActiveSnapshot = saveActiveSnapshot;
        }
 }
 
index 759dc64d940eaa39aedb617eaa204634c3ab6257..4a539a1e1e897eeaac3765da9ed21366f5845b1d 100644 (file)
@@ -17,7 +17,7 @@
  * Portions Copyright (c) 1996-2008, PostgreSQL Global Development Group
  * Portions Copyright (c) 1994, Regents of the University of California
  *
- *     $PostgreSQL: pgsql/src/backend/parser/analyze.c,v 1.371 2008/01/01 19:45:50 momjian Exp $
+ *     $PostgreSQL: pgsql/src/backend/parser/analyze.c,v 1.371.2.1 2008/12/13 02:00:29 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -227,6 +227,55 @@ transformStmt(ParseState *pstate, Node *parseTree)
        return result;
 }
 
+/*
+ * analyze_requires_snapshot
+ *             Returns true if a snapshot must be set before doing parse analysis
+ *             on the given raw parse tree.
+ *
+ * Classification here should match transformStmt().
+ */
+bool
+analyze_requires_snapshot(Node *parseTree)
+{
+       bool            result;
+
+       switch (nodeTag(parseTree))
+       {
+                       /*
+                        * Optimizable statements
+                        */
+               case T_InsertStmt:
+               case T_DeleteStmt:
+               case T_UpdateStmt:
+               case T_SelectStmt:
+                       result = true;
+                       break;
+
+                       /*
+                        * Special cases
+                        */
+               case T_DeclareCursorStmt:
+                       /* yes, because it's analyzed just like SELECT */
+                       result = true;
+                       break;
+
+               case T_ExplainStmt:
+                       /*
+                        * We only need a snapshot in varparams case, but it doesn't seem
+                        * worth complicating this function's API to distinguish that.
+                        */
+                       result = true;
+                       break;
+
+               default:
+                       /* utility statements don't have any active parse analysis */
+                       result = false;
+                       break;
+       }
+
+       return result;
+}
+
 /*
  * transformDeleteStmt -
  *       transforms a Delete Statement
index c3531b584494bc4051d0de910c33086b5a462c3b..85a266172f867ce1253f89e28bdde0001d5e9d95 100644 (file)
@@ -8,7 +8,7 @@
  *
  *
  * IDENTIFICATION
- *       $PostgreSQL: pgsql/src/backend/tcop/postgres.c,v 1.542.2.2 2008/04/02 18:32:00 tgl Exp $
+ *       $PostgreSQL: pgsql/src/backend/tcop/postgres.c,v 1.542.2.3 2008/12/13 02:00:29 tgl Exp $
  *
  * NOTES
  *       this is the "main" module of the postgres backend and
@@ -674,6 +674,9 @@ pg_plan_query(Query *querytree, int cursorOptions, ParamListInfo boundParams)
        if (querytree->commandType == CMD_UTILITY)
                return NULL;
 
+       /* Planner must have a snapshot in case it calls user-defined functions. */
+       Assert(ActiveSnapshot != NULL);
+
        if (log_planner_stats)
                ResetUsage();
 
@@ -866,6 +869,7 @@ exec_simple_query(const char *query_string)
        foreach(parsetree_item, parsetree_list)
        {
                Node       *parsetree = (Node *) lfirst(parsetree_item);
+               Snapshot        mySnapshot = NULL;
                const char *commandTag;
                char            completionTag[COMPLETION_TAG_BUFSIZE];
                List       *querytree_list,
@@ -907,6 +911,15 @@ exec_simple_query(const char *query_string)
                /* If we got a cancel signal in parsing or prior command, quit */
                CHECK_FOR_INTERRUPTS();
 
+               /*
+                * Set up a snapshot if parse analysis/planning will need one.
+                */
+               if (analyze_requires_snapshot(parsetree))
+               {
+                       mySnapshot = CopySnapshot(GetTransactionSnapshot());
+                       ActiveSnapshot = mySnapshot;
+               }
+
                /*
                 * OK to analyze, rewrite, and plan this query.
                 *
@@ -918,7 +931,12 @@ exec_simple_query(const char *query_string)
                querytree_list = pg_analyze_and_rewrite(parsetree, query_string,
                                                                                                NULL, 0);
 
-               plantree_list = pg_plan_queries(querytree_list, 0, NULL, true);
+               plantree_list = pg_plan_queries(querytree_list, 0, NULL, false);
+
+               /* Done with the snapshot used for parsing/planning */
+               ActiveSnapshot = NULL;
+               if (mySnapshot)
+                       FreeSnapshot(mySnapshot);
 
                /* If we got a cancel signal in analysis or planning, quit */
                CHECK_FOR_INTERRUPTS();
@@ -933,7 +951,7 @@ exec_simple_query(const char *query_string)
 
                /*
                 * We don't have to copy anything into the portal, because everything
-                * we are passsing here is in MessageContext, which will outlive the
+                * we are passing here is in MessageContext, which will outlive the
                 * portal anyway.
                 */
                PortalDefineQuery(portal,
@@ -1168,6 +1186,7 @@ exec_parse_message(const char *query_string,      /* string to execute */
        if (parsetree_list != NIL)
        {
                Query      *query;
+               Snapshot        mySnapshot = NULL;
                int                     i;
 
                raw_parse_tree = (Node *) linitial(parsetree_list);
@@ -1192,6 +1211,15 @@ exec_parse_message(const char *query_string,     /* string to execute */
                                         errmsg("current transaction is aborted, "
                                                "commands ignored until end of transaction block")));
 
+               /*
+                * Set up a snapshot if parse analysis/planning will need one.
+                */
+               if (analyze_requires_snapshot(raw_parse_tree))
+               {
+                       mySnapshot = CopySnapshot(GetTransactionSnapshot());
+                       ActiveSnapshot = mySnapshot;
+               }
+
                /*
                 * OK to analyze, rewrite, and plan this query.  Note that the
                 * originally specified parameter set is not required to be complete,
@@ -1239,9 +1267,14 @@ exec_parse_message(const char *query_string,     /* string to execute */
                }
                else
                {
-                       stmt_list = pg_plan_queries(querytree_list, 0, NULL, true);
+                       stmt_list = pg_plan_queries(querytree_list, 0, NULL, false);
                        fully_planned = true;
                }
+
+               /* Done with the snapshot used for parsing/planning */
+               ActiveSnapshot = NULL;
+               if (mySnapshot)
+                       FreeSnapshot(mySnapshot);
        }
        else
        {
@@ -1365,6 +1398,7 @@ exec_bind_message(StringInfo input_message)
        List       *plan_list;
        MemoryContext oldContext;
        bool            save_log_statement_stats = log_statement_stats;
+       Snapshot        mySnapshot = NULL;
        char            msec_str[32];
 
        /* Get the fixed part of the message */
@@ -1486,6 +1520,17 @@ exec_bind_message(StringInfo input_message)
        else
                saved_stmt_name = NULL;
 
+       /*
+        * Set a snapshot if we have parameters to fetch (since the input
+        * functions might need it) or the query isn't a utility command (and
+        * hence could require redoing parse analysis and planning).
+        */
+       if (numParams > 0 || analyze_requires_snapshot(psrc->raw_parse_tree))
+       {
+               mySnapshot = CopySnapshot(GetTransactionSnapshot());
+               ActiveSnapshot = mySnapshot;
+       }
+
        /*
         * Fetch parameters, if any, and store in the portal's memory context.
         */
@@ -1674,7 +1719,7 @@ exec_bind_message(StringInfo input_message)
                 */
                oldContext = MemoryContextSwitchTo(PortalGetHeapMemory(portal));
                query_list = copyObject(cplan->stmt_list);
-               plan_list = pg_plan_queries(query_list, 0, params, true);
+               plan_list = pg_plan_queries(query_list, 0, params, false);
                MemoryContextSwitchTo(oldContext);
 
                /* We no longer need the cached plan refcount ... */
@@ -1683,6 +1728,11 @@ exec_bind_message(StringInfo input_message)
                cplan = NULL;
        }
 
+       /* Done with the snapshot used for parameter I/O and parsing/planning */
+       ActiveSnapshot = NULL;
+       if (mySnapshot)
+               FreeSnapshot(mySnapshot);
+
        /*
         * Define portal and start execution.
         */
@@ -3434,6 +3484,9 @@ PostgresMain(int argc, char *argv[], const char *username)
                 */
                debug_query_string = NULL;
 
+               /* No active snapshot any more either */
+               ActiveSnapshot = NULL;
+
                /*
                 * Abort the current transaction in order to recover.
                 */
index bbfdd888c1c0db10c361a505eb11724a467faddd..5019df6f86a91472b9ec117b47ff6926a77a6a0d 100644 (file)
@@ -33,7 +33,7 @@
  * Portions Copyright (c) 1994, Regents of the University of California
  *
  * IDENTIFICATION
- *       $PostgreSQL: pgsql/src/backend/utils/cache/plancache.c,v 1.15.2.1 2008/09/15 23:37:49 tgl Exp $
+ *       $PostgreSQL: pgsql/src/backend/utils/cache/plancache.c,v 1.15.2.2 2008/12/13 02:00:29 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -70,7 +70,6 @@ static List *cached_plans_list = NIL;
 
 static void StoreCachedPlan(CachedPlanSource *plansource, List *stmt_list,
                                MemoryContext plan_context);
-static List *do_planning(List *querytrees, int cursorOptions);
 static void AcquireExecutorLocks(List *stmt_list, bool acquire);
 static void AcquirePlannerLocks(List *stmt_list, bool acquire);
 static void LockRelid(Oid relid, LOCKMODE lockmode, void *arg);
@@ -457,8 +456,8 @@ RevalidateCachedPlan(CachedPlanSource *plansource, bool useResOwner)
         */
        if (!plan)
        {
+               Snapshot        saveActiveSnapshot = ActiveSnapshot;
                List       *slist;
-               TupleDesc       resultDesc;
 
                /*
                 * Restore the search_path that was in use when the plan was made.
@@ -467,50 +466,86 @@ RevalidateCachedPlan(CachedPlanSource *plansource, bool useResOwner)
                PushOverrideSearchPath(plansource->search_path);
 
                /*
-                * Run parse analysis and rule rewriting.  The parser tends to
-                * scribble on its input, so we must copy the raw parse tree to
-                * prevent corruption of the cache.  Note that we do not use
-                * parse_analyze_varparams(), assuming that the caller never wants the
-                * parameter types to change from the original values.
+                * If a snapshot is already set (the normal case), we can just use
+                * that for parsing/planning.  But if it isn't, install one.  We must
+                * arrange to restore ActiveSnapshot afterward, to ensure that
+                * RevalidateCachedPlan has no caller-visible effects on the
+                * snapshot.  Having to replan is an unusual case, and it seems a
+                * really bad idea for RevalidateCachedPlan to affect the snapshot
+                * only in unusual cases.  (Besides, the snap might have been created
+                * in a short-lived context.)
                 */
-               slist = pg_analyze_and_rewrite(copyObject(plansource->raw_parse_tree),
-                                                                          plansource->query_string,
-                                                                          plansource->param_types,
-                                                                          plansource->num_params);
-
-               if (plansource->fully_planned)
+               PG_TRY();
                {
-                       /* Generate plans for queries */
-                       slist = do_planning(slist, plansource->cursor_options);
-               }
+                       Snapshot        mySnapshot = NULL;
+                       TupleDesc       resultDesc;
 
-               /*
-                * Check or update the result tupdesc.  XXX should we use a weaker
-                * condition than equalTupleDescs() here?
-                */
-               resultDesc = PlanCacheComputeResultDesc(slist);
-               if (resultDesc == NULL && plansource->resultDesc == NULL)
-               {
-                       /* OK, doesn't return tuples */
+                       if (ActiveSnapshot == NULL)
+                       {
+                               mySnapshot = CopySnapshot(GetTransactionSnapshot());
+                               ActiveSnapshot = mySnapshot;
+                       }
+
+                       /*
+                        * Run parse analysis and rule rewriting.  The parser tends to
+                        * scribble on its input, so we must copy the raw parse tree to
+                        * prevent corruption of the cache.  Note that we do not use
+                        * parse_analyze_varparams(), assuming that the caller never wants
+                        * the parameter types to change from the original values.
+                        */
+                       slist = pg_analyze_and_rewrite(copyObject(plansource->raw_parse_tree),
+                                                                                  plansource->query_string,
+                                                                                  plansource->param_types,
+                                                                                  plansource->num_params);
+
+                       if (plansource->fully_planned)
+                       {
+                               /* Generate plans for queries */
+                               slist = pg_plan_queries(slist, plansource->cursor_options,
+                                                                               NULL, false);
+                       }
+
+                       /*
+                        * Check or update the result tupdesc.  XXX should we use a weaker
+                        * condition than equalTupleDescs() here?
+                        */
+                       resultDesc = PlanCacheComputeResultDesc(slist);
+                       if (resultDesc == NULL && plansource->resultDesc == NULL)
+                       {
+                               /* OK, doesn't return tuples */
+                       }
+                       else if (resultDesc == NULL || plansource->resultDesc == NULL ||
+                                        !equalTupleDescs(resultDesc, plansource->resultDesc))
+                       {
+                               MemoryContext oldcxt;
+
+                               /* can we give a better error message? */
+                               if (plansource->fixed_result)
+                                       ereport(ERROR,
+                                                       (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+                                                        errmsg("cached plan must not change result type")));
+                               oldcxt = MemoryContextSwitchTo(plansource->context);
+                               if (resultDesc)
+                                       resultDesc = CreateTupleDescCopy(resultDesc);
+                               if (plansource->resultDesc)
+                                       FreeTupleDesc(plansource->resultDesc);
+                               plansource->resultDesc = resultDesc;
+                               MemoryContextSwitchTo(oldcxt);
+                       }
+
+                       /* Done with snapshot */
+                       if (mySnapshot)
+                               FreeSnapshot(mySnapshot);
                }
-               else if (resultDesc == NULL || plansource->resultDesc == NULL ||
-                                !equalTupleDescs(resultDesc, plansource->resultDesc))
+               PG_CATCH();
                {
-                       MemoryContext oldcxt;
-
-                       /* can we give a better error message? */
-                       if (plansource->fixed_result)
-                               ereport(ERROR,
-                                               (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
-                                                errmsg("cached plan must not change result type")));
-                       oldcxt = MemoryContextSwitchTo(plansource->context);
-                       if (resultDesc)
-                               resultDesc = CreateTupleDescCopy(resultDesc);
-                       if (plansource->resultDesc)
-                               FreeTupleDesc(plansource->resultDesc);
-                       plansource->resultDesc = resultDesc;
-                       MemoryContextSwitchTo(oldcxt);
+                       /* Restore global vars and propagate error */
+                       ActiveSnapshot = saveActiveSnapshot;
+                       PG_RE_THROW();
                }
+               PG_END_TRY();
+
+               ActiveSnapshot = saveActiveSnapshot;
 
                /* Now we can restore current search path */
                PopOverrideSearchPath();
@@ -536,49 +571,6 @@ RevalidateCachedPlan(CachedPlanSource *plansource, bool useResOwner)
        return plan;
 }
 
-/*
- * Invoke the planner on some rewritten queries.  This is broken out of
- * RevalidateCachedPlan just to avoid plastering "volatile" all over that
- * function's variables.
- */
-static List *
-do_planning(List *querytrees, int cursorOptions)
-{
-       List       *stmt_list;
-
-       /*
-        * If a snapshot is already set (the normal case), we can just use that
-        * for planning.  But if it isn't, we have to tell pg_plan_queries to make
-        * a snap if it needs one.      In that case we should arrange to reset
-        * ActiveSnapshot afterward, to ensure that RevalidateCachedPlan has no
-        * caller-visible effects on the snapshot.      Having to replan is an unusual
-        * case, and it seems a really bad idea for RevalidateCachedPlan to affect
-        * the snapshot only in unusual cases.  (Besides, the snap might have been
-        * created in a short-lived context.)
-        */
-       if (ActiveSnapshot != NULL)
-               stmt_list = pg_plan_queries(querytrees, cursorOptions, NULL, false);
-       else
-       {
-               PG_TRY();
-               {
-                       stmt_list = pg_plan_queries(querytrees, cursorOptions, NULL, true);
-               }
-               PG_CATCH();
-               {
-                       /* Restore global vars and propagate error */
-                       ActiveSnapshot = NULL;
-                       PG_RE_THROW();
-               }
-               PG_END_TRY();
-
-               ActiveSnapshot = NULL;
-       }
-
-       return stmt_list;
-}
-
-
 /*
  * ReleaseCachedPlan: release active use of a cached plan.
  *
index eb4913294266a1fe133acecc22ba66078589ee1e..7d9361268ca444de003a88bbe82c3fc0de1c98e1 100644 (file)
@@ -7,7 +7,7 @@
  * Portions Copyright (c) 1996-2008, PostgreSQL Global Development Group
  * Portions Copyright (c) 1994, Regents of the University of California
  *
- * $PostgreSQL: pgsql/src/include/parser/analyze.h,v 1.38 2008/01/01 19:45:58 momjian Exp $
+ * $PostgreSQL: pgsql/src/include/parser/analyze.h,v 1.38.2.1 2008/12/13 02:00:30 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -25,6 +25,8 @@ extern Query *parse_analyze_varparams(Node *parseTree, const char *sourceText,
 extern Query *parse_sub_analyze(Node *parseTree, ParseState *parentParseState);
 extern Query *transformStmt(ParseState *pstate, Node *parseTree);
 
+extern bool analyze_requires_snapshot(Node *parseTree);
+
 extern void CheckSelectLocking(Query *qry);
 extern void applyLockingClause(Query *qry, Index rtindex,
                                   bool forUpdate, bool noWait);