Re: BUG #15060: Row in table not found when using pg function in an expression - Mailing list pgsql-bugs

I wrote:
> Hm.  So we could improve on the fix I proposed earlier if there were a
> way to tell GetCachedPlan "take a new snapshot even though there is an
> active snapshot".  It's still expensive, but at least the code path where
> we use an existing generic plan doesn't get any added cost.

Here's a draft patch along those lines.  Because it changes
GetCachedPlan's API, I'd be a bit worried about back-patching it; there
might be third-party code calling that directly.  However, given that it
took so many years for anyone to notice a problem here, I think maybe
just fixing it in HEAD is fine.

            regards, tom lane

diff --git a/src/backend/commands/prepare.c b/src/backend/commands/prepare.c
index b945b15..8991078 100644
*** a/src/backend/commands/prepare.c
--- b/src/backend/commands/prepare.c
*************** ExecuteQuery(ExecuteStmt *stmt, IntoClau
*** 243,249 ****
                                         entry->plansource->query_string);

      /* Replan if needed, and increment plan refcount for portal */
!     cplan = GetCachedPlan(entry->plansource, paramLI, false, NULL);
      plan_list = cplan->stmt_list;

      /*
--- 243,249 ----
                                         entry->plansource->query_string);

      /* Replan if needed, and increment plan refcount for portal */
!     cplan = GetCachedPlan(entry->plansource, paramLI, false, false, NULL);
      plan_list = cplan->stmt_list;

      /*
*************** ExplainExecuteQuery(ExecuteStmt *execstm
*** 670,676 ****
      }

      /* Replan if needed, and acquire a transient refcount */
!     cplan = GetCachedPlan(entry->plansource, paramLI, true, queryEnv);

      INSTR_TIME_SET_CURRENT(planduration);
      INSTR_TIME_SUBTRACT(planduration, planstart);
--- 670,676 ----
      }

      /* Replan if needed, and acquire a transient refcount */
!     cplan = GetCachedPlan(entry->plansource, paramLI, true, false, queryEnv);

      INSTR_TIME_SET_CURRENT(planduration);
      INSTR_TIME_SUBTRACT(planduration, planstart);
diff --git a/src/backend/executor/spi.c b/src/backend/executor/spi.c
index 9fc4431..502b2e6 100644
*** a/src/backend/executor/spi.c
--- b/src/backend/executor/spi.c
*************** SPI_cursor_open_internal(const char *nam
*** 1289,1296 ****
       * plancache refcount.
       */

!     /* Replan if needed, and increment plan refcount for portal */
!     cplan = GetCachedPlan(plansource, paramLI, false, _SPI_current->queryEnv);
      stmt_list = cplan->stmt_list;

      if (!plan->saved)
--- 1289,1301 ----
       * plancache refcount.
       */

!     /*
!      * Replan if needed, and increment plan refcount for portal.  If
!      * read_only, we can use the current snapshot for replanning, same as
!      * we'll do for execution.  Otherwise best get a new one.
!      */
!     cplan = GetCachedPlan(plansource, paramLI, false, !read_only,
!                           _SPI_current->queryEnv);
      stmt_list = cplan->stmt_list;

      if (!plan->saved)
*************** SPI_plan_get_cached_plan(SPIPlanPtr plan
*** 1722,1729 ****
      spierrcontext.previous = error_context_stack;
      error_context_stack = &spierrcontext;

!     /* Get the generic plan for the query */
!     cplan = GetCachedPlan(plansource, NULL, plan->saved,
                            _SPI_current->queryEnv);
      Assert(cplan == plansource->gplan);

--- 1727,1737 ----
      spierrcontext.previous = error_context_stack;
      error_context_stack = &spierrcontext;

!     /*
!      * Get the generic plan for the query.  Be paranoid and force use of a new
!      * snapshot for any replan activity (in current use, that's uncommon).
!      */
!     cplan = GetCachedPlan(plansource, NULL, plan->saved, true,
                            _SPI_current->queryEnv);
      Assert(cplan == plansource->gplan);

*************** _SPI_execute_plan(SPIPlanPtr plan, Param
*** 2010,2015 ****
--- 2018,2024 ----
      Oid            my_lastoid = InvalidOid;
      SPITupleTable *my_tuptable = NULL;
      int            res = 0;
+     bool        useNewSnapshot;
      bool        pushed_active_snap = false;
      ErrorContextCallback spierrcontext;
      CachedPlan *cplan = NULL;
*************** _SPI_execute_plan(SPIPlanPtr plan, Param
*** 2057,2062 ****
--- 2066,2079 ----
          }
      }

+     /*
+      * In the default non-read-only case, we must get a new snapshot for each
+      * plansource, and also tell GetCachedPlan to get new snapshots.  (Without
+      * the latter, we have anomalies if any stable functions that are
+      * speculatively executed by the planner look at the contents of tables.)
+      */
+     useNewSnapshot = (snapshot == InvalidSnapshot && !read_only);
+
      foreach(lc1, plan->plancache_list)
      {
          CachedPlanSource *plansource = (CachedPlanSource *) lfirst(lc1);
*************** _SPI_execute_plan(SPIPlanPtr plan, Param
*** 2114,2127 ****
           * Replan if needed, and increment plan refcount.  If it's a saved
           * plan, the refcount must be backed by the CurrentResourceOwner.
           */
!         cplan = GetCachedPlan(plansource, paramLI, plan->saved, _SPI_current->queryEnv);
          stmt_list = cplan->stmt_list;

          /*
!          * In the default non-read-only case, get a new snapshot, replacing
!          * any that we pushed in a previous cycle.
           */
!         if (snapshot == InvalidSnapshot && !read_only)
          {
              if (pushed_active_snap)
                  PopActiveSnapshot();
--- 2131,2145 ----
           * Replan if needed, and increment plan refcount.  If it's a saved
           * plan, the refcount must be backed by the CurrentResourceOwner.
           */
!         cplan = GetCachedPlan(plansource, paramLI, plan->saved, useNewSnapshot,
!                               _SPI_current->queryEnv);
          stmt_list = cplan->stmt_list;

          /*
!          * If required, get a new snapshot, replacing any that we pushed in a
!          * previous cycle.
           */
!         if (useNewSnapshot)
          {
              if (pushed_active_snap)
                  PopActiveSnapshot();
diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index 6dc2095..3b8c5c9 100644
*** a/src/backend/tcop/postgres.c
--- b/src/backend/tcop/postgres.c
*************** exec_bind_message(StringInfo input_messa
*** 1794,1800 ****
       * will be generated in MessageContext.  The plan refcount will be
       * assigned to the Portal, so it will be released at portal destruction.
       */
!     cplan = GetCachedPlan(psrc, params, false, NULL);

      /*
       * Now we can define the portal.
--- 1794,1800 ----
       * will be generated in MessageContext.  The plan refcount will be
       * assigned to the Portal, so it will be released at portal destruction.
       */
!     cplan = GetCachedPlan(psrc, params, false, false, NULL);

      /*
       * Now we can define the portal.
diff --git a/src/backend/utils/cache/plancache.c b/src/backend/utils/cache/plancache.c
index 8d7d8e0..b596417 100644
*** a/src/backend/utils/cache/plancache.c
--- b/src/backend/utils/cache/plancache.c
*************** static CachedPlanSource *first_saved_pla
*** 89,98 ****

  static void ReleaseGenericPlan(CachedPlanSource *plansource);
  static List *RevalidateCachedQuery(CachedPlanSource *plansource,
                        QueryEnvironment *queryEnv);
  static bool CheckCachedPlan(CachedPlanSource *plansource);
  static CachedPlan *BuildCachedPlan(CachedPlanSource *plansource, List *qlist,
!                 ParamListInfo boundParams, QueryEnvironment *queryEnv);
  static bool choose_custom_plan(CachedPlanSource *plansource,
                     ParamListInfo boundParams);
  static double cached_plan_cost(CachedPlan *plan, bool include_planner);
--- 89,100 ----

  static void ReleaseGenericPlan(CachedPlanSource *plansource);
  static List *RevalidateCachedQuery(CachedPlanSource *plansource,
+                       bool useNewSnapshot,
                        QueryEnvironment *queryEnv);
  static bool CheckCachedPlan(CachedPlanSource *plansource);
  static CachedPlan *BuildCachedPlan(CachedPlanSource *plansource, List *qlist,
!                 ParamListInfo boundParams, bool useNewSnapshot,
!                 QueryEnvironment *queryEnv);
  static bool choose_custom_plan(CachedPlanSource *plansource,
                     ParamListInfo boundParams);
  static double cached_plan_cost(CachedPlan *plan, bool include_planner);
*************** ReleaseGenericPlan(CachedPlanSource *pla
*** 547,553 ****
   * planning.
   *
   * If any parse analysis activity is required, the caller's memory context is
!  * used for that work.
   *
   * The result value is the transient analyzed-and-rewritten query tree if we
   * had to do re-analysis, and NIL otherwise.  (This is returned just to save
--- 549,556 ----
   * planning.
   *
   * If any parse analysis activity is required, the caller's memory context is
!  * used for that work.  Also, if useNewSnapshot is true or there's no
!  * ActiveSnapshot, we push a new snapshot for that work.
   *
   * The result value is the transient analyzed-and-rewritten query tree if we
   * had to do re-analysis, and NIL otherwise.  (This is returned just to save
*************** ReleaseGenericPlan(CachedPlanSource *pla
*** 555,560 ****
--- 558,564 ----
   */
  static List *
  RevalidateCachedQuery(CachedPlanSource *plansource,
+                       bool useNewSnapshot,
                        QueryEnvironment *queryEnv)
  {
      bool        snapshot_set;
*************** RevalidateCachedQuery(CachedPlanSource *
*** 663,675 ****

      /*
       * 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.
       */
      snapshot_set = false;
!     if (!ActiveSnapshotSet())
      {
          PushActiveSnapshot(GetTransactionSnapshot());
          snapshot_set = true;
--- 667,680 ----

      /*
       * If a snapshot is already set (the normal case), we can just use that
!      * for parsing/planning.  But if it isn't (or if the caller thinks it's
!      * possibly not fresh enough), install a new 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.
       */
      snapshot_set = false;
!     if (useNewSnapshot || !ActiveSnapshotSet())
      {
          PushActiveSnapshot(GetTransactionSnapshot());
          snapshot_set = true;
*************** CheckCachedPlan(CachedPlanSource *planso
*** 873,885 ****
   * each parameter value; otherwise the planner will treat the value as a
   * hint rather than a hard constant.
   *
   * Planning work is done in the caller's memory context.  The finished plan
   * is in a child memory context, which typically should get reparented
   * (unless this is a one-shot plan, in which case we don't copy the plan).
   */
  static CachedPlan *
  BuildCachedPlan(CachedPlanSource *plansource, List *qlist,
!                 ParamListInfo boundParams, QueryEnvironment *queryEnv)
  {
      CachedPlan *plan;
      List       *plist;
--- 878,895 ----
   * each parameter value; otherwise the planner will treat the value as a
   * hint rather than a hard constant.
   *
+  * Pass useNewSnapshot = true to force a new snapshot to be taken for
+  * planning; otherwise, we'll use the ActiveSnapshot if there is one.
+  *
   * Planning work is done in the caller's memory context.  The finished plan
   * is in a child memory context, which typically should get reparented
   * (unless this is a one-shot plan, in which case we don't copy the plan).
   */
  static CachedPlan *
  BuildCachedPlan(CachedPlanSource *plansource, List *qlist,
!                 ParamListInfo boundParams,
!                 bool useNewSnapshot,
!                 QueryEnvironment *queryEnv)
  {
      CachedPlan *plan;
      List       *plist;
*************** BuildCachedPlan(CachedPlanSource *planso
*** 903,909 ****
       * safety, let's treat it as real and redo the RevalidateCachedQuery call.
       */
      if (!plansource->is_valid)
!         qlist = RevalidateCachedQuery(plansource, queryEnv);

      /*
       * If we don't already have a copy of the querytree list that can be
--- 913,919 ----
       * safety, let's treat it as real and redo the RevalidateCachedQuery call.
       */
      if (!plansource->is_valid)
!         qlist = RevalidateCachedQuery(plansource, useNewSnapshot, queryEnv);

      /*
       * If we don't already have a copy of the querytree list that can be
*************** BuildCachedPlan(CachedPlanSource *planso
*** 920,929 ****

      /*
       * 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() &&
          plansource->raw_parse_tree &&
          analyze_requires_snapshot(plansource->raw_parse_tree))
      {
--- 930,940 ----

      /*
       * If a snapshot is already set (the normal case), we can just use that
!      * for planning.  But if it isn't (or if the caller thinks it's possibly
!      * not fresh enough), and we need one, install one.
       */
      snapshot_set = false;
!     if ((useNewSnapshot || !ActiveSnapshotSet()) &&
          plansource->raw_parse_tree &&
          analyze_requires_snapshot(plansource->raw_parse_tree))
      {
*************** cached_plan_cost(CachedPlan *plan, bool
*** 1129,1139 ****
   * only be true if it's a "saved" CachedPlanSource).
   *
   * Note: if any replanning activity is required, the caller's memory context
!  * is used for that work.
   */
  CachedPlan *
  GetCachedPlan(CachedPlanSource *plansource, ParamListInfo boundParams,
!               bool useResOwner, QueryEnvironment *queryEnv)
  {
      CachedPlan *plan = NULL;
      List       *qlist;
--- 1140,1153 ----
   * only be true if it's a "saved" CachedPlanSource).
   *
   * Note: if any replanning activity is required, the caller's memory context
!  * is used for that work.  Also, we will take a new snapshot for that work
!  * if useNewSnapshot is true, and otherwise use the ActiveSnapshot if there
!  * is one.
   */
  CachedPlan *
  GetCachedPlan(CachedPlanSource *plansource, ParamListInfo boundParams,
!               bool useResOwner, bool useNewSnapshot,
!               QueryEnvironment *queryEnv)
  {
      CachedPlan *plan = NULL;
      List       *qlist;
*************** GetCachedPlan(CachedPlanSource *plansour
*** 1147,1153 ****
          elog(ERROR, "cannot apply ResourceOwner to non-saved cached plan");

      /* Make sure the querytree list is valid and we have parse-time locks */
!     qlist = RevalidateCachedQuery(plansource, queryEnv);

      /* Decide whether to use a custom plan */
      customplan = choose_custom_plan(plansource, boundParams);
--- 1161,1167 ----
          elog(ERROR, "cannot apply ResourceOwner to non-saved cached plan");

      /* Make sure the querytree list is valid and we have parse-time locks */
!     qlist = RevalidateCachedQuery(plansource, useNewSnapshot, queryEnv);

      /* Decide whether to use a custom plan */
      customplan = choose_custom_plan(plansource, boundParams);
*************** GetCachedPlan(CachedPlanSource *plansour
*** 1163,1169 ****
          else
          {
              /* Build a new generic plan */
!             plan = BuildCachedPlan(plansource, qlist, NULL, queryEnv);
              /* Just make real sure plansource->gplan is clear */
              ReleaseGenericPlan(plansource);
              /* Link the new generic plan into the plansource */
--- 1177,1184 ----
          else
          {
              /* Build a new generic plan */
!             plan = BuildCachedPlan(plansource, qlist, NULL,
!                                    useNewSnapshot, queryEnv);
              /* Just make real sure plansource->gplan is clear */
              ReleaseGenericPlan(plansource);
              /* Link the new generic plan into the plansource */
*************** GetCachedPlan(CachedPlanSource *plansour
*** 1208,1214 ****
      if (customplan)
      {
          /* Build a custom plan */
!         plan = BuildCachedPlan(plansource, qlist, boundParams, queryEnv);
          /* Accumulate total costs of custom plans, but 'ware overflow */
          if (plansource->num_custom_plans < INT_MAX)
          {
--- 1223,1230 ----
      if (customplan)
      {
          /* Build a custom plan */
!         plan = BuildCachedPlan(plansource, qlist, boundParams,
!                                useNewSnapshot, queryEnv);
          /* Accumulate total costs of custom plans, but 'ware overflow */
          if (plansource->num_custom_plans < INT_MAX)
          {
*************** CachedPlanGetTargetList(CachedPlanSource
*** 1438,1445 ****
      if (plansource->resultDesc == NULL)
          return NIL;

!     /* Make sure the querytree list is valid and we have parse-time locks */
!     RevalidateCachedQuery(plansource, queryEnv);

      /* Get the primary statement and find out what it returns */
      pstmt = QueryListGetPrimaryStmt(plansource->query_list);
--- 1454,1467 ----
      if (plansource->resultDesc == NULL)
          return NIL;

!     /*
!      * Make sure the querytree list is valid and we have parse-time locks.
!      * Force a new snapshot to be taken if we redo parse analysis.  (In some
!      * cases that might be overkill, but existing callers aren't performance
!      * critical, so it seems not worth complicating this function's API by
!      * asking callers to decide whether that's necessary.)
!      */
!     RevalidateCachedQuery(plansource, true, queryEnv);

      /* Get the primary statement and find out what it returns */
      pstmt = QueryListGetPrimaryStmt(plansource->query_list);
diff --git a/src/include/utils/plancache.h b/src/include/utils/plancache.h
index ab20aa0..4930781 100644
*** a/src/include/utils/plancache.h
--- b/src/include/utils/plancache.h
*************** extern List *CachedPlanGetTargetList(Cac
*** 178,184 ****

  extern CachedPlan *GetCachedPlan(CachedPlanSource *plansource,
                ParamListInfo boundParams,
!               bool useResOwner,
                QueryEnvironment *queryEnv);
  extern void ReleaseCachedPlan(CachedPlan *plan, bool useResOwner);

--- 178,184 ----

  extern CachedPlan *GetCachedPlan(CachedPlanSource *plansource,
                ParamListInfo boundParams,
!               bool useResOwner, bool useNewSnapshot,
                QueryEnvironment *queryEnv);
  extern void ReleaseCachedPlan(CachedPlan *plan, bool useResOwner);

diff --git a/src/test/regress/expected/plpgsql.out b/src/test/regress/expected/plpgsql.out
index 4f9501d..a1ea306 100644
*** a/src/test/regress/expected/plpgsql.out
--- b/src/test/regress/expected/plpgsql.out
*************** select sp_add_user('user3');
*** 2238,2243 ****
--- 2238,2281 ----
  drop function sp_add_user(text);
  drop function sp_id_user(text);
  --
+ -- Variant of above: check for sane behavior during planner's speculative
+ -- execution of stable functions (bug #15060)
+ --
+ create function sp_id_user(a_login text) returns int as $$
+ declare x int;
+ begin
+   select into x id from users where login = a_login;
+   if not found then raise exception '% not found', a_login; end if;
+   return x;
+ end$$ language plpgsql stable;
+ select sp_id_user('user1');
+  sp_id_user
+ ------------
+           1
+ (1 row)
+
+ select sp_id_user('userx');
+ ERROR:  userx not found
+ CONTEXT:  PL/pgSQL function sp_id_user(text) line 5 at RAISE
+ create function sp_add_user(a_login text) returns int as $$
+ declare my_id_user int;
+ begin
+   INSERT INTO users ( login ) VALUES ( a_login );
+   SELECT id INTO my_id_user FROM users WHERE id = sp_id_user( a_login );
+   IF NOT FOUND THEN
+     RETURN -1;  -- error code for insertion failure
+   END IF;
+   RETURN my_id_user;
+ end$$ language plpgsql;
+ select sp_add_user('user4');
+  sp_add_user
+ -------------
+            4
+ (1 row)
+
+ drop function sp_add_user(text);
+ drop function sp_id_user(text);
+ --
  -- tests for refcursors
  --
  create table rc_test (a int, b int);
diff --git a/src/test/regress/sql/plpgsql.sql b/src/test/regress/sql/plpgsql.sql
index 3914651..411b7b5 100644
*** a/src/test/regress/sql/plpgsql.sql
--- b/src/test/regress/sql/plpgsql.sql
*************** drop function sp_add_user(text);
*** 1901,1906 ****
--- 1901,1938 ----
  drop function sp_id_user(text);

  --
+ -- Variant of above: check for sane behavior during planner's speculative
+ -- execution of stable functions (bug #15060)
+ --
+
+ create function sp_id_user(a_login text) returns int as $$
+ declare x int;
+ begin
+   select into x id from users where login = a_login;
+   if not found then raise exception '% not found', a_login; end if;
+   return x;
+ end$$ language plpgsql stable;
+
+ select sp_id_user('user1');
+ select sp_id_user('userx');
+
+ create function sp_add_user(a_login text) returns int as $$
+ declare my_id_user int;
+ begin
+   INSERT INTO users ( login ) VALUES ( a_login );
+   SELECT id INTO my_id_user FROM users WHERE id = sp_id_user( a_login );
+   IF NOT FOUND THEN
+     RETURN -1;  -- error code for insertion failure
+   END IF;
+   RETURN my_id_user;
+ end$$ language plpgsql;
+
+ select sp_add_user('user4');
+
+ drop function sp_add_user(text);
+ drop function sp_id_user(text);
+
+ --
  -- tests for refcursors
  --
  create table rc_test (a int, b int);