Re: Patch: plan invalidation vs stored procedures - Mailing list pgsql-hackers

Tom Lane wrote:
> I hadn't read it yet, but that makes it wrong already.  There's no need
> for any new inval traffic --- the existing syscache inval messages on
> pg_proc entries should serve fine.

Yes, creating a new message type was a bit short sighted -- attached is a patch
that uses syscache invalidation messages instead. This also adds additional
tupleId field to SharedInvalCatcacheMsg. This is used to identify the
invalidated tuple in PROC messages, for now others still pass InvalidOid.

> More generally, if we are to try to invalidate on the strength of
> pg_proc changes, what of other DDL changes?  Operators, operator
> classes, maybe?  How about renaming a schema?  I would like to see a
> line drawn between things we find worth trying to track and things we
> don't.  If there is no such line, we're going to need a patch a lot
> larger than this one.

The attached patch registers callbacks for namespace, operator and op family
catalog changes. PlanCacheCallback now takes catalog id as arg and can
take actions depending on the catalog type. Adding new catalogs is just a
matter of registering the callback in InitPlanCache. Of course, only tables
and functions have exact tracking -- other changes just invalidate all.

I'm wondering if the list of catalogs to be tracked should be fixed at all.
Maybe it would be better to call PlanCacheCallback directly on any syscache
entry invalidation? This way no catalog would be overlooked and the
cache_callback_list could be kept nice and short. PlanCacheCallback would
receive the catalog id and OID of the invalidated tuple and could then
decide whether it can do precise invalidation, flush the cache or just
skip the event. What do you think?

regards,
Martin

Index: src/backend/commands/prepare.c
===================================================================
RCS file: /projects/cvsroot/pgsql/src/backend/commands/prepare.c,v
retrieving revision 1.90
diff -c -r1.90 prepare.c
*** src/backend/commands/prepare.c    25 Aug 2008 22:42:32 -0000    1.90
--- src/backend/commands/prepare.c    28 Aug 2008 09:20:21 -0000
***************
*** 189,197 ****
      /* Shouldn't have a non-fully-planned plancache entry */
      if (!entry->plansource->fully_planned)
          elog(ERROR, "EXECUTE does not support unplanned prepared statements");
-     /* Shouldn't get any non-fixed-result cached plan, either */
-     if (!entry->plansource->fixed_result)
-         elog(ERROR, "EXECUTE does not support variable-result cached plans");

      /* Evaluate parameters, if any */
      if (entry->plansource->num_params > 0)
--- 189,194 ----
***************
*** 463,469 ****
                                    cursor_options,
                                    stmt_list,
                                    true,
!                                   true);

      /* Now we can add entry to hash table */
      entry = (PreparedStatement *) hash_search(prepared_queries,
--- 460,466 ----
                                    cursor_options,
                                    stmt_list,
                                    true,
!                                   false);

      /* Now we can add entry to hash table */
      entry = (PreparedStatement *) hash_search(prepared_queries,
***************
*** 524,534 ****
  TupleDesc
  FetchPreparedStatementResultDesc(PreparedStatement *stmt)
  {
!     /*
!      * Since we don't allow prepared statements' result tupdescs to change,
!      * there's no need for a revalidate call here.
!      */
!     Assert(stmt->plansource->fixed_result);
      if (stmt->plansource->resultDesc)
          return CreateTupleDescCopy(stmt->plansource->resultDesc);
      else
--- 521,529 ----
  TupleDesc
  FetchPreparedStatementResultDesc(PreparedStatement *stmt)
  {
!     /* Revalidate the plan to allow changes in tupdescs. */
!     RevalidateCachedPlan(stmt->plansource, false);
!
      if (stmt->plansource->resultDesc)
          return CreateTupleDescCopy(stmt->plansource->resultDesc);
      else
***************
*** 650,658 ****
      /* Shouldn't have a non-fully-planned plancache entry */
      if (!entry->plansource->fully_planned)
          elog(ERROR, "EXPLAIN EXECUTE does not support unplanned prepared statements");
-     /* Shouldn't get any non-fixed-result cached plan, either */
-     if (!entry->plansource->fixed_result)
-         elog(ERROR, "EXPLAIN EXECUTE does not support variable-result cached plans");

      /* Replan if needed, and acquire a transient refcount */
      cplan = RevalidateCachedPlan(entry->plansource, true);
--- 645,650 ----
Index: src/backend/nodes/copyfuncs.c
===================================================================
RCS file: /projects/cvsroot/pgsql/src/backend/nodes/copyfuncs.c,v
retrieving revision 1.401
diff -c -r1.401 copyfuncs.c
*** src/backend/nodes/copyfuncs.c    22 Aug 2008 00:16:03 -0000    1.401
--- src/backend/nodes/copyfuncs.c    28 Aug 2008 09:20:21 -0000
***************
*** 84,89 ****
--- 84,90 ----
      COPY_NODE_FIELD(returningLists);
      COPY_NODE_FIELD(rowMarks);
      COPY_NODE_FIELD(relationOids);
+     COPY_NODE_FIELD(functionOids);
      COPY_SCALAR_FIELD(nParamExec);

      return newnode;
***************
*** 1882,1887 ****
--- 1883,1889 ----
      COPY_NODE_FIELD(limitCount);
      COPY_NODE_FIELD(rowMarks);
      COPY_NODE_FIELD(setOperations);
+     COPY_NODE_FIELD(functionOids);

      return newnode;
  }
Index: src/backend/optimizer/plan/planner.c
===================================================================
RCS file: /projects/cvsroot/pgsql/src/backend/optimizer/plan/planner.c,v
retrieving revision 1.242
diff -c -r1.242 planner.c
*** src/backend/optimizer/plan/planner.c    17 Aug 2008 01:19:59 -0000    1.242
--- src/backend/optimizer/plan/planner.c    28 Aug 2008 09:20:22 -0000
***************
*** 214,219 ****
--- 214,220 ----
      result->rowMarks = parse->rowMarks;
      result->relationOids = glob->relationOids;
      result->nParamExec = list_length(glob->paramlist);
+     result->functionOids = parse->functionOids;

      return result;
  }
Index: src/backend/parser/analyze.c
===================================================================
RCS file: /projects/cvsroot/pgsql/src/backend/parser/analyze.c,v
retrieving revision 1.377
diff -c -r1.377 analyze.c
*** src/backend/parser/analyze.c    25 Aug 2008 22:42:33 -0000    1.377
--- src/backend/parser/analyze.c    28 Aug 2008 09:20:23 -0000
***************
*** 226,231 ****
--- 226,234 ----
      result->querySource = QSRC_ORIGINAL;
      result->canSetTag = true;

+     /* Add the function oid list to Query */
+     result->functionOids = pstate->p_functionOids;
+
      return result;
  }

Index: src/backend/parser/parse_func.c
===================================================================
RCS file: /projects/cvsroot/pgsql/src/backend/parser/parse_func.c,v
retrieving revision 1.205
diff -c -r1.205 parse_func.c
*** src/backend/parser/parse_func.c    25 Aug 2008 22:42:33 -0000    1.205
--- src/backend/parser/parse_func.c    28 Aug 2008 09:20:23 -0000
***************
*** 323,328 ****
--- 323,332 ----
                       parser_errposition(pstate, location)));
      }

+     /* add the function Oid to ParseState - this is later copied to Query */
+     if (!list_member_oid(pstate->p_functionOids, funcid))
+         pstate->p_functionOids = lappend_oid(pstate->p_functionOids, funcid);
+
      return retval;
  }

Index: src/backend/tcop/postgres.c
===================================================================
RCS file: /projects/cvsroot/pgsql/src/backend/tcop/postgres.c,v
retrieving revision 1.556
diff -c -r1.556 postgres.c
*** src/backend/tcop/postgres.c    19 Aug 2008 18:30:04 -0000    1.556
--- src/backend/tcop/postgres.c    28 Aug 2008 09:20:25 -0000
***************
*** 2177,2184 ****
                       errmsg("unnamed prepared statement does not exist")));
      }

!     /* Prepared statements shouldn't have changeable result descs */
!     Assert(psrc->fixed_result);

      /*
       * If we are in aborted transaction state, we can't run
--- 2177,2184 ----
                       errmsg("unnamed prepared statement does not exist")));
      }

!     /* Revalidate the plan to allow tupdesc changes */
!     RevalidateCachedPlan(psrc, true);

      /*
       * If we are in aborted transaction state, we can't run
Index: src/backend/utils/cache/catcache.c
===================================================================
RCS file: /projects/cvsroot/pgsql/src/backend/utils/cache/catcache.c,v
retrieving revision 1.144
diff -c -r1.144 catcache.c
*** src/backend/utils/cache/catcache.c    19 Jun 2008 00:46:05 -0000    1.144
--- src/backend/utils/cache/catcache.c    28 Aug 2008 09:20:25 -0000
***************
*** 1757,1763 ****
  void
  PrepareToInvalidateCacheTuple(Relation relation,
                                HeapTuple tuple,
!                             void (*function) (int, uint32, ItemPointer, Oid))
  {
      CatCache   *ccp;
      Oid            reloid;
--- 1757,1763 ----
  void
  PrepareToInvalidateCacheTuple(Relation relation,
                                HeapTuple tuple,
!                             void (*function) (int, uint32, ItemPointer, Oid, Oid))
  {
      CatCache   *ccp;
      Oid            reloid;
***************
*** 1794,1800 ****
          (*function) (ccp->id,
                       CatalogCacheComputeTupleHashValue(ccp, tuple),
                       &tuple->t_self,
!                      ccp->cc_relisshared ? (Oid) 0 : MyDatabaseId);
      }
  }

--- 1794,1801 ----
          (*function) (ccp->id,
                       CatalogCacheComputeTupleHashValue(ccp, tuple),
                       &tuple->t_self,
!                      ccp->cc_relisshared ? (Oid) 0 : MyDatabaseId,
!                      (ccp->id == PROCOID) ? HeapTupleGetOid(tuple) : InvalidOid);
      }
  }

Index: src/backend/utils/cache/inval.c
===================================================================
RCS file: /projects/cvsroot/pgsql/src/backend/utils/cache/inval.c,v
retrieving revision 1.86
diff -c -r1.86 inval.c
*** src/backend/utils/cache/inval.c    19 Jun 2008 21:32:56 -0000    1.86
--- src/backend/utils/cache/inval.c    28 Aug 2008 09:20:26 -0000
***************
*** 307,319 ****
  static void
  AddCatcacheInvalidationMessage(InvalidationListHeader *hdr,
                                 int id, uint32 hashValue,
!                                ItemPointer tuplePtr, Oid dbId)
  {
      SharedInvalidationMessage msg;

      msg.cc.id = (int16) id;
      msg.cc.tuplePtr = *tuplePtr;
      msg.cc.dbId = dbId;
      msg.cc.hashValue = hashValue;
      AddInvalidationMessage(&hdr->cclist, &msg);
  }
--- 307,320 ----
  static void
  AddCatcacheInvalidationMessage(InvalidationListHeader *hdr,
                                 int id, uint32 hashValue,
!                                ItemPointer tuplePtr, Oid dbId, Oid tupleId)
  {
      SharedInvalidationMessage msg;

      msg.cc.id = (int16) id;
      msg.cc.tuplePtr = *tuplePtr;
      msg.cc.dbId = dbId;
+     msg.cc.tupleId = tupleId;
      msg.cc.hashValue = hashValue;
      AddInvalidationMessage(&hdr->cclist, &msg);
  }
***************
*** 414,423 ****
  RegisterCatcacheInvalidation(int cacheId,
                               uint32 hashValue,
                               ItemPointer tuplePtr,
!                              Oid dbId)
  {
      AddCatcacheInvalidationMessage(&transInvalInfo->CurrentCmdInvalidMsgs,
!                                    cacheId, hashValue, tuplePtr, dbId);
  }

  /*
--- 415,424 ----
  RegisterCatcacheInvalidation(int cacheId,
                               uint32 hashValue,
                               ItemPointer tuplePtr,
!                              Oid dbId, Oid tupleId)
  {
      AddCatcacheInvalidationMessage(&transInvalInfo->CurrentCmdInvalidMsgs,
!                                    cacheId, hashValue, tuplePtr, dbId, tupleId);
  }

  /*
***************
*** 489,495 ****
                  struct CACHECALLBACK *ccitem = cache_callback_list + i;

                  if (ccitem->id == msg->cc.id)
!                     (*ccitem->function) (ccitem->arg, InvalidOid);
              }
          }
      }
--- 490,496 ----
                  struct CACHECALLBACK *ccitem = cache_callback_list + i;

                  if (ccitem->id == msg->cc.id)
!                     (*ccitem->function) (ccitem->arg, msg->cc.tupleId);
              }
          }
      }
Index: src/backend/utils/cache/plancache.c
===================================================================
RCS file: /projects/cvsroot/pgsql/src/backend/utils/cache/plancache.c,v
retrieving revision 1.20
diff -c -r1.20 plancache.c
*** src/backend/utils/cache/plancache.c    25 Aug 2008 22:42:34 -0000    1.20
--- src/backend/utils/cache/plancache.c    28 Aug 2008 09:20:26 -0000
***************
*** 52,57 ****
--- 52,58 ----
  #include "utils/memutils.h"
  #include "utils/resowner.h"
  #include "utils/snapmgr.h"
+ #include "utils/syscache.h"


  typedef struct
***************
*** 81,87 ****
  static bool ScanQueryWalker(Node *node, ScanQueryWalkerContext *context);
  static bool rowmark_member(List *rowMarks, int rt_index);
  static bool plan_list_is_transient(List *stmt_list);
! static void PlanCacheCallback(Datum arg, Oid relid);
  static void InvalRelid(Oid relid, LOCKMODE lockmode,
             InvalRelidContext *context);

--- 82,88 ----
  static bool ScanQueryWalker(Node *node, ScanQueryWalkerContext *context);
  static bool rowmark_member(List *rowMarks, int rt_index);
  static bool plan_list_is_transient(List *stmt_list);
! static void PlanCacheCallback(Datum arg, Oid objId);
  static void InvalRelid(Oid relid, LOCKMODE lockmode,
             InvalRelidContext *context);

***************
*** 94,100 ****
  void
  InitPlanCache(void)
  {
!     CacheRegisterRelcacheCallback(PlanCacheCallback, (Datum) 0);
  }

  /*
--- 95,105 ----
  void
  InitPlanCache(void)
  {
!     CacheRegisterRelcacheCallback(PlanCacheCallback, Int32GetDatum(RELOID));
!     CacheRegisterSyscacheCallback(PROCOID, PlanCacheCallback, Int32GetDatum(PROCOID));
!     CacheRegisterSyscacheCallback(NAMESPACEOID, PlanCacheCallback, Int32GetDatum(NAMESPACEOID));
!     CacheRegisterSyscacheCallback(OPEROID, PlanCacheCallback, Int32GetDatum(OPEROID));
!     CacheRegisterSyscacheCallback(OPFAMILYOID, PlanCacheCallback, Int32GetDatum(OPFAMILYOID));
  }

  /*
***************
*** 866,877 ****
   * PlanCacheCallback
   *        Relcache inval callback function
   *
!  * Invalidate all plans mentioning the given rel, or all plans mentioning
!  * any rel at all if relid == InvalidOid.
   */
  static void
! PlanCacheCallback(Datum arg, Oid relid)
  {
      ListCell   *lc1;
      ListCell   *lc2;

--- 871,883 ----
   * PlanCacheCallback
   *        Relcache inval callback function
   *
!  * Invalidate all plans mentioning the given OID, or all plans
!  * if objId == InvalidOid.
   */
  static void
! PlanCacheCallback(Datum arg, Oid objId)
  {
+     int            catId = DatumGetInt32(arg);
      ListCell   *lc1;
      ListCell   *lc2;

***************
*** 883,925 ****
          /* No work if it's already invalidated */
          if (!plan || plan->dead)
              continue;
-         if (plan->fully_planned)
-         {
-             foreach(lc2, plan->stmt_list)
-             {
-                 PlannedStmt *plannedstmt = (PlannedStmt *) lfirst(lc2);

!                 Assert(!IsA(plannedstmt, Query));
!                 if (!IsA(plannedstmt, PlannedStmt))
!                     continue;    /* Ignore utility statements */
!                 if ((relid == InvalidOid) ? plannedstmt->relationOids != NIL :
!                     list_member_oid(plannedstmt->relationOids, relid))
!                 {
!                     /* Invalidate the plan! */
!                     plan->dead = true;
!                     break;        /* out of stmt_list scan */
!                 }
!             }
!         }
!         else
          {
!             /*
!              * For not-fully-planned entries we use ScanQueryForRelids, since
!              * a recursive traversal is needed.  The callback API is a bit
!              * tedious but avoids duplication of coding.
!              */
!             InvalRelidContext context;

!             context.inval_relid = relid;
!             context.plan = plan;
!
!             foreach(lc2, plan->stmt_list)
              {
!                 Query       *query = (Query *) lfirst(lc2);

!                 Assert(IsA(query, Query));
!                 ScanQueryForRelids(query, InvalRelid, (void *) &context);
              }
          }
      }
  }
--- 889,934 ----
          /* No work if it's already invalidated */
          if (!plan || plan->dead)
              continue;

!         foreach(lc2, plan->stmt_list)
          {
!             PlannedStmt *plannedstmt = (PlannedStmt *) lfirst(lc2);

!             Assert(!IsA(plannedstmt, Query));
!             if (!IsA(plannedstmt, PlannedStmt))
!                 continue;    /* Ignore utility statements */
!
!             if (objId == InvalidOid)
!                 /* InvalidOid - invalidate all */
!                 plan->dead = true;
!             else if (catId == PROCOID)
!                 /* Invalidate if we have this function */
!                 plan->dead = list_member_oid(plannedstmt->functionOids, objId);
!             else if (catId == RELOID)
              {
!                 /* Invalidate if we have this relation */
!                 if (plan->fully_planned)
!                     plan->dead = list_member_oid(plannedstmt->relationOids, objId);
!                 else
!                 {
!                     /*
!                      * For not-fully-planned entries we use ScanQueryForRelids,
!                      * since a recursive traversal is needed.  The callback API
!                      * is a bit tedious but avoids duplication of coding.
!                      */
!                     InvalRelidContext context;
!                     Query *query = (Query *) lfirst(lc2);
!
!                     context.inval_relid = objId;
!                     context.plan = plan;

!                     Assert(IsA(query, Query));
!                     ScanQueryForRelids(query, InvalRelid, (void *) &context);
!                 }
              }
+
+             if (plan->dead)
+                 break;        /* out of stmt_list scan */
          }
      }
  }
Index: src/include/nodes/parsenodes.h
===================================================================
RCS file: /projects/cvsroot/pgsql/src/include/nodes/parsenodes.h,v
retrieving revision 1.371
diff -c -r1.371 parsenodes.h
*** src/include/nodes/parsenodes.h    7 Aug 2008 01:11:51 -0000    1.371
--- src/include/nodes/parsenodes.h    28 Aug 2008 09:20:28 -0000
***************
*** 132,137 ****
--- 132,139 ----

      Node       *setOperations;    /* set-operation tree if this is top level of
                                   * a UNION/INTERSECT/EXCEPT query */
+
+     List       *functionOids;    /* OIDs of functions the query references */
  } Query;


Index: src/include/nodes/plannodes.h
===================================================================
RCS file: /projects/cvsroot/pgsql/src/include/nodes/plannodes.h,v
retrieving revision 1.102
diff -c -r1.102 plannodes.h
*** src/include/nodes/plannodes.h    7 Aug 2008 19:35:02 -0000    1.102
--- src/include/nodes/plannodes.h    28 Aug 2008 09:20:28 -0000
***************
*** 72,77 ****
--- 72,79 ----

      List       *relationOids;    /* OIDs of relations the plan depends on */

+     List       *functionOids;    /* OIDs of functions the plan depends on */
+
      int            nParamExec;        /* number of PARAM_EXEC Params used */
  } PlannedStmt;

Index: src/include/parser/parse_node.h
===================================================================
RCS file: /projects/cvsroot/pgsql/src/include/parser/parse_node.h,v
retrieving revision 1.54
diff -c -r1.54 parse_node.h
*** src/include/parser/parse_node.h    19 Jun 2008 00:46:06 -0000    1.54
--- src/include/parser/parse_node.h    28 Aug 2008 09:20:28 -0000
***************
*** 80,85 ****
--- 80,86 ----
      bool        p_is_update;
      Relation    p_target_relation;
      RangeTblEntry *p_target_rangetblentry;
+     List        *p_functionOids;
  } ParseState;

  extern ParseState *make_parsestate(ParseState *parentParseState);
Index: src/include/storage/sinval.h
===================================================================
RCS file: /projects/cvsroot/pgsql/src/include/storage/sinval.h,v
retrieving revision 1.48
diff -c -r1.48 sinval.h
*** src/include/storage/sinval.h    19 Jun 2008 21:32:56 -0000    1.48
--- src/include/storage/sinval.h    28 Aug 2008 09:20:28 -0000
***************
*** 54,59 ****
--- 54,60 ----
      int16        id;                /* cache ID --- must be first */
      ItemPointerData tuplePtr;    /* tuple identifier in cached relation */
      Oid            dbId;            /* database ID, or 0 if a shared relation */
+     Oid            tupleId;        /* OID of the invalidated tuple */
      uint32        hashValue;        /* hash value of key for this catcache */
  } SharedInvalCatcacheMsg;

Index: src/include/utils/catcache.h
===================================================================
RCS file: /projects/cvsroot/pgsql/src/include/utils/catcache.h,v
retrieving revision 1.67
diff -c -r1.67 catcache.h
*** src/include/utils/catcache.h    19 Jun 2008 00:46:06 -0000    1.67
--- src/include/utils/catcache.h    28 Aug 2008 09:20:28 -0000
***************
*** 184,190 ****
                           ItemPointer pointer);
  extern void PrepareToInvalidateCacheTuple(Relation relation,
                                HeapTuple tuple,
!                            void (*function) (int, uint32, ItemPointer, Oid));

  extern void PrintCatCacheLeakWarning(HeapTuple tuple);
  extern void PrintCatCacheListLeakWarning(CatCList *list);
--- 184,190 ----
                           ItemPointer pointer);
  extern void PrepareToInvalidateCacheTuple(Relation relation,
                                HeapTuple tuple,
!                            void (*function) (int, uint32, ItemPointer, Oid, Oid));

  extern void PrintCatCacheLeakWarning(HeapTuple tuple);
  extern void PrintCatCacheListLeakWarning(CatCList *list);
Index: src/test/regress/expected/plancache.out
===================================================================
RCS file: /projects/cvsroot/pgsql/src/test/regress/expected/plancache.out,v
retrieving revision 1.7
diff -c -r1.7 plancache.out
*** src/test/regress/expected/plancache.out    8 Jun 2008 22:41:04 -0000    1.7
--- src/test/regress/expected/plancache.out    28 Aug 2008 09:20:29 -0000
***************
*** 53,61 ****
  -- since clients probably aren't expecting that to change on the fly
  ALTER TABLE pcachetest ADD COLUMN q3 bigint;
  EXECUTE prepstmt;
! ERROR:  cached plan must not change result type
  EXECUTE prepstmt2(123);
! ERROR:  cached plan must not change result type
  -- but we're nice guys and will let you undo your mistake
  ALTER TABLE pcachetest DROP COLUMN q3;
  EXECUTE prepstmt;
--- 53,74 ----
  -- since clients probably aren't expecting that to change on the fly
  ALTER TABLE pcachetest ADD COLUMN q3 bigint;
  EXECUTE prepstmt;
!         q1        |        q2         | q3
! ------------------+-------------------+----
!  4567890123456789 | -4567890123456789 |
!  4567890123456789 |               123 |
!               123 |               456 |
!               123 |  4567890123456789 |
!  4567890123456789 |  4567890123456789 |
! (5 rows)
!
  EXECUTE prepstmt2(123);
!  q1  |        q2        | q3
! -----+------------------+----
!  123 |              456 |
!  123 | 4567890123456789 |
! (2 rows)
!
  -- but we're nice guys and will let you undo your mistake
  ALTER TABLE pcachetest DROP COLUMN q3;
  EXECUTE prepstmt;