Обсуждение: BUG #15060: Row in table not found when using pg function in anexpression

The following bug has been logged on the website:

Bug reference:      15060
Logged by:          Dejan Petrovic
Email address:      dejan.petrovic@islonline.com
PostgreSQL version: 10.2
Operating system:   CentOS 6
Description:

Hello,

I tested this in postgresql versions 9.1, 10.1 and 10.2 on centOS. 

In short this is what happens (in a plpgsql function):
1.) An insert is done into 'bug' table
2.) A SELECT is done to make sure the INSERT was successful
3.) Another function (get_bug_id) is called which returns id based on
value.
When the function is called directly, it returns the id correctly. When it's
called in an expression, it does not find the inserted row and an exception
is raised.

I have prepared a minimal example:

CREATE TABLE public.bug
(
  id integer NOT NULL,
  text text,
  CONSTRAINT bug_pkey PRIMARY KEY (id)
)
WITH (
  OIDS=FALSE
);
ALTER TABLE public.bug
  OWNER TO postgres;

CREATE OR REPLACE FUNCTION public.get_bug_id(in_text text)
  RETURNS integer AS
$BODY$
DECLARE 
    my_int int;
BEGIN
  SELECT INTO  my_int id from bug WHERE text = in_text;
  IF NOT FOUND THEN
    RAISE EXCEPTION 'row not found - BUG?';
  END IF;

RETURN my_int;
END;
$BODY$
  LANGUAGE plpgsql STABLE
  COST 100;
ALTER FUNCTION public.get_bug_id(text)
  OWNER TO postgres;

CREATE OR REPLACE FUNCTION test_bug()
  RETURNS text AS
$BODY$
DECLARE
my_int int;
my_text text;
BEGIN
my_text := 'this is a bug';

INSERT INTO bug (id,text) VALUES (1,my_text); 

SELECT INTO my_int id from bug WHERE text = my_text;
IF NOT FOUND THEN
        RAISE EXCEPTION 'row does not exist';
END IF;
perform get_bug_id(my_text); -- This is OK - get_bug_id returns '1'
perform id FROM bug WHERE id = get_bug_id(my_text); -- This fails -
get_bug_id raises exception in version 10, works OK in version 9.1
RETURN 'OK';
END;
$BODY$
  LANGUAGE plpgsql VOLATILE
  COST 100;
ALTER FUNCTION test_bug()
  OWNER TO postgres;

 select test_bug()

PG Bug reporting form wrote
> I tested this in postgresql versions 9.1, 10.1 and 10.2 on centOS. 
> 
> In short this is what happens (in a plpgsql function):
> 1.) An insert is done into 'bug' table
> 2.) A SELECT is done to make sure the INSERT was successful
> 3.) Another function (get_bug_id) is called which returns id based on
> value.
> When the function is called directly, it returns the id correctly. When
> it's
> called in an expression, it does not find the inserted row and an
> exception
> is raised.

Significant is that function get_bug_id() being STABLE is necessary for the
bug to occur. 
It appears to be not a bug, since documentation reads:

> For functions written in SQL or in any of the standard procedural
> languages, there is a second important property determined by the
> volatility category, 
*
> namely the visibility of any data changes that have been made by the SQL
> command that is calling the function. A VOLATILE function will see such
> changes, a STABLE or IMMUTABLE function will not
*
> . This behavior is implemented using the snapshotting behavior of MVCC
> (see Chapter 13): STABLE and IMMUTABLE functions use a snapshot
> established as of the start of the calling query, whereas VOLATILE
> functions obtain a fresh snapshot at the start of each query they execute.

Although i.m.o. it is strange that one produces the proper result and the
other call does not.




--
Sent from: http://www.postgresql-archive.org/PostgreSQL-bugs-f2117394.html

Marko Tiikkaja-4 wrote
> I don't think this is relevant, since the changes were NOT made by the SQL
> command calling the function.  They were made by the INSERT which executed
> earlier in a VOLATILE function.

Yes I should have read a few lines further, and wake up:

> Because of this snapshotting behavior, a function containing only SELECT
> commands can safely be marked STABLE, even if it selects from tables that
> might be undergoing modifications by concurrent queries. PostgreSQL will
> execute all commands of a STABLE function using the snapshot established
> for the calling query, and so it will see a fixed view of the database
> throughout that query.

Sorry for inconvenience. Hope the VOLATILE-STABLE difference in behavior
helps in resolving this bug.





--
Sent from: http://www.postgresql-archive.org/PostgreSQL-bugs-f2117394.html

>>>>> "PG" == PG Bug reporting form <noreply@postgresql.org> writes:

 PG> I tested this in postgresql versions 9.1, 10.1 and 10.2 on centOS. 

 PG> In short this is what happens (in a plpgsql function):
 PG> 1.) An insert is done into 'bug' table
 PG> 2.) A SELECT is done to make sure the INSERT was successful
 PG> 3.) Another function (get_bug_id) is called which returns id based on
 PG> value.
 PG> When the function is called directly, it returns the id correctly.
 PG> When it's called in an expression, it does not find the inserted
 PG> row and an exception is raised.

So what's happening here is that the function get_bug_id, being stable,
is being called speculatively at plan time for the query where it
appears in the WHERE clause. For whatever reason, the snapshot it's
being run in at that time is not the same one actually used for the
later execution of the query, and the plan-time snapshot doesn't see the
just-inserted row.

It looks like what's going on here is that SPI does GetCachedPlan -
which is where planning will happen - _before_ establishing the new
snapshot in the non-read-only case (read_only is false here because the
calling function, test_bug(), is volatile).

-- 
Andrew (irc:RhodiumToad)

Andrew Gierth <andrew@tao11.riddles.org.uk> writes:
> So what's happening here is that the function get_bug_id, being stable,
> is being called speculatively at plan time for the query where it
> appears in the WHERE clause. For whatever reason, the snapshot it's
> being run in at that time is not the same one actually used for the
> later execution of the query, and the plan-time snapshot doesn't see the
> just-inserted row.

> It looks like what's going on here is that SPI does GetCachedPlan -
> which is where planning will happen - _before_ establishing the new
> snapshot in the non-read-only case (read_only is false here because the
> calling function, test_bug(), is volatile).

Yeah, I came to the same conclusion.  I think it's basically accidental
that the given test case works before 9.2: the reason seems to be that
in 9.1, the plancache doesn't pass through the parameter list containing
the value of "my_text", so that the planner is unable to speculatively
execute get_bug_id().  The order of operations in _SPI_execute_plan
is just as wrong though.

The attached patch makes it better and doesn't seem to break any
regression tests.  I'm not sure how nervous I am about unexpected
side-effects ...

            regards, tom lane

diff --git a/src/backend/executor/spi.c b/src/backend/executor/spi.c
index 9fc4431..45e62ba 100644
*** a/src/backend/executor/spi.c
--- b/src/backend/executor/spi.c
*************** _SPI_execute_plan(SPIPlanPtr plan, Param
*** 2066,2071 ****
--- 2066,2083 ----
          spierrcontext.arg = (void *) plansource->query_string;

          /*
+          * 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();
+             PushActiveSnapshot(GetTransactionSnapshot());
+             pushed_active_snap = true;
+         }
+
+         /*
           * If this is a one-shot plan, we still need to do parse analysis.
           */
          if (plan->oneshot)
*************** _SPI_execute_plan(SPIPlanPtr plan, Param
*** 2117,2134 ****
          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();
-             PushActiveSnapshot(GetTransactionSnapshot());
-             pushed_active_snap = true;
-         }
-
          foreach(lc2, stmt_list)
          {
              PlannedStmt *stmt = lfirst_node(PlannedStmt, lc2);
--- 2129,2134 ----

I wrote:
> The attached patch makes it better and doesn't seem to break any
> regression tests.  I'm not sure how nervous I am about unexpected
> side-effects ...

Oh ... my nervousness was justified.  This is no good at all; it'd
create the same problem introduced at the client query level by
commit d573e239f and later reverted by 532994299, namely that we'd
be reading tables using a snapshot acquired before we've acquired
locks on said tables.

To really fix this complaint, it seems like we'd need to take a snapshot
before planning and then a new one after planning, matching what happens
in the main query pipeline.  That's pretty nasty for performance :-(
... it would more or less double the load on the ProcArray for SPI-heavy
applications.

One could argue that, since this function is throwing an error, it's
not really stable --- that implies a lack of interesting side-effects.
I don't find that argument totally convincing, but the price of avoiding
the error might be more than we really want to pay.

            regards, tom lane

>>>>> "Marko" == Marko Tiikkaja <marko@joh.to> writes:

 >>> It looks like what's going on here is that SPI does GetCachedPlan -
 >>> which is where planning will happen - _before_ establishing the new
 >>> snapshot in the non-read-only case (read_only is false here because
 >>> the calling function, test_bug(), is volatile).

 >> Yeah, I came to the same conclusion. I think it's basically
 >> accidental that the given test case works before 9.2: the reason
 >> seems to be that in 9.1, the plancache doesn't pass through the
 >> parameter list containing the value of "my_text", so that the
 >> planner is unable to speculatively execute get_bug_id(). The order
 >> of operations in _SPI_execute_plan is just as wrong though.

 Marko> I'm not sure I understand. When's the snapshot used for planning
 Marko> actually taken here?

GetCachedPlan will use either whatever snapshot is already set, if there
is one, or it will set one of its own (actually at least two: separate
snapshots for revalidate + parse analysis and for planning).

In the case of a volatile plpgsql function, the snapshot in which the
function was called will, I believe, still be the active snapshot at the
relevant point, so calls made in planning won't see the function's own
changes.

The recent introduction of procedures exposes this interesting little
variation in behavior (pg11 only):

create table bug (id integer, d text);
create or replace function getbug(text) returns integer
  language plpgsql stable
  as $$
    declare
      b_id integer;
    begin
      select into b_id id from bug where d = $1;
      if not found then
        raise info 'bug % not found',$1;
      else
        raise info 'bug % id %',$1,b_id;
      end if;
      return b_id;
    end;
$$;

truncate table bug;
do $$
  begin
    insert into bug values (1,'foo');
    perform * from bug where id = getbug('foo');
  end;
$$;
INFO:  bug foo not found
INFO:  bug foo id 1

truncate table bug;
do $$
  begin
    commit;
    insert into bug values (1,'foo');
    perform * from bug where id = getbug('foo');
  end;
$$;
INFO:  bug foo id 1
INFO:  bug foo id 1

I assume that what's going on here is that the commit, which ends the
transaction in which the DO was invoked and begins a new one, doesn't
set a new active snapshot in the new transaction, and so planning of the
perform in the second case is taking new snapshots inside GetCachedPlan.

-- 
Andrew (irc:RhodiumToad)

Andrew Gierth <andrew@tao11.riddles.org.uk> writes:
> "Marko" == Marko Tiikkaja <marko@joh.to> writes:
>  Marko> I'm not sure I understand. When's the snapshot used for planning
>  Marko> actually taken here?

> GetCachedPlan will use either whatever snapshot is already set, if there
> is one, or it will set one of its own (actually at least two: separate
> snapshots for revalidate + parse analysis and for planning).

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.

CC'ing Peter because of this part:

> The recent introduction of procedures exposes this interesting little
> variation in behavior (pg11 only):
> ...
> I assume that what's going on here is that the commit, which ends the
> transaction in which the DO was invoked and begins a new one, doesn't
> set a new active snapshot in the new transaction, and so planning of the
> perform in the second case is taking new snapshots inside GetCachedPlan.

I find this concerning.  No part of plpgsql, or any other PL, has ever
before executed without the constant presence of an ActiveSnapshot.
It seems likely to me that this will expose, if not outright bugs,
at least strange differences in behavior between "procedure" code and
"function" code.  Perhaps the commit-and-restart-transaction code ought
to include pushing a new ActiveSnapshot.

            regards, tom lane

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);