Avoid unnecessary snapshot-acquisitions in BuildCachedPlan.

I had copied-and-pasted a claim that we couldn't reach this point when
dealing with utility statements, but that was a leftover from when the
caller was required to supply a plan to start with.  We now will go
through here at least once when handling a utility statement, so it
seems worth a check to see whether a snapshot is actually needed.
(Note that analyze_requires_snapshot is quite a cheap test.)

Per suggestion from Yamamoto Takashi.  I don't think I believe that this
resolves his reported assertion failure; but it's worth changing anyway,
just to save a cycle or two.

diff --git a/src/backend/utils/cache/plancache.c b/src/backend/utils/cache/plancache.c
index 9cccc876f81d04c0e3314eefc40f93c550600521..cfeb8245b8c7fc12daa22e7c2ddcbd2a249bb40a 100644 (file)
--- a/src/backend/utils/cache/plancache.c
+++ b/src/backend/utils/cache/plancache.c
@@ -54,6 +54,7 @@
 #include "nodes/nodeFuncs.h"
 #include "optimizer/planmain.h"
 #include "optimizer/prep.h"
+#include "parser/analyze.h"
 #include "parser/parsetree.h"
 #include "storage/lmgr.h"
 #include "tcop/pquery.h"
@@ -756,14 +757,12 @@ BuildCachedPlan(CachedPlanSource *plansource, List *qlist,
    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 a snapshot is already set (the normal case), we can just use that
+    * for planning.  But if it isn't, and we need one, install one.
     */
    snapshot_set = false;
-   if (!ActiveSnapshotSet())
+   if (!ActiveSnapshotSet() &&
+       analyze_requires_snapshot(plansource->raw_parse_tree))
    {
        PushActiveSnapshot(GetTransactionSnapshot());
        snapshot_set = true;