Re: BUG #18059: Unexpected error 25001 in stored procedure

I wrote:
> I started to code this, and immediately noticed that transformStmt()
> already has a companion function analyze_requires_snapshot() that
> returns "true" in the cases of interest ... except that it does
> not return true for T_CallStmt.  Perhaps that was intentional to
> begin with, but it is very hard to believe that it isn't a bug now,
> since transformCallStmt can invoke nearly arbitrary processing via
> transformExpr().  What semantic anomalies, if any, do we risk if CALL
> processing forces a transaction start?  (I rather imagine it does
> already, somewhere later on...)

I poked around some more, and determined that there should not be any
new semantic anomalies if analyze_requires_snapshot starts returning
true for CallStmts, because ExecuteCallStmt already acquires and
releases a snapshot before invoking the procedure (at least in the
non-atomic case, which is the one of interest here).  I spent some
time trying to devise a test case showing it's broken, and did not
succeed: the fact that we disallow sub-SELECTs in CALL arguments makes
it a lot harder than I'd expected to reach anyplace that would require
having a transaction snapshot set.  Nonetheless, I have very little
faith that such a scenario doesn't exist today, and even less that
we won't add one in future.  The only real reason I can see for not
setting a snapshot here is as a micro-optimization.  While that's
not without value, it seems hard to argue that CALL deserves an
optimization that SELECT doesn't get.

I also realized that ReturnStmts are likewise missing from
analyze_requires_snapshot().  This is probably unreachable, because
ReturnStmt can only appear in a SQL-language function and I can't
think of a scenario where we'd be parsing one outside a transaction.
Nonetheless it seems hard to argue that this is an optimization
we need to keep.

Hence I propose the attached patch, which invents
stmt_requires_parse_analysis() and makes analyze_requires_snapshot()
into an alias for it, so that all these statement types are treated
alike.  I made the adjacent comments a lot more opinionated, too,
in hopes that future additions won't overlook these concerns.

The actual bug fix is in plancache.c.  I decided to invert the tests
in plancache.c, because the macro really needed renaming anyway and
it seemed to read better this way.  I also noticed that
ResetPlanCache() already tries to optimize away invalidation of
utility statements, but that logic seems no longer necessary ---
what's more, it's outright wrong for CALL, because that does need
invalidation and won't get it.  (I have not tried to build a test
case proving that that's broken, but surely it is.)

Barring objections, this needs to be back-patched as far as v11.

            regards, tom lane

diff --git a/src/backend/parser/analyze.c b/src/backend/parser/analyze.c
index 4006632092..bc8176f545 100644
--- a/src/backend/parser/analyze.c
+++ b/src/backend/parser/analyze.c
@@ -335,6 +335,11 @@ transformStmt(ParseState *pstate, Node *parseTree)
     }
 #endif                            /* RAW_EXPRESSION_COVERAGE_TEST */

+    /*
+     * Caution: when changing the set of statement types that have non-default
+     * processing here, see also stmt_requires_parse_analysis() and
+     * analyze_requires_snapshot().
+     */
     switch (nodeTag(parseTree))
     {
             /*
@@ -421,14 +426,22 @@ transformStmt(ParseState *pstate, Node *parseTree)
 }

 /*
- * analyze_requires_snapshot
- *        Returns true if a snapshot must be set before doing parse analysis
- *        on the given raw parse tree.
+ * stmt_requires_parse_analysis
+ *        Returns true if parse analysis will do anything non-trivial
+ *        with the given raw parse tree.
+ *
+ * Generally, this should return true for any statement type for which
+ * transformStmt() does more than wrap a CMD_UTILITY Query around it.
+ * When it returns false, the caller may assume that there is no situation
+ * in which parse analysis of the raw statement could need to be re-done.
  *
- * Classification here should match transformStmt().
+ * Currently, since the rewriter and planner do nothing for CMD_UTILITY
+ * Queries, a false result means that the entire parse analysis/rewrite/plan
+ * pipeline will never need to be re-done.  If that ever changes, callers
+ * will likely need adjustment.
  */
 bool
-analyze_requires_snapshot(RawStmt *parseTree)
+stmt_requires_parse_analysis(RawStmt *parseTree)
 {
     bool        result;

@@ -442,6 +455,7 @@ analyze_requires_snapshot(RawStmt *parseTree)
         case T_UpdateStmt:
         case T_MergeStmt:
         case T_SelectStmt:
+        case T_ReturnStmt:
         case T_PLAssignStmt:
             result = true;
             break;
@@ -452,12 +466,12 @@ analyze_requires_snapshot(RawStmt *parseTree)
         case T_DeclareCursorStmt:
         case T_ExplainStmt:
         case T_CreateTableAsStmt:
-            /* yes, because we must analyze the contained statement */
+        case T_CallStmt:
             result = true;
             break;

         default:
-            /* other utility statements don't have any real parse analysis */
+            /* all other statements just get wrapped in a CMD_UTILITY Query */
             result = false;
             break;
     }
@@ -465,6 +479,30 @@ analyze_requires_snapshot(RawStmt *parseTree)
     return result;
 }

+/*
+ * analyze_requires_snapshot
+ *        Returns true if a snapshot must be set before doing parse analysis
+ *        on the given raw parse tree.
+ */
+bool
+analyze_requires_snapshot(RawStmt *parseTree)
+{
+    /*
+     * Currently, this should return true in exactly the same cases that
+     * stmt_requires_parse_analysis() does, so we just invoke that function
+     * rather than duplicating it.  We keep the two entry points separate for
+     * clarity of callers, since from the callers' standpoint these are
+     * different conditions.
+     *
+     * While there may someday be a statement type for which transformStmt()
+     * does something nontrivial and yet no snapshot is needed for that
+     * processing, it seems likely that making such a choice would be fragile.
+     * If you want to install an exception, document the reasoning for it in a
+     * comment.
+     */
+    return stmt_requires_parse_analysis(parseTree);
+}
+
 /*
  * transformDeleteStmt -
  *      transforms a Delete Statement
diff --git a/src/backend/utils/cache/plancache.c b/src/backend/utils/cache/plancache.c
index d67cd9a405..7d4168f82f 100644
--- a/src/backend/utils/cache/plancache.c
+++ b/src/backend/utils/cache/plancache.c
@@ -77,13 +77,15 @@

 /*
  * We must skip "overhead" operations that involve database access when the
- * cached plan's subject statement is a transaction control command.
- * For the convenience of postgres.c, treat empty statements as control
- * commands too.
+ * cached plan's subject statement is a transaction control command or one
+ * that requires a snapshot not to be set yet (such as SET or LOCK).  More
+ * generally, statements that do not require parse analysis/rewrite/plan
+ * activity never need to be revalidated, so we can treat them all like that.
+ * For the convenience of postgres.c, treat empty statements that way too.
  */
-#define IsTransactionStmtPlan(plansource)  \
-    ((plansource)->raw_parse_tree == NULL || \
-     IsA((plansource)->raw_parse_tree->stmt, TransactionStmt))
+#define StmtPlanRequiresRevalidation(plansource)  \
+    ((plansource)->raw_parse_tree != NULL && \
+     stmt_requires_parse_analysis((plansource)->raw_parse_tree))

 /*
  * This is the head of the backend's list of "saved" CachedPlanSources (i.e.,
@@ -383,13 +385,13 @@ CompleteCachedPlan(CachedPlanSource *plansource,
     plansource->query_context = querytree_context;
     plansource->query_list = querytree_list;

-    if (!plansource->is_oneshot && !IsTransactionStmtPlan(plansource))
+    if (!plansource->is_oneshot && StmtPlanRequiresRevalidation(plansource))
     {
         /*
          * 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 this call.)  We can skip this for one-shot plans, and
-         * transaction control commands have no such dependencies anyway.
+         * plans not needing revalidation have no such dependencies anyway.
          */
         extract_query_dependencies((Node *) querytree_list,
                                    &plansource->relationOids,
@@ -568,11 +570,11 @@ RevalidateCachedQuery(CachedPlanSource *plansource,
     /*
      * For one-shot plans, we do not support revalidation checking; it's
      * assumed the query is parsed, planned, and executed in one transaction,
-     * 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).
+     * so that no lock re-acquisition is necessary.  Also, if the statement
+     * type can't require revalidation, we needn't do anything (and we mustn't
+     * risk catalog accesses when handling, eg, transaction control commands).
      */
-    if (plansource->is_oneshot || IsTransactionStmtPlan(plansource))
+    if (plansource->is_oneshot || !StmtPlanRequiresRevalidation(plansource))
     {
         Assert(plansource->is_valid);
         return NIL;
@@ -1029,8 +1031,8 @@ choose_custom_plan(CachedPlanSource *plansource, ParamListInfo boundParams)
     /* Otherwise, never any point in a custom plan if there's no parameters */
     if (boundParams == NULL)
         return false;
-    /* ... nor for transaction control statements */
-    if (IsTransactionStmtPlan(plansource))
+    /* ... nor when planning would be a no-op */
+    if (!StmtPlanRequiresRevalidation(plansource))
         return false;

     /* Let settings force the decision */
@@ -1972,8 +1974,8 @@ PlanCacheRelCallback(Datum arg, Oid relid)
         if (!plansource->is_valid)
             continue;

-        /* Never invalidate transaction control commands */
-        if (IsTransactionStmtPlan(plansource))
+        /* Never invalidate if parse/plan would be a no-op anyway */
+        if (!StmtPlanRequiresRevalidation(plansource))
             continue;

         /*
@@ -2057,8 +2059,8 @@ PlanCacheObjectCallback(Datum arg, int cacheid, uint32 hashvalue)
         if (!plansource->is_valid)
             continue;

-        /* Never invalidate transaction control commands */
-        if (IsTransactionStmtPlan(plansource))
+        /* Never invalidate if parse/plan would be a no-op anyway */
+        if (!StmtPlanRequiresRevalidation(plansource))
             continue;

         /*
@@ -2167,7 +2169,6 @@ ResetPlanCache(void)
     {
         CachedPlanSource *plansource = dlist_container(CachedPlanSource,
                                                        node, iter.cur);
-        ListCell   *lc;

         Assert(plansource->magic == CACHEDPLANSOURCE_MAGIC);

@@ -2179,32 +2180,16 @@ ResetPlanCache(void)
          * We *must not* mark transaction control statements as invalid,
          * particularly not ROLLBACK, because they may need to be executed in
          * aborted transactions when we can't revalidate them (cf bug #5269).
+         * In general there's no point in invalidating statements for which a
+         * new parse analysis/rewrite/plan cycle would certainly give the same
+         * results.
          */
-        if (IsTransactionStmtPlan(plansource))
+        if (!StmtPlanRequiresRevalidation(plansource))
             continue;

-        /*
-         * In general there is no point in invalidating utility statements
-         * since they have no plans anyway.  So invalidate it only if it
-         * contains at least one non-utility statement, or contains a utility
-         * statement that contains a pre-analyzed query (which could have
-         * dependencies.)
-         */
-        foreach(lc, plansource->query_list)
-        {
-            Query       *query = lfirst_node(Query, lc);
-
-            if (query->commandType != CMD_UTILITY ||
-                UtilityContainsQuery(query->utilityStmt))
-            {
-                /* non-utility statement, so invalidate */
-                plansource->is_valid = false;
-                if (plansource->gplan)
-                    plansource->gplan->is_valid = false;
-                /* no need to look further */
-                break;
-            }
-        }
+        plansource->is_valid = false;
+        if (plansource->gplan)
+            plansource->gplan->is_valid = false;
     }

     /* Likewise invalidate cached expressions */
diff --git a/src/include/parser/analyze.h b/src/include/parser/analyze.h
index 1cef1833a6..c96483ae78 100644
--- a/src/include/parser/analyze.h
+++ b/src/include/parser/analyze.h
@@ -47,6 +47,7 @@ extern List *transformUpdateTargetList(ParseState *pstate,
 extern Query *transformTopLevelStmt(ParseState *pstate, RawStmt *parseTree);
 extern Query *transformStmt(ParseState *pstate, Node *parseTree);

+extern bool stmt_requires_parse_analysis(RawStmt *parseTree);
 extern bool analyze_requires_snapshot(RawStmt *parseTree);

 extern const char *LCS_asString(LockClauseStrength strength);
diff --git a/src/pl/plpgsql/src/expected/plpgsql_call.out b/src/pl/plpgsql/src/expected/plpgsql_call.out
index 1ec6182a8d..7ab23c6a21 100644
--- a/src/pl/plpgsql/src/expected/plpgsql_call.out
+++ b/src/pl/plpgsql/src/expected/plpgsql_call.out
@@ -35,6 +35,23 @@ SELECT * FROM test1;
  55
 (1 row)

+-- Check that plan revalidation doesn't prevent setting transaction properties
+-- (bug #18059).  This test must include the first temp-object creation in
+-- this script, or it won't exercise the bug scenario.  Hence we put it early.
+CREATE PROCEDURE test_proc3a()
+LANGUAGE plpgsql
+AS $$
+BEGIN
+   COMMIT;
+   SET TRANSACTION ISOLATION LEVEL REPEATABLE READ;
+   RAISE NOTICE 'done';
+END;
+$$;
+CALL test_proc3a();
+NOTICE:  done
+CREATE TEMP TABLE tt1(f1 int);
+CALL test_proc3a();
+NOTICE:  done
 -- nested CALL
 TRUNCATE TABLE test1;
 CREATE PROCEDURE test_proc4(y int)
diff --git a/src/pl/plpgsql/src/sql/plpgsql_call.sql b/src/pl/plpgsql/src/sql/plpgsql_call.sql
index 5028398348..14bbffa0b2 100644
--- a/src/pl/plpgsql/src/sql/plpgsql_call.sql
+++ b/src/pl/plpgsql/src/sql/plpgsql_call.sql
@@ -38,6 +38,24 @@ CALL test_proc3(55);
 SELECT * FROM test1;


+-- Check that plan revalidation doesn't prevent setting transaction properties
+-- (bug #18059).  This test must include the first temp-object creation in
+-- this script, or it won't exercise the bug scenario.  Hence we put it early.
+CREATE PROCEDURE test_proc3a()
+LANGUAGE plpgsql
+AS $$
+BEGIN
+   COMMIT;
+   SET TRANSACTION ISOLATION LEVEL REPEATABLE READ;
+   RAISE NOTICE 'done';
+END;
+$$;
+
+CALL test_proc3a();
+CREATE TEMP TABLE tt1(f1 int);
+CALL test_proc3a();
+
+
 -- nested CALL
 TRUNCATE TABLE test1;