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:20 +0000 (02:00 +0000)
committerTom Lane <tgl@sss.pgh.pa.us>
Sat, 13 Dec 2008 02:00:20 +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 7a08ffca5e9c0d9b34921c44153782e641f7b5ff..d5d90d57044108b072004d91c478e9995820fe7a 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.241 2008/11/21 20:14:27 mha Exp $
+ *       $PostgreSQL: pgsql/src/backend/commands/trigger.c,v 1.242 2008/12/13 02:00:18 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -3716,11 +3716,27 @@ AfterTriggerSetState(ConstraintsSetStmt *stmt)
        if (!stmt->deferred)
        {
                AfterTriggerEventList *events = &afterTriggers->events;
+               bool            snapshot_set = false;
 
                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 (!snapshot_set)
+                       {
+                               PushActiveSnapshot(GetTransactionSnapshot());
+                               snapshot_set = true;
+                       }
+
                        /*
                         * We can delete fired events if we are at top transaction level,
                         * but we'd better not if inside a subtransaction, since the
@@ -3730,6 +3746,9 @@ AfterTriggerSetState(ConstraintsSetStmt *stmt)
                                                                                 !IsSubTransaction()))
                                break;                  /* all fired */
                }
+
+               if (snapshot_set)
+                       PopActiveSnapshot();
        }
 }
 
index 21694ea56e5908b7690f6acb3a64abad8f081d97..cdac02b71db69399e00b4a63eefe0d2f9f481ad0 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.383 2008/11/15 19:43:46 tgl Exp $
+ *     $PostgreSQL: pgsql/src/backend/parser/analyze.c,v 1.384 2008/12/13 02:00:19 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -217,6 +217,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 b09401652b8fd42273b267f5f402664a64c5b18e..97b9b170b92ba0d789cf9a349987bea4467de228 100644 (file)
@@ -8,7 +8,7 @@
  *
  *
  * IDENTIFICATION
- *       $PostgreSQL: pgsql/src/backend/tcop/postgres.c,v 1.560 2008/12/09 15:59:39 heikki Exp $
+ *       $PostgreSQL: pgsql/src/backend/tcop/postgres.c,v 1.561 2008/12/13 02:00:19 tgl Exp $
  *
  * NOTES
  *       this is the "main" module of the postgres backend and
@@ -685,6 +685,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(ActiveSnapshotSet());
+
        TRACE_POSTGRESQL_QUERY_PLAN_START();
 
        if (log_planner_stats)
@@ -872,6 +875,7 @@ exec_simple_query(const char *query_string)
        foreach(parsetree_item, parsetree_list)
        {
                Node       *parsetree = (Node *) lfirst(parsetree_item);
+               bool            snapshot_set = false;
                const char *commandTag;
                char            completionTag[COMPLETION_TAG_BUFSIZE];
                List       *querytree_list,
@@ -913,6 +917,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))
+               {
+                       PushActiveSnapshot(GetTransactionSnapshot());
+                       snapshot_set = true;
+               }
+
                /*
                 * OK to analyze, rewrite, and plan this query.
                 *
@@ -924,7 +937,11 @@ 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 */
+               if (snapshot_set)
+                       PopActiveSnapshot();
 
                /* If we got a cancel signal in analysis or planning, quit */
                CHECK_FOR_INTERRUPTS();
@@ -939,7 +956,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,
@@ -1178,6 +1195,7 @@ exec_parse_message(const char *query_string,      /* string to execute */
        if (parsetree_list != NIL)
        {
                Query      *query;
+               bool            snapshot_set = false;
                int                     i;
 
                raw_parse_tree = (Node *) linitial(parsetree_list);
@@ -1202,6 +1220,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))
+               {
+                       PushActiveSnapshot(GetTransactionSnapshot());
+                       snapshot_set = true;
+               }
+
                /*
                 * OK to analyze, rewrite, and plan this query.  Note that the
                 * originally specified parameter set is not required to be complete,
@@ -1249,9 +1276,13 @@ 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 */
+               if (snapshot_set)
+                       PopActiveSnapshot();
        }
        else
        {
@@ -1375,6 +1406,7 @@ exec_bind_message(StringInfo input_message)
        List       *plan_list;
        MemoryContext oldContext;
        bool            save_log_statement_stats = log_statement_stats;
+       bool            snapshot_set = false;
        char            msec_str[32];
 
        /* Get the fixed part of the message */
@@ -1494,6 +1526,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))
+       {
+               PushActiveSnapshot(GetTransactionSnapshot());
+               snapshot_set = true;
+       }
+
        /*
         * Fetch parameters, if any, and store in the portal's memory context.
         */
@@ -1682,7 +1725,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 ... */
@@ -1691,6 +1734,10 @@ exec_bind_message(StringInfo input_message)
                cplan = NULL;
        }
 
+       /* Done with the snapshot used for parameter I/O and parsing/planning */
+       if (snapshot_set)
+               PopActiveSnapshot();
+
        /*
         * Define portal and start execution.
         */
index fd535c0090d8ea585a23ff912441d0cf2b6db7f4..88affe67102e2244af8ae3cf12d3b6ec4c94fabf 100644 (file)
@@ -35,7 +35,7 @@
  * Portions Copyright (c) 1994, Regents of the University of California
  *
  * IDENTIFICATION
- *       $PostgreSQL: pgsql/src/backend/utils/cache/plancache.c,v 1.23 2008/10/04 21:56:54 tgl Exp $
+ *       $PostgreSQL: pgsql/src/backend/utils/cache/plancache.c,v 1.24 2008/12/13 02:00:20 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -463,6 +463,7 @@ RevalidateCachedPlan(CachedPlanSource *plansource, bool useResOwner)
         */
        if (!plan)
        {
+               bool            snapshot_set = false;
                List       *slist;
                TupleDesc       resultDesc;
 
@@ -472,6 +473,19 @@ RevalidateCachedPlan(CachedPlanSource *plansource, bool useResOwner)
                 */
                PushOverrideSearchPath(plansource->search_path);
 
+               /*
+                * If a snapshot is already set (the normal case), we can just use
+                * that for parsing/planning.  But if it isn't, install one.  Note:
+                * no point in checking whether parse analysis requires a snapshot;
+                * utility commands don't have invalidatable plans, so we'd not get
+                * here for such a command.
+                */
+               if (!ActiveSnapshotSet())
+               {
+                       PushActiveSnapshot(GetTransactionSnapshot());
+                       snapshot_set = true;
+               }
+
                /*
                 * Run parse analysis and rule rewriting.  The parser tends to
                 * scribble on its input, so we must copy the raw parse tree to
@@ -488,13 +502,9 @@ RevalidateCachedPlan(CachedPlanSource *plansource, bool useResOwner)
                {
                        /*
                         * Generate plans for queries.
-                        *
-                        * 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.
                         */
                        slist = pg_plan_queries(slist, plansource->cursor_options,
-                                                                       NULL, !ActiveSnapshotSet());
+                                                                       NULL, false);
                }
 
                /*
@@ -525,6 +535,10 @@ RevalidateCachedPlan(CachedPlanSource *plansource, bool useResOwner)
                        MemoryContextSwitchTo(oldcxt);
                }
 
+               /* Release snapshot if we got one */
+               if (snapshot_set)
+                       PopActiveSnapshot();
+
                /* Now we can restore current search path */
                PopOverrideSearchPath();
 
index eb4913294266a1fe133acecc22ba66078589ee1e..13e0771d958c2a3e1489c358a023a239484c967e 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.39 2008/12/13 02:00:20 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);