Thread: Patch: plan invalidation vs stored procedures

This is a followup for thread "plan invalidation vs stored procedures".

The background is that it is impossible to change function return type without
dropping and recreating. Unfortunately dropping a function ruins all of the
prepared statements that reference that function (including other functions).
To make matters worse the ruined plans are never invalidated and keep returning
"cache lookup failed" error until replanned (requires admin intervention). Also
the DBA that dropped the function probably has no clue that something is wrong
- not before looking at the server logs at least. This is NOT good, especially
if the database is supporting paid services.

I have prepared proof of concept patch to support plan invalidation on function
DROP (will add ALTER, REPLACE, etc. later). Currently the invalidation is
handled by just dropping all the plans when invalidation message is received.
The desired behaviour would be of course to drop only the affected plans. This
needs function oid list to be present in PlannedStmt -- will look into this later.
Probably a job for the planner.

Changing statement result type is also currently prohibited in
StorePreparedStatement. There maybe good reasons for this, but for the
invalidation to be really useful, it should be enabled.  Right now the attempt
to change type renders the plan unusable -- "ERROR:  cached plan must not
change result type". Besides that, the patch could already be useful in some
environments - if you are willing to trade the errors for some extra planning
CPU.

There are probably a lot of details that I have overlooked. I'd be really
thankful for some constructive comments and criticism. Especially, what needs
to be done to have this in the core.  Feedback appreciated.

regards,
Martin
*** ./src/backend/commands/functioncmds.c.orig    2008-08-06 17:01:28.000000000 +0300
--- ./src/backend/commands/functioncmds.c    2008-08-06 19:10:51.000000000 +0300
***************
*** 59,64 ****
--- 59,65 ----
  #include "utils/rel.h"
  #include "utils/syscache.h"
  #include "utils/tqual.h"
+ #include "utils/inval.h"


  static void AlterFunctionOwner_internal(Relation rel, HeapTuple tup,
***************
*** 906,911 ****
--- 907,915 ----
      object.objectSubId = 0;

      performDeletion(&object, stmt->behavior);
+
+     /* Notify that cached plans should be replanned */
+     CacheInvalidateProcedure(funcOid);
  }

  /*
*** ./src/backend/utils/cache/inval.c.orig    2008-08-06 16:12:17.000000000 +0300
--- ./src/backend/utils/cache/inval.c    2008-08-06 18:01:24.000000000 +0300
***************
*** 115,121 ****
  typedef struct InvalidationListHeader
  {
      InvalidationChunk *cclist;    /* list of chunks holding catcache msgs */
!     InvalidationChunk *rclist;    /* list of chunks holding relcache/smgr msgs */
  } InvalidationListHeader;

  /*----------------
--- 115,121 ----
  typedef struct InvalidationListHeader
  {
      InvalidationChunk *cclist;    /* list of chunks holding catcache msgs */
!     InvalidationChunk *rclist;    /* list of chunks holding relcache/smgr/proc msgs */
  } InvalidationListHeader;

  /*----------------
***************
*** 177,182 ****
--- 177,183 ----
  #define TWOPHASE_INFO_FILE_AFTER    2    /* relcache file inval */

  static void PersistInvalidationMessage(SharedInvalidationMessage *msg);
+ static void CacheRegisterCallback(int cacheid, CacheCallbackFunction func, Datum arg);


  /* ----------------------------------------------------------------
***************
*** 363,368 ****
--- 364,393 ----
  }

  /*
+  * Add a proc inval entry
+  */
+ static void
+ AddProcInvalidationMessage(InvalidationListHeader *hdr,
+                            Oid dbId, Oid procId)
+ {
+     SharedInvalidationMessage msg;
+
+     /* Don't add a duplicate item */
+     /* We assume dbId need not be checked because it will never change */
+
+     /* XXX: for now, only keep one proc invalidation message per database */
+     ProcessMessageList(hdr->rclist,
+                        if (msg->pm.id == SHAREDINVALPROC_ID)
+                        return);
+
+     /* OK, add the item */
+     msg.pm.id = SHAREDINVALPROC_ID;
+     msg.pm.dbId = dbId;
+     msg.pm.procId = procId;
+     AddInvalidationMessage(&hdr->rclist, &msg);
+ }
+
+ /*
   * Append one list of invalidation messages to another, resetting
   * the source list to empty.
   */
***************
*** 465,470 ****
--- 490,512 ----
  }

  /*
+  * RegisterProcInvalidation
+  *
+  * As above, but register a procedure invalidation event.
+  */
+ static void
+ RegisterProcInvalidation(Oid dbId, Oid procId)
+ {
+     AddProcInvalidationMessage(&transInvalInfo->CurrentCmdInvalidMsgs,
+                                    dbId, procId);
+
+     /*
+      * As above, just in case there is not an associated catalog change.
+      */
+     (void) GetCurrentCommandId(true);
+ }
+
+ /*
   * LocalExecuteInvalidationMessage
   *
   * Process a single invalidation message (which could be of any type).
***************
*** 516,521 ****
--- 558,577 ----
           */
          smgrclosenode(msg->sm.rnode);
      }
+     else if (msg->id == SHAREDINVALPROC_ID)
+     {
+         if (msg->rc.dbId == MyDatabaseId)
+         {
+             /* for now we just need to handle callback functions */
+             for (i = 0; i < cache_callback_count; i++)
+             {
+                 struct CACHECALLBACK *ccitem = cache_callback_list + i;
+
+                 if (ccitem->id == SHAREDINVALPROC_ID)
+                     (*ccitem->function) (ccitem->arg, msg->rc.relId);
+             }
+         }
+     }
      else
          elog(FATAL, "unrecognized SI message id: %d", msg->id);
  }
***************
*** 1175,1191 ****
  }

  /*
!  * CacheRegisterSyscacheCallback
   *        Register the specified function to be called for all future
   *        invalidation events in the specified cache.
   *
-  * NOTE: currently, the OID argument to the callback routine is not
-  * provided for syscache callbacks; the routine doesn't really get any
-  * useful info as to exactly what changed.    It should treat every call
-  * as a "cache flush" request.
   */
! void
! CacheRegisterSyscacheCallback(int cacheid,
                                CacheCallbackFunction func,
                                Datum arg)
  {
--- 1231,1254 ----
  }

  /*
!  * CacheInvalidateProcedure
!  *        Register invalidation of the specified stored procedure.
!  *
!  */
! void
! CacheInvalidateProcedure(Oid procId)
! {
!     RegisterProcInvalidation(MyDatabaseId, procId);
! }
!
! /*
!  * CacheRegisterCallback
   *        Register the specified function to be called for all future
   *        invalidation events in the specified cache.
   *
   */
! static void
! CacheRegisterCallback(int cacheid,
                                CacheCallbackFunction func,
                                Datum arg)
  {
***************
*** 1199,1204 ****
--- 1262,1286 ----
      ++cache_callback_count;
  }

+
+ /*
+  * CacheRegisterSyscacheCallback
+  *        Register the specified function to be called for all future
+  *        invalidation events in the specified cache.
+  *
+  * NOTE: currently, the OID argument to the callback routine is not
+  * provided for syscache callbacks; the routine doesn't really get any
+  * useful info as to exactly what changed.    It should treat every call
+  * as a "cache flush" request.
+  */
+ void
+ CacheRegisterSyscacheCallback(int cacheid,
+                               CacheCallbackFunction func,
+                               Datum arg)
+ {
+     CacheRegisterCallback(cacheid, func, arg);
+ }
+
  /*
   * CacheRegisterRelcacheCallback
   *        Register the specified function to be called for all future
***************
*** 1212,1223 ****
  CacheRegisterRelcacheCallback(CacheCallbackFunction func,
                                Datum arg)
  {
!     if (cache_callback_count >= MAX_CACHE_CALLBACKS)
!         elog(FATAL, "out of cache_callback_list slots");
!
!     cache_callback_list[cache_callback_count].id = SHAREDINVALRELCACHE_ID;
!     cache_callback_list[cache_callback_count].function = func;
!     cache_callback_list[cache_callback_count].arg = arg;

!     ++cache_callback_count;
  }
--- 1294,1311 ----
  CacheRegisterRelcacheCallback(CacheCallbackFunction func,
                                Datum arg)
  {
!     CacheRegisterCallback(SHAREDINVALRELCACHE_ID, func, arg);
! }

! /*
!  * CacheRegisterProcCallback
!  *        Register the specified function to be called for all future
!  *        proccache invalidation events.  The OID of the procedure being
!  *        invalidated will be passed to the function.
!  */
! void
! CacheRegisterProcCallback(CacheCallbackFunction func, Datum arg)
! {
!     CacheRegisterCallback(SHAREDINVALPROC_ID, func, arg);
  }
+
*** ./src/backend/utils/cache/plancache.c.orig    2008-08-06 16:43:11.000000000 +0300
--- ./src/backend/utils/cache/plancache.c    2008-08-06 18:34:30.000000000 +0300
***************
*** 82,87 ****
--- 82,88 ----
  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 PlanCacheProcCallback(Datum arg, Oid procId);
  static void InvalRelid(Oid relid, LOCKMODE lockmode,
             InvalRelidContext *context);

***************
*** 95,100 ****
--- 96,102 ----
  InitPlanCache(void)
  {
      CacheRegisterRelcacheCallback(PlanCacheCallback, (Datum) 0);
+     CacheRegisterProcCallback(PlanCacheProcCallback, (Datum) 0);
  }

  /*
***************
*** 925,930 ****
--- 927,951 ----
  }

  /*
+  * PlanCacheProcCallback
+  *        Procedure inval callback function
+  *
+  * XXX: For now we just throw away everything.
+  */
+ static void
+ PlanCacheProcCallback(Datum arg, Oid procId)
+ {
+     ListCell   *lc;
+
+     foreach(lc, cached_plans_list)
+     {
+         CachedPlanSource *ps = (CachedPlanSource *)lfirst(lc);
+         if (ps && ps->plan)
+             ps->plan->dead = true;
+     }
+ }
+
+ /*
   * ResetPlanCache: drop all cached plans.
   */
  void
*** ./src/include/storage/sinval.h.orig    2008-08-06 16:19:41.000000000 +0300
--- ./src/include/storage/sinval.h    2008-08-06 16:32:50.000000000 +0300
***************
*** 74,85 ****
--- 74,95 ----
      RelFileNode rnode;            /* physical file ID */
  } SharedInvalSmgrMsg;

+ #define SHAREDINVALPROC_ID        (-3)
+
+ typedef struct
+ {
+     int16        id;                /* type field --- must be first */
+     Oid            dbId;            /* database ID */
+     Oid            procId;            /* procedure ID */
+ } SharedInvalProcMsg;
+
  typedef union
  {
      int16        id;                /* type field --- must be first */
      SharedInvalCatcacheMsg cc;
      SharedInvalRelcacheMsg rc;
      SharedInvalSmgrMsg sm;
+     SharedInvalProcMsg pm;
  } SharedInvalidationMessage;


*** ./src/include/utils/inval.h.orig    2008-08-06 16:27:58.000000000 +0300
--- ./src/include/utils/inval.h    2008-08-06 16:55:40.000000000 +0300
***************
*** 49,54 ****
--- 49,56 ----

  extern void CacheInvalidateRelcacheByRelid(Oid relid);

+ extern void CacheInvalidateProcedure(Oid procId);
+
  extern void CacheRegisterSyscacheCallback(int cacheid,
                                CacheCallbackFunction func,
                                Datum arg);
***************
*** 56,61 ****
--- 58,66 ----
  extern void CacheRegisterRelcacheCallback(CacheCallbackFunction func,
                                Datum arg);

+ extern void CacheRegisterProcCallback(CacheCallbackFunction func,
+                               Datum arg);
+
  extern void inval_twophase_postcommit(TransactionId xid, uint16 info,
                            void *recdata, uint32 len);

Martin Pihlak <martin.pihlak@gmail.com> writes:
> Changing statement result type is also currently prohibited in
> StorePreparedStatement. There maybe good reasons for this,

How about "the SQL spec says so"?

Admittedly, it's a bit of a jump from views to prepared statements,
but the spec is perfectly clear that altering a table doesn't alter
any views dependent on it: SQL99 11.11 <add column definition> saith
           NOTE 189 - The addition of a column to a table has no effect on           any existing <query expression>
includedin a view descriptor,           <triggered action> included in a trigger descriptor, or <search
condition>included in a constraint descriptor because any           implicit column references in these descriptor
elementsare           syntactically substituted by explicit column references under           the Syntax Rules of
Subclause7.11, "<query specification>".           Furthermore, by implication (from the lack of any General Rules
   to the contrary), the meaning of a column reference is never           retroactively changed by the addition of a
columnsubsequent           to the invocation of the <SQL schema statement> containing that           column reference.
 

and there was a comparable restriction in SQL92.  You'd need to make a
pretty strong argument why prepared statements should behave differently
from views to convince me that changing this is a good idea.
        regards, tom lane

> > Changing statement result type is also currently prohibited in
> > StorePreparedStatement. There maybe good reasons for this,
>
> How about "the SQL spec says so"?

Prepare time is often also the time when you bind the result, or more
generally set up the code to handle the result.

Generally I argue, that a mode of operation must exist where a change in
return type throws an error, so the client can readjust to the change.

We are only allowed to silently replan when it is clear that
the caller is agnostic to the change.
e.g. because the caller only accesses explicit columns of the return type/result set,
or does not supply a new parameter with a default, (or because he set some
parameter that tells us he can cope).

Certainly a new prepare must be able to cope with the change though,
which currently does not seem to be the case when an SP calls another
one that was dropped (and recreated)?

Andreas

Tom Lane wrote:
> Martin Pihlak <martin.pihlak@gmail.com> writes:
>> Changing statement result type is also currently prohibited in
>> StorePreparedStatement. There maybe good reasons for this,
>
> How about "the SQL spec says so"?
>
> Admittedly, it's a bit of a jump from views to prepared statements,
> but the spec is perfectly clear that altering a table doesn't alter
> any views dependent on it: SQL99 11.11 <add column definition> saith

As you said it is a bit of a jump ... For one thing view definitions are
persistent whereas statements are bound to be replanned sooner or later -
reconnects etc. Disallowing replanning after invalidation just postpones
it and meanwhile the cached plans are left unusable ("cached plan must not
change result"). IMHO the problem should be left for the application to handle.
Because this is where it will end up anyway.

Attached is a patch that implements plan invalidation on function DROP,
REPLACE and ALTER.  Function oids used by the query are collected in analyze phase
and stored in PlannedStmt. Only plans that reference the altered function are
invalidated. The patch also enables replanning on result set change.

regards,
Martin

Index: src/backend/commands/functioncmds.c
===================================================================
RCS file: /projects/cvsroot/pgsql/src/backend/commands/functioncmds.c,v
retrieving revision 1.98
diff -c -r1.98 functioncmds.c
*** src/backend/commands/functioncmds.c    18 Jul 2008 03:32:52 -0000    1.98
--- src/backend/commands/functioncmds.c    15 Aug 2008 11:12:51 -0000
***************
*** 59,64 ****
--- 59,65 ----
  #include "utils/rel.h"
  #include "utils/syscache.h"
  #include "utils/tqual.h"
+ #include "utils/inval.h"


  static void AlterFunctionOwner_internal(Relation rel, HeapTuple tup,
***************
*** 680,685 ****
--- 681,687 ----
      HeapTuple    languageTuple;
      Form_pg_language languageStruct;
      List       *as_clause;
+     Oid            funcOid;

      /* Convert list of names to a name and namespace */
      namespaceId = QualifiedNameGetCreationNamespace(stmt->funcname,
***************
*** 817,823 ****
       * And now that we have all the parameters, and know we're permitted to do
       * so, go ahead and create the function.
       */
!     ProcedureCreate(funcname,
                      namespaceId,
                      stmt->replace,
                      returnsSet,
--- 819,825 ----
       * And now that we have all the parameters, and know we're permitted to do
       * so, go ahead and create the function.
       */
!     funcOid = ProcedureCreate(funcname,
                      namespaceId,
                      stmt->replace,
                      returnsSet,
***************
*** 837,842 ****
--- 839,848 ----
                      PointerGetDatum(proconfig),
                      procost,
                      prorows);
+
+     /* Send invalidation on REPLACE */
+     if (stmt->replace)
+         CacheInvalidateProcedure(funcOid);
  }


***************
*** 906,911 ****
--- 912,920 ----
      object.objectSubId = 0;

      performDeletion(&object, stmt->behavior);
+
+     /* Notify that cached plans should be replanned */
+     CacheInvalidateProcedure(funcOid);
  }

  /*
***************
*** 1029,1034 ****
--- 1038,1046 ----

      heap_close(rel, NoLock);
      heap_freetuple(tup);
+
+     /* Need plan invalidation after this */
+     CacheInvalidateProcedure(procOid);
  }

  /*
***************
*** 1294,1299 ****
--- 1306,1314 ----

      heap_close(rel, NoLock);
      heap_freetuple(tup);
+
+     /* Invalidate plans after this */
+     CacheInvalidateProcedure(funcOid);
  }

  /*
Index: src/backend/commands/prepare.c
===================================================================
RCS file: /projects/cvsroot/pgsql/src/backend/commands/prepare.c,v
retrieving revision 1.89
diff -c -r1.89 prepare.c
*** src/backend/commands/prepare.c    21 Jul 2008 15:26:55 -0000    1.89
--- src/backend/commands/prepare.c    15 Aug 2008 11:12:52 -0000
***************
*** 188,196 ****
      /* 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)
--- 188,193 ----
***************
*** 462,468 ****
                                    cursor_options,
                                    stmt_list,
                                    true,
!                                   true);

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

      /* Now we can add entry to hash table */
      entry = (PreparedStatement *) hash_search(prepared_queries,
***************
*** 523,533 ****
  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
--- 520,528 ----
  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
***************
*** 649,657 ****
      /* 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);
--- 644,649 ----
Index: src/backend/nodes/copyfuncs.c
===================================================================
RCS file: /projects/cvsroot/pgsql/src/backend/nodes/copyfuncs.c,v
retrieving revision 1.399
diff -c -r1.399 copyfuncs.c
*** src/backend/nodes/copyfuncs.c    7 Aug 2008 19:35:02 -0000    1.399
--- src/backend/nodes/copyfuncs.c    15 Aug 2008 11:12:53 -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;
***************
*** 1866,1871 ****
--- 1867,1873 ----
      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.240
diff -c -r1.240 planner.c
*** src/backend/optimizer/plan/planner.c    7 Aug 2008 01:11:50 -0000    1.240
--- src/backend/optimizer/plan/planner.c    15 Aug 2008 11:12:54 -0000
***************
*** 215,220 ****
--- 215,221 ----
      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.376
diff -c -r1.376 analyze.c
*** src/backend/parser/analyze.c    7 Aug 2008 01:11:51 -0000    1.376
--- src/backend/parser/analyze.c    15 Aug 2008 11:12:55 -0000
***************
*** 227,232 ****
--- 227,235 ----
      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.204
diff -c -r1.204 parse_func.c
*** src/backend/parser/parse_func.c    30 Jul 2008 17:05:04 -0000    1.204
--- src/backend/parser/parse_func.c    15 Aug 2008 11:12:55 -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.555
diff -c -r1.555 postgres.c
*** src/backend/tcop/postgres.c    1 Aug 2008 13:16:09 -0000    1.555
--- src/backend/tcop/postgres.c    15 Aug 2008 11:12:57 -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/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    15 Aug 2008 11:12:57 -0000
***************
*** 115,121 ****
  typedef struct InvalidationListHeader
  {
      InvalidationChunk *cclist;    /* list of chunks holding catcache msgs */
!     InvalidationChunk *rclist;    /* list of chunks holding relcache/smgr msgs */
  } InvalidationListHeader;

  /*----------------
--- 115,121 ----
  typedef struct InvalidationListHeader
  {
      InvalidationChunk *cclist;    /* list of chunks holding catcache msgs */
!     InvalidationChunk *rclist;    /* list of chunks holding relcache/smgr/proc msgs */
  } InvalidationListHeader;

  /*----------------
***************
*** 177,182 ****
--- 177,183 ----
  #define TWOPHASE_INFO_FILE_AFTER    2    /* relcache file inval */

  static void PersistInvalidationMessage(SharedInvalidationMessage *msg);
+ static void CacheRegisterCallback(int cacheid, CacheCallbackFunction func, Datum arg);


  /* ----------------------------------------------------------------
***************
*** 363,368 ****
--- 364,393 ----
  }

  /*
+  * Add a proc inval entry
+  */
+ static void
+ AddProcInvalidationMessage(InvalidationListHeader *hdr,
+                            Oid dbId, Oid procId)
+ {
+     SharedInvalidationMessage msg;
+
+     /* Don't add a duplicate item */
+     /* We assume dbId need not be checked because it will never change */
+
+     ProcessMessageList(hdr->rclist,
+                        if (msg->pm.id == SHAREDINVALPROC_ID &&
+                            msg->pm.procId == procId)
+                        return);
+
+     /* OK, add the item */
+     msg.pm.id = SHAREDINVALPROC_ID;
+     msg.pm.dbId = dbId;
+     msg.pm.procId = procId;
+     AddInvalidationMessage(&hdr->rclist, &msg);
+ }
+
+ /*
   * Append one list of invalidation messages to another, resetting
   * the source list to empty.
   */
***************
*** 465,470 ****
--- 490,512 ----
  }

  /*
+  * RegisterProcInvalidation
+  *
+  * As above, but register a procedure invalidation event.
+  */
+ static void
+ RegisterProcInvalidation(Oid dbId, Oid procId)
+ {
+     AddProcInvalidationMessage(&transInvalInfo->CurrentCmdInvalidMsgs,
+                                    dbId, procId);
+
+     /*
+      * As above, just in case there is not an associated catalog change.
+      */
+     (void) GetCurrentCommandId(true);
+ }
+
+ /*
   * LocalExecuteInvalidationMessage
   *
   * Process a single invalidation message (which could be of any type).
***************
*** 516,521 ****
--- 558,577 ----
           */
          smgrclosenode(msg->sm.rnode);
      }
+     else if (msg->id == SHAREDINVALPROC_ID)
+     {
+         if (msg->rc.dbId == MyDatabaseId)
+         {
+             /* for now we just need to handle callback functions */
+             for (i = 0; i < cache_callback_count; i++)
+             {
+                 struct CACHECALLBACK *ccitem = cache_callback_list + i;
+
+                 if (ccitem->id == SHAREDINVALPROC_ID)
+                     (*ccitem->function) (ccitem->arg, msg->rc.relId);
+             }
+         }
+     }
      else
          elog(FATAL, "unrecognized SI message id: %d", msg->id);
  }
***************
*** 1175,1191 ****
  }

  /*
!  * CacheRegisterSyscacheCallback
   *        Register the specified function to be called for all future
   *        invalidation events in the specified cache.
   *
-  * NOTE: currently, the OID argument to the callback routine is not
-  * provided for syscache callbacks; the routine doesn't really get any
-  * useful info as to exactly what changed.    It should treat every call
-  * as a "cache flush" request.
   */
! void
! CacheRegisterSyscacheCallback(int cacheid,
                                CacheCallbackFunction func,
                                Datum arg)
  {
--- 1231,1254 ----
  }

  /*
!  * CacheInvalidateProcedure
!  *        Register invalidation of the specified stored procedure.
!  *
!  */
! void
! CacheInvalidateProcedure(Oid procId)
! {
!     RegisterProcInvalidation(MyDatabaseId, procId);
! }
!
! /*
!  * CacheRegisterCallback
   *        Register the specified function to be called for all future
   *        invalidation events in the specified cache.
   *
   */
! static void
! CacheRegisterCallback(int cacheid,
                                CacheCallbackFunction func,
                                Datum arg)
  {
***************
*** 1199,1204 ****
--- 1262,1286 ----
      ++cache_callback_count;
  }

+
+ /*
+  * CacheRegisterSyscacheCallback
+  *        Register the specified function to be called for all future
+  *        invalidation events in the specified cache.
+  *
+  * NOTE: currently, the OID argument to the callback routine is not
+  * provided for syscache callbacks; the routine doesn't really get any
+  * useful info as to exactly what changed.    It should treat every call
+  * as a "cache flush" request.
+  */
+ void
+ CacheRegisterSyscacheCallback(int cacheid,
+                               CacheCallbackFunction func,
+                               Datum arg)
+ {
+     CacheRegisterCallback(cacheid, func, arg);
+ }
+
  /*
   * CacheRegisterRelcacheCallback
   *        Register the specified function to be called for all future
***************
*** 1212,1223 ****
  CacheRegisterRelcacheCallback(CacheCallbackFunction func,
                                Datum arg)
  {
!     if (cache_callback_count >= MAX_CACHE_CALLBACKS)
!         elog(FATAL, "out of cache_callback_list slots");
!
!     cache_callback_list[cache_callback_count].id = SHAREDINVALRELCACHE_ID;
!     cache_callback_list[cache_callback_count].function = func;
!     cache_callback_list[cache_callback_count].arg = arg;

!     ++cache_callback_count;
  }
--- 1294,1311 ----
  CacheRegisterRelcacheCallback(CacheCallbackFunction func,
                                Datum arg)
  {
!     CacheRegisterCallback(SHAREDINVALRELCACHE_ID, func, arg);
! }

! /*
!  * CacheRegisterProcCallback
!  *        Register the specified function to be called for all future
!  *        proccache invalidation events.  The OID of the procedure being
!  *        invalidated will be passed to the function.
!  */
! void
! CacheRegisterProcCallback(CacheCallbackFunction func, Datum arg)
! {
!     CacheRegisterCallback(SHAREDINVALPROC_ID, func, arg);
  }
+
Index: src/backend/utils/cache/plancache.c
===================================================================
RCS file: /projects/cvsroot/pgsql/src/backend/utils/cache/plancache.c,v
retrieving revision 1.19
diff -c -r1.19 plancache.c
*** src/backend/utils/cache/plancache.c    18 Jul 2008 20:26:06 -0000    1.19
--- src/backend/utils/cache/plancache.c    15 Aug 2008 11:12:58 -0000
***************
*** 82,89 ****
--- 82,91 ----
  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 PlanCacheProcCallback(Datum arg, Oid procId);
  static void InvalRelid(Oid relid, LOCKMODE lockmode,
             InvalRelidContext *context);
+ static void ScanForInvalidations(Oid objId, bool is_function_oid);


  /*
***************
*** 95,100 ****
--- 97,103 ----
  InitPlanCache(void)
  {
      CacheRegisterRelcacheCallback(PlanCacheCallback, (Datum) 0);
+     CacheRegisterProcCallback(PlanCacheProcCallback, (Datum) 0);
  }

  /*
***************
*** 863,876 ****
  }

  /*
!  * 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;
--- 866,879 ----
  }

  /*
!  * ScanForInvalidations
   *        Relcache inval callback function
   *
!  * Invalidate all plans mentioning the given object, or all plans mentioning
!  * any object at all if objId == InvalidOid.
   */
  static void
! ScanForInvalidations(Oid objId, bool is_function_oid)
  {
      ListCell   *lc1;
      ListCell   *lc2;
***************
*** 888,899 ****
              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;
--- 891,911 ----
              foreach(lc2, plan->stmt_list)
              {
                  PlannedStmt *plannedstmt = (PlannedStmt *) lfirst(lc2);
+                 bool invalidate = false;

                  Assert(!IsA(plannedstmt, Query));
                  if (!IsA(plannedstmt, PlannedStmt))
                      continue;    /* Ignore utility statements */
!
!                 if (objId == InvalidOid)
!                     /* invalidate everything */
!                     invalidate = true;
!                 else if (is_function_oid)
!                     invalidate = list_member_oid(plannedstmt->functionOids, objId);
!                 else
!                     invalidate = list_member_oid(plannedstmt->relationOids, objId);
!
!                 if (invalidate)
                  {
                      /* Invalidate the plan! */
                      plan->dead = true;
***************
*** 910,916 ****
               */
              InvalRelidContext context;

!             context.inval_relid = relid;
              context.plan = plan;

              foreach(lc2, plan->stmt_list)
--- 922,928 ----
               */
              InvalRelidContext context;

!             context.inval_relid = objId;
              context.plan = plan;

              foreach(lc2, plan->stmt_list)
***************
*** 918,930 ****
                  Query       *query = (Query *) lfirst(lc2);

                  Assert(IsA(query, Query));
!                 ScanQueryForRelids(query, InvalRelid, (void *) &context);
              }
          }
      }
  }

  /*
   * ResetPlanCache: drop all cached plans.
   */
  void
--- 930,977 ----
                  Query       *query = (Query *) lfirst(lc2);

                  Assert(IsA(query, Query));
!
!                 if (is_function_oid)
!                 {
!                     /* Functions are easy - we already have the list of OIDs in Query */
!                     if (objId == InvalidOid ||
!                         list_member_oid(query->functionOids, objId))
!                     {
!                         plan->dead = true;
!                         break;
!                     }
!                 }
!                 else
!                 {
!                     /* No precomputed relation OID list in Query - need to scan */
!                     ScanQueryForRelids(query, InvalRelid, (void *) &context);
!                 }
              }
          }
      }
  }

  /*
+  * PlanCacheCallback
+  *        Relcache inval callback function
+  */
+ static void
+ PlanCacheCallback(Datum arg, Oid relid)
+ {
+     ScanForInvalidations(relid, false);
+ }
+
+ /*
+  * PlanCacheProcCallback
+  *        Stored procedure invalidation callback function
+  */
+ static void
+ PlanCacheProcCallback(Datum arg, Oid procId)
+ {
+     ScanForInvalidations(procId, true);
+ }
+
+ /*
   * ResetPlanCache: drop all cached plans.
   */
  void
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    15 Aug 2008 11:12:59 -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    15 Aug 2008 11:12:59 -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    15 Aug 2008 11:12:59 -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    15 Aug 2008 11:12:59 -0000
***************
*** 74,85 ****
--- 74,95 ----
      RelFileNode rnode;            /* physical file ID */
  } SharedInvalSmgrMsg;

+ #define SHAREDINVALPROC_ID        (-3)
+
+ typedef struct
+ {
+     int16        id;                /* type field --- must be first */
+     Oid            dbId;            /* database ID */
+     Oid            procId;            /* procedure ID */
+ } SharedInvalProcMsg;
+
  typedef union
  {
      int16        id;                /* type field --- must be first */
      SharedInvalCatcacheMsg cc;
      SharedInvalRelcacheMsg rc;
      SharedInvalSmgrMsg sm;
+     SharedInvalProcMsg pm;
  } SharedInvalidationMessage;


Index: src/include/utils/inval.h
===================================================================
RCS file: /projects/cvsroot/pgsql/src/include/utils/inval.h,v
retrieving revision 1.43
diff -c -r1.43 inval.h
*** src/include/utils/inval.h    19 Jun 2008 00:46:06 -0000    1.43
--- src/include/utils/inval.h    15 Aug 2008 11:12:59 -0000
***************
*** 49,54 ****
--- 49,56 ----

  extern void CacheInvalidateRelcacheByRelid(Oid relid);

+ extern void CacheInvalidateProcedure(Oid procId);
+
  extern void CacheRegisterSyscacheCallback(int cacheid,
                                CacheCallbackFunction func,
                                Datum arg);
***************
*** 56,61 ****
--- 58,66 ----
  extern void CacheRegisterRelcacheCallback(CacheCallbackFunction func,
                                Datum arg);

+ extern void CacheRegisterProcCallback(CacheCallbackFunction func,
+                               Datum arg);
+
  extern void inval_twophase_postcommit(TransactionId xid, uint16 info,
                            void *recdata, uint32 len);

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    15 Aug 2008 11:13:01 -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;

<div dir="ltr">Hi<br /><br />We need plan invalidation fix in 8.3 also at least it would make migrating from 8.2 to 8.3
muchmore attractive. <br />Currenlty we are having problems related to plan invalidation couple of times per week
(mainlywe have to let developers change their code before we release it into live databases but it feels like sitting
onticking bomb after previous downtime). <br /> Is it possible to get it into some official 8.3.x release or should we
doit in house? <br /><div class="gmail_quote">Who should add it into september commitfest?<br /><br />Asko<br /><br
/><br/>On Fri, Aug 15, 2008 at 2:13 PM, Martin Pihlak <span dir="ltr"><<a
href="mailto:martin.pihlak@gmail.com">martin.pihlak@gmail.com</a>></span>wrote:<br /><blockquote class="gmail_quote"
style="border-left:1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;"><div class="Ih2E3d">Tom
Lanewrote:<br /> > Martin Pihlak <<a href="mailto:martin.pihlak@gmail.com">martin.pihlak@gmail.com</a>>
writes:<br/> >> Changing statement result type is also currently prohibited in<br /> >>
StorePreparedStatement.There maybe good reasons for this,<br /> ><br /> > How about "the SQL spec says so"?<br />
><br/> > Admittedly, it's a bit of a jump from views to prepared statements,<br /> > but the spec is perfectly
clearthat altering a table doesn't alter<br /> > any views dependent on it: SQL99 11.11 <add column
definition>saith<br /><br /></div>As you said it is a bit of a jump ... For one thing view definitions are<br />
persistentwhereas statements are bound to be replanned sooner or later -<br /> reconnects etc. Disallowing replanning
afterinvalidation just postpones<br /> it and meanwhile the cached plans are left unusable ("cached plan must not<br />
changeresult"). IMHO the problem should be left for the application to handle.<br /> Because this is where it will end
upanyway.<br /><br /> Attached is a patch that implements plan invalidation on function DROP,<br /> REPLACE and ALTER.
 Functionoids used by the query are collected in analyze phase<br /> and stored in PlannedStmt. Only plans that
referencethe altered function are<br /> invalidated. The patch also enables replanning on result set change.<br /><br
/>regards,<br /><font color="#888888">Martin<br /><br /></font><br /><br /> --<br /> Sent via pgsql-hackers mailing
list(<a href="mailto:pgsql-hackers@postgresql.org">pgsql-hackers@postgresql.org</a>)<br /> To make changes to your
subscription:<br/><a href="http://www.postgresql.org/mailpref/pgsql-hackers"
target="_blank">http://www.postgresql.org/mailpref/pgsql-hackers</a><br/><br /></blockquote></div><br /></div> 

"Asko Oja" <ascoja@gmail.com> writes:
> Is it possible to get it into some official 8.3.x release

This is not the kind of patch we put into stable branches.
        regards, tom lane

On Sat, Aug 16, 2008 at 09:40:19PM -0400, Tom Lane wrote:
> "Asko Oja" <ascoja@gmail.com> writes:
> > Is it possible to get it into some official 8.3.x release
> 
> This is not the kind of patch we put into stable branches.

Does this really count as a user-visible change, except in the sense
that they won't see things erroring out?  It doesn't add new syntax,
as far as I can tell.

Cheers,
David.
-- 
David Fetter <david@fetter.org> http://fetter.org/
Phone: +1 415 235 3778  AIM: dfetter666  Yahoo!: dfetter
Skype: davidfetter      XMPP: david.fetter@gmail.com

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

David Fetter wrote:
> On Sat, Aug 16, 2008 at 09:40:19PM -0400, Tom Lane wrote:
>   
>> "Asko Oja" <ascoja@gmail.com> writes:
>>     
>>> Is it possible to get it into some official 8.3.x release
>>>       
>> This is not the kind of patch we put into stable branches.
>>     
>
> Does this really count as a user-visible change, except in the sense
> that they won't see things erroring out?  It doesn't add new syntax,
> as far as I can tell.
>
>   

So what? That is not the only criterion for backpatching.

The bigger the change the more resistance there will be to backpatching 
it. Code stability is a major concern.

cheers

andrew

Hi,

Le lundi 18 août 2008, Andrew Dunstan a écrit :
> > On Sat, Aug 16, 2008 at 09:40:19PM -0400, Tom Lane wrote:
> >> This is not the kind of patch we put into stable branches.
>
> So what? That is not the only criterion for backpatching.

I fail to understand why this problem is not qualified as a bug.

Regards,
--
dim

2008/8/18 Dimitri Fontaine <dfontaine@hi-media.com>:
> Hi,
>
> Le lundi 18 août 2008, Andrew Dunstan a écrit :
>> > On Sat, Aug 16, 2008 at 09:40:19PM -0400, Tom Lane wrote:
>> >> This is not the kind of patch we put into stable branches.
>>
>> So what? That is not the only criterion for backpatching.
>
> I fail to understand why this problem is not qualified as a bug.
>

Does it change of result some queries? It is protection to server's hang?

> Regards,
> --
> dim
>

On Mon, 2008-08-18 at 11:05 +0200, Pavel Stehule wrote:
> 2008/8/18 Dimitri Fontaine <dfontaine@hi-media.com>:
> > Hi,
> >
> > Le lundi 18 août 2008, Andrew Dunstan a écrit :
> >> > On Sat, Aug 16, 2008 at 09:40:19PM -0400, Tom Lane wrote:
> >> >> This is not the kind of patch we put into stable branches.
> >>
> >> So what? That is not the only criterion for backpatching.
> >
> > I fail to understand why this problem is not qualified as a bug.
> >
> 
> Does it change of result some queries? 

Not in the long run, but not invalidating the functions (current
behaviour) postpones seeing the results of function change until DBA
manually restarts the error-producing client.

> It is protection to server's hang?

Can't understand this question :(

If you mean, does the change protect against hanging the server, then
no, currently the server does not actually hang, it just becomes
unusable until reconnect :(

---------
Hannu

2008/8/18 Hannu Krosing <hannu@2ndquadrant.com>:
> On Mon, 2008-08-18 at 11:05 +0200, Pavel Stehule wrote:
>> 2008/8/18 Dimitri Fontaine <dfontaine@hi-media.com>:
>> > Hi,
>> >
>> > Le lundi 18 août 2008, Andrew Dunstan a écrit :
>> >> > On Sat, Aug 16, 2008 at 09:40:19PM -0400, Tom Lane wrote:
>> >> >> This is not the kind of patch we put into stable branches.
>> >>
>> >> So what? That is not the only criterion for backpatching.
>> >
>> > I fail to understand why this problem is not qualified as a bug.
>> >
>>
>> Does it change of result some queries?
>
> Not in the long run, but not invalidating the functions (current
> behaviour) postpones seeing the results of function change until DBA
> manually restarts the error-producing client.
>
>> It is protection to server's hang?
>
> Can't understand this question :(
>
> If you mean, does the change protect against hanging the server, then
> no, currently the server does not actually hang, it just becomes
> unusable until reconnect :(

Hi

I am sorry, but it's really new feature and not bug fix

Pavel
>
> ---------
> Hannu
>
>

"Asko Oja" <ascoja@gmail.com> writes:
> For users of stored procedures it is protection from downtime. For Skype it
> has been around 20% of databse related downtime this year.

Perhaps Skype needs to rethink how they are modifying functions.

The reason that this case wasn't covered in 8.3 is that there didn't
seem to be a use-case that justified doing the extra work.  I still
haven't seen one.  Other than inline-able SQL functions there is no
reason to invalidate a stored plan based on the fact that some function
it called changed contents.
        regards, tom lane

<div dir="ltr">Hi<br /><br />> The reason that this case wasn't covered in 8.3 is that there didn't<br />> seem
tobe a use-case that justified doing the extra work.  I still<br /> > haven't seen one. <br />You just stopped
readingthe thread where it was discussed after your troll remark? <br /><br />> Other than inline-able SQL functions
thereis no reason to invalidate a stored plan <br />> based on the fact that some function it called changed
contents.<br/> Isn't it reason enough for this patch?<br />ERROR:  cache lookup failed for function is normal and good
behaviourand should not be recoverd from because it never happen if you PostgreSQL right :)<br /><br />Usecase 1:
Inlinedfunctions <br /> postgres=# create or replace function salary_without_income_tax(i_salary in numeric, salary out
numeric) returns numeric as $$ select $1 * 0.76 as salary $$ language sql;<br /> postgres=# prepare c2 as select
salary,salary_without_income_tax(salary) from salaries;<br /> postgres=#  execute c2;<br />  salary |
salary_without_income_tax<br /> --------+---------------------------<br />   10000 |                   7600.00<br />
postgres=#create or replace function salary_without_income_tax(i_salary in numeric, salary out numeric ) returns
numericas $$ select $1 * 0.74 as salary $$ language sql;<br /> postgres=#  execute c2; salary |
salary_without_income_tax<br /> --------+---------------------------<br />   10000 |                   7600.00<br /><br
/>Use case 2: While rewriting existing modules due to changes in business requirements then in addition to new code we
haveto refactor lots of old functions one natural thing to do would be to get rid of return types as they are even more
inconvenientto use than out parameters. Another reason is keep coding style consistent over modules so future
maintenacewill be less painful in the assholes.<br /> postgres=# create type public.ret_status as ( status integer,
status_texttext);<br />CREATE TYPE<br />postgres=# create or replace function x ( i_param text ) returns
public.ret_statusas $$ select 200::int, 'ok'::text; $$ language sql;<br /> CREATE FUNCTION<br />postgres=# create or
replacefunction x ( i_param text, status OUT int, status_text OUT text ) returns record as $$ select 200::int,
'ok'::text;$$ language sql;<br />ERROR:  cannot change return type of existing function<br /> HINT:  Use DROP FUNCTION
first<br/><br />Usecase 3.: Extra out parameters are needed in existing functions. I assure you if you have 5 years of
legacycode that is constantly changing it does happen (often).<br /> postgres=# create or replace function xl ( i_param
text,status OUT int, status_text OUT text, more_text OUT text ) returns record as $$ select 200::int, 'ok'::text,
'cat'::text;$$ language sql;<br /><div class="Ih2E3d"> ERROR:  cannot change return type of existing function<br
/></div>DETAIL: Row type defined by OUT parameters is different.<br />HINT:  Use DROP FUNCTION first.<br /><br
/>Usecase4: Things are even worse when you need to change the type that is used in functions. You have to drop and
recreatethe type and all the functions that are using it. Sometimes type is used in several functions and only some of
themneed changes.<br /> postgres=# create type public.ret_status_ext as ( status integer, status_text text, more_money
numeric);                                      <br />CREATE TYPE<br />postgres=# create or replace function x ( i_param
text) returns public.ret_status_ext as $$ select 200::int, 'ok'::text; $$ language sql;<br /> ERROR:  cannot change
returntype of existing function<br />HINT:  Use DROP FUNCTION first.<br /><br />And whenever we do drop and create as
hinted then we receive error flood that won't stop until something is manually done to get rid of it<br /> postgres=#
dropfunction x(text);<br />DROP FUNCTION<br />postgres=# create or replace function x ( i_param text ) returns
public.ret_status_extas $$ select 200::int, $1, 2.3 $$ language sql;<br />CREATE FUNCTION<br />postgres=# execute c;<br
/>ERROR:  cache lookup failed for function 24598<br /><br />I hope i have answered your question  "Why do you not use
CREATEOR REPLACE FUNCTION?" That leaves us to deal with functions in our usual bad, wrong and stupid ways.<br /> * We
createa function with new name and redirect all the calls to it. (stupid as it creates extra development, testing, code
reviewingand releasing work and leaves around old code). <br />* We pause pgBouncer and after release let it reconnect
allconnections (bad as it creates downtime).<br /> * We invalidate all procedures using update to pg_proc (simply wrong
wayto do it but still our best workaround short of restarting postgres).<br />postgres=# update pg_proc set proname =
proname;<br/>UPDATE 2152<br />postgres=#  execute c2;<br />  salary | salary_without_income_tax <br
/>--------+---------------------------<br/>  10000 |                   7400.00<br /> <br />> Perhaps Skype needs to
rethinkhow they are modifying functions.<br /> We have had to change the way we use functions to suit PostgreSQL for 5
yearsnow. That creates us quite a lot of extra work both on development side and DBA side plus the constantly hanging
dangerof downtime. Our DBA teams job is to reduce all possible causes for downtime and this patch is solution to one of
them.Sadly we just get trolled into the ground :)<br /><br />All in all it's not the job of PostgreSQL to tell the user
whatwe he can or can't replace in functions. It should be up to the user to decide what and how to replace so that all
surroundingapplications will say working. <br /><br />regards<br />Asko<br /><br /><div class="gmail_quote">PS: It all
confusespoor developers "Hi Asko, I work on web backend and am in the process of changing infodb.eurorates_exchange()
andinfodb._eurorates_lookup db functions found in dbs. Problem is the _eurorates_lookup function did not use IN OUT
paramsbut a type, we have been told we should update all functions to use IN OUT params. In doing so I will also need
todrop the function when it comes to deployment. This function is in the accounts partition and I know we are not
supposedto drop functions in partitions due to cache problems it creates. Could you tell me what I should do?
Thanks"<br/><br /><br /><br />On Tue, Aug 19, 2008 at 3:29 AM, Tom Lane <span dir="ltr"><<a
href="mailto:tgl@sss.pgh.pa.us">tgl@sss.pgh.pa.us</a>></span>wrote:<br /><blockquote class="gmail_quote"
style="border-left:1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;"><div
class="Ih2E3d">"AskoOja" <<a href="mailto:ascoja@gmail.com">ascoja@gmail.com</a>> writes:<br /></div><div
class="Ih2E3d">>For users of stored procedures it is protection from downtime. For Skype it<br /> > has been
around20% of databse related downtime this year.<br /><br /></div>Perhaps Skype needs to rethink how they are modifying
functions.<br/><br /> The reason that this case wasn't covered in 8.3 is that there didn't<br /> seem to be a use-case
thatjustified doing the extra work.  I still<br /> haven't seen one.  Other than inline-able SQL functions there is
no<br/> reason to invalidate a stored plan based on the fact that some function<br /> it called changed contents.<br
/><br/>                        regards, tom lane<br /></blockquote></div><br /></div> 

On Mon, 2008-08-18 at 22:41 +0200, Pavel Stehule wrote:
> 2008/8/18 Hannu Krosing <hannu@2ndquadrant.com>:
> > On Mon, 2008-08-18 at 11:05 +0200, Pavel Stehule wrote:
> >> 2008/8/18 Dimitri Fontaine <dfontaine@hi-media.com>:
> >> > Hi,
> >> >
> >> > Le lundi 18 août 2008, Andrew Dunstan a écrit :
> >> >> > On Sat, Aug 16, 2008 at 09:40:19PM -0400, Tom Lane wrote:
> >> >> >> This is not the kind of patch we put into stable branches.
> >> >>
> >> >> So what? That is not the only criterion for backpatching.
> >> >
> >> > I fail to understand why this problem is not qualified as a bug.
> >> >
> >>
> >> Does it change of result some queries?
> >
> > Not in the long run, but not invalidating the functions (current
> > behaviour) postpones seeing the results of function change until DBA
> > manually restarts the error-producing client.
> >
> >> It is protection to server's hang?
> >
> > Can't understand this question :(
> >
> > If you mean, does the change protect against hanging the server, then
> > no, currently the server does not actually hang, it just becomes
> > unusable until reconnect :(
> 
> Hi
> 
> I am sorry, but it's really new feature and not bug fix

Could you please explain why you think so ?

As I see it, the patch does not change visible behaviour, except
removing some sonditions where client becomes unusable after some other
backend does some legitimate changes.

Is the current behavior planned or even defined by spec ? 

I agree, that the bug (if it is a bug) could also be circumvented by the
calling function by detecting a failed cache lookup and doing
replan/requery itself, but this would require all PL implementations and
other functions with stored plans to do it independently.

-----
Hannu

On Mon, 2008-08-18 at 20:29 -0400, Tom Lane wrote:
> "Asko Oja" <ascoja@gmail.com> writes:
> > For users of stored procedures it is protection from downtime. For Skype it
> > has been around 20% of databse related downtime this year.
> 
> Perhaps Skype needs to rethink how they are modifying functions.

Why not suggest they just should stop using functions and move all
business logic into client or "3rd tier" ?

(Actually I would not recommend that  as functions are very good way to
abstract database access AND provide better security AND speed up
queries)

> The reason that this case wasn't covered in 8.3 is that there didn't
> seem to be a use-case that justified doing the extra work.  I still
> haven't seen one.  Other than inline-able SQL functions there is no
> reason to invalidate a stored plan based on the fact that some function
> it called changed contents.

Maybe there should be something in postgreSQL docs that warns users against 
using functions in any non-trivial circumstances, as functions are not 
expected to behave like the rest of postgreSQL features and there is 
not plan to fix that ?

----------------
Hannu

2008/8/19 Hannu Krosing <hannu@2ndquadrant.com>:
> On Mon, 2008-08-18 at 22:41 +0200, Pavel Stehule wrote:
>> 2008/8/18 Hannu Krosing <hannu@2ndquadrant.com>:
>> > On Mon, 2008-08-18 at 11:05 +0200, Pavel Stehule wrote:
>> >> 2008/8/18 Dimitri Fontaine <dfontaine@hi-media.com>:
>> >> > Hi,
>> >> >
>> >> > Le lundi 18 août 2008, Andrew Dunstan a écrit :
>> >> >> > On Sat, Aug 16, 2008 at 09:40:19PM -0400, Tom Lane wrote:
>> >> >> >> This is not the kind of patch we put into stable branches.
>> >> >>
>> >> >> So what? That is not the only criterion for backpatching.
>> >> >
>> >> > I fail to understand why this problem is not qualified as a bug.
>> >> >
>> >>
>> >> Does it change of result some queries?
>> >
>> > Not in the long run, but not invalidating the functions (current
>> > behaviour) postpones seeing the results of function change until DBA
>> > manually restarts the error-producing client.
>> >
>> >> It is protection to server's hang?
>> >
>> > Can't understand this question :(
>> >
>> > If you mean, does the change protect against hanging the server, then
>> > no, currently the server does not actually hang, it just becomes
>> > unusable until reconnect :(
>>
>> Hi
>>
>> I am sorry, but it's really new feature and not bug fix
>
> Could you please explain why you think so ?
>
> As I see it, the patch does not change visible behaviour, except
> removing some sonditions where client becomes unusable after some other
> backend does some legitimate changes.

Are you sure, so this behave hasn't any secondary effect? So this
change doesn't breaks any application?

Pavel

>
> Is the current behavior planned or even defined by spec ?
>
> I agree, that the bug (if it is a bug) could also be circumvented by the
> calling function by detecting a failed cache lookup and doing
> replan/requery itself, but this would require all PL implementations and
> other functions with stored plans to do it independently.
>
> -----
> Hannu
>
>
>
>

2008/8/19 Hannu Krosing <hannu@2ndquadrant.com>:
> On Mon, 2008-08-18 at 22:41 +0200, Pavel Stehule wrote:
>> 2008/8/18 Hannu Krosing <hannu@2ndquadrant.com>:
>> > On Mon, 2008-08-18 at 11:05 +0200, Pavel Stehule wrote:
>> >> 2008/8/18 Dimitri Fontaine <dfontaine@hi-media.com>:
>> >> > Hi,
>> >> >
>> >> > Le lundi 18 août 2008, Andrew Dunstan a écrit :
>> >> >> > On Sat, Aug 16, 2008 at 09:40:19PM -0400, Tom Lane wrote:
>> >> >> >> This is not the kind of patch we put into stable branches.
>> >> >>
>> >> >> So what? That is not the only criterion for backpatching.
>> >> >
>> >> > I fail to understand why this problem is not qualified as a bug.
>> >> >
>> >>
>> >> Does it change of result some queries?
>> >
>> > Not in the long run, but not invalidating the functions (current
>> > behaviour) postpones seeing the results of function change until DBA
>> > manually restarts the error-producing client.
>> >
>> >> It is protection to server's hang?
>> >
>> > Can't understand this question :(
>> >
>> > If you mean, does the change protect against hanging the server, then
>> > no, currently the server does not actually hang, it just becomes
>> > unusable until reconnect :(
>>
>> Hi
>>
>> I am sorry, but it's really new feature and not bug fix
>
> Could you please explain why you think so ?
>
> As I see it, the patch does not change visible behaviour, except
> removing some sonditions where client becomes unusable after some other
> backend does some legitimate changes.
>
> Is the current behavior planned or even defined by spec ?
>
> I agree, that the bug (if it is a bug) could also be circumvented by the
> calling function by detecting a failed cache lookup and doing
> replan/requery itself, but this would require all PL implementations and
> other functions with stored plans to do it independently.
>

I am not against to this patch or this feature. But I am sure, so
isn't well to do not necessary changes in stable version.

Pavel

> -----
> Hannu
>
>
>
>

On Tue, 2008-08-19 at 12:42 +0200, Pavel Stehule wrote:
> 2008/8/19 Hannu Krosing <hannu@2ndquadrant.com>:
> > On Mon, 2008-08-18 at 22:41 +0200, Pavel Stehule wrote:
> >> 2008/8/18 Hannu Krosing <hannu@2ndquadrant.com>:
> >> > On Mon, 2008-08-18 at 11:05 +0200, Pavel Stehule wrote:
> >> >> 2008/8/18 Dimitri Fontaine <dfontaine@hi-media.com>:
> >> >> > Hi,
> >> >> >
> >> >> > Le lundi 18 août 2008, Andrew Dunstan a écrit :
> >> >> >> > On Sat, Aug 16, 2008 at 09:40:19PM -0400, Tom Lane wrote:
> >> >> >> >> This is not the kind of patch we put into stable branches.
> >> >> >>
> >> >> >> So what? That is not the only criterion for backpatching.
> >> >> >
> >> >> > I fail to understand why this problem is not qualified as a bug.
> >> >> >
> >> >>
> >> >> Does it change of result some queries?
> >> >
> >> > Not in the long run, but not invalidating the functions (current
> >> > behaviour) postpones seeing the results of function change until DBA
> >> > manually restarts the error-producing client.
> >> >
> >> >> It is protection to server's hang?
> >> >
> >> > Can't understand this question :(
> >> >
> >> > If you mean, does the change protect against hanging the server, then
> >> > no, currently the server does not actually hang, it just becomes
> >> > unusable until reconnect :(
> >>
> >> Hi
> >>
> >> I am sorry, but it's really new feature and not bug fix
> >
> > Could you please explain why you think so ?
> >
> > As I see it, the patch does not change visible behaviour, except
> > removing some sonditions where client becomes unusable after some other
> > backend does some legitimate changes.
> 
> Are you sure, so this behave hasn't any secondary effect? So this
> change doesn't breaks any application?

I can't think of any.

What it does, is it makes the changed function usable right after
redefining the new function.

Current behaviour is to make the calling function unusable until the
backend is restarted, after which it still will use the new version of
the function.

> Pavel
> 
> >
> > Is the current behavior planned or even defined by spec ?
> >
> > I agree, that the bug (if it is a bug) could also be circumvented by the
> > calling function by detecting a failed cache lookup and doing
> > replan/requery itself, but this would require all PL implementations and
> > other functions with stored plans to do it independently.
> >
> > -----
> > Hannu
> >
> >
> >
> >
> 

"Hannu Krosing" <hannu@2ndQuadrant.com> writes:

> Maybe there should be something in postgreSQL docs that warns users against 
> using functions in any non-trivial circumstances, as functions are not 
> expected to behave like the rest of postgreSQL features and there is 
> not plan to fix that ?

Now who's trolling :)

--  Gregory Stark EnterpriseDB          http://www.enterprisedb.com Get trained by Bruce Momjian - ask me about
EnterpriseDB'sPostgreSQL training!
 

Hannu Krosing <hannu@2ndQuadrant.com> writes:
> On Mon, 2008-08-18 at 22:41 +0200, Pavel Stehule wrote:
>> I am sorry, but it's really new feature and not bug fix

> Could you please explain why you think so ?

For the same reasons that plan invalidation itself was a new feature
and not a bug fix; notably, risk vs reward tradeoff and not wanting
to change long-established behavior in stable branches.
        regards, tom lane

Le mardi 19 août 2008, Tom Lane a écrit :
> For the same reasons that plan invalidation itself was a new feature
> and not a bug fix;

I'm sorry but that doesn't help me a dime to understand current situation. It
could well be just me, but... here's how I see it:
- plan invalidation is a new feature in 8.3- previous releases are out of business: new feature against stable code-
now,we have found a bug in plan invalidation- HEAD (to be 8.4) will get some new code to fix it 

But 8.3 won't benefit from this bugfix? On the grounds that the feature which
is now deployed on the field should *maybe* not get used the way it *is*?

Sorry again, I really don't get it.
--
dim

On Tue, 19 Aug 2008 12:48:06 +0100
Gregory Stark <stark@enterprisedb.com> wrote:

> "Hannu Krosing" <hannu@2ndQuadrant.com> writes:
> 
> > Maybe there should be something in postgreSQL docs that warns users
> > against using functions in any non-trivial circumstances, as
> > functions are not expected to behave like the rest of postgreSQL
> > features and there is not plan to fix that ?
> 
> Now who's trolling :)

Although I read his remark as sarcastic after reading the entire
thread I have to say it may be a good idea to have the something in
the docs about the limitation. I never think about it anymore
because I am used to the behavior. I can see where and entity
like skype who has I am sure thousands of procedures would have this
as a constant irritant.

Do I think it should be pushed back to 8.3.x; no. It is a feature. I
don't consider the existing behavior a bug. I consider it a limitation
and we don't back patch fixes for limitations. 

Sincerely,

Joshua D. Drake


-- 
The PostgreSQL Company since 1997: http://www.commandprompt.com/ 
PostgreSQL Community Conference: http://www.postgresqlconference.org/
United States PostgreSQL Association: http://www.postgresql.us/
Donate to the PostgreSQL Project: http://www.postgresql.org/about/donate

Dimitri Fontaine <dfontaine@hi-media.com> writes:
>  - now, we have found a bug in plan invalidation

[ shrug... ] You have not found a bug in plan invalidation.  You have
found omitted functionality --- functionality that was *intentionally*
omitted from the 8.3 version.
        regards, tom lane

Le mardi 19 août 2008, Tom Lane a écrit :
> [ shrug... ] You have not found a bug in plan invalidation.  You have
> found omitted functionality --- functionality that was *intentionally*
> omitted from the 8.3 version.

Thanks a lot for this clarification, now I understand you viewpoint.

So, the 8.3 "fix" would be about documenting this intentionnal omit in the
great manual, maybe in a Limits section of the sql-createfunction page?

Another thing I do not understand well is how people are expected to work in
8.3 with a function based API, without hitting Skype problems. I'm having a
project here where the project manager wants a database function API to keep
data logic at serverside, should I tell him to reconsider this while 8.4 is
not ready?
We would then have to go live with an 8.3 based solution containing middleware
code, then port it again to SQL functions when 8.4 is out & stable. Not
appealing, but I sure understand the no new feature in stable code base
argument here.

Regards,
--
dim

Dimitri Fontaine <dfontaine@hi-media.com> writes:
> Another thing I do not understand well is how people are expected to work in 
> 8.3 with a function based API, without hitting Skype problems.

I could understand this level of complaining if this were a new problem
that'd appeared in 8.3.  But *every PG version that we've ever released*
behaves the same way with respect to function drop/recreate.  If the
Skype folk have developed a way of working that is guaranteed not to
work with any released version, one has to wonder what they were
thinking.

If you need to DROP rather than CREATE OR REPLACE functions, then 8.3
doesn't make things better for you than prior releases did, but it
does't make them worse either.  Making things better for that case is
unequivocally a new feature.  And it's rather a corner case at that,
else there would have been enough prior complaints to put it on the
radar screen for 8.3.

What we've got at this point is a submitted patch for a new feature
that hasn't even been accepted into HEAD yet.  Lobbying to get it
back-patched is entirely inappropriate IMHO.
        regards, tom lane

2008/8/19 Dimitri Fontaine <dfontaine@hi-media.com>:
> Le mardi 19 août 2008, Tom Lane a écrit :
>> [ shrug... ] You have not found a bug in plan invalidation.  You have
>> found omitted functionality --- functionality that was *intentionally*
>> omitted from the 8.3 version.
>
> Thanks a lot for this clarification, now I understand you viewpoint.
>
> So, the 8.3 "fix" would be about documenting this intentionnal omit in the
> great manual, maybe in a Limits section of the sql-createfunction page?
>
> Another thing I do not understand well is how people are expected to work in
> 8.3 with a function based API, without hitting Skype problems. I'm having a
> project here where the project manager wants a database function API to keep
> data logic at serverside, should I tell him to reconsider this while 8.4 is
> not ready?

You could to use patched 8.3.

> We would then have to go live with an 8.3 based solution containing middleware
> code, then port it again to SQL functions when 8.4 is out & stable. Not
> appealing, but I sure understand the no new feature in stable code base
> argument here.

This problem isn't too hard without pooling. Not all systems are
global - so usually is possible to find some window and recreate
functions and close all user connections.

Regards
Pavel Stehule

>
> Regards,
> --
> dim
>

Joshua Drake wrote:
> On Tue, 19 Aug 2008 12:48:06 +0100
> Gregory Stark <stark@enterprisedb.com> wrote:
> 
> > "Hannu Krosing" <hannu@2ndQuadrant.com> writes:
> > 
> > > Maybe there should be something in postgreSQL docs that warns users
> > > against using functions in any non-trivial circumstances, as
> > > functions are not expected to behave like the rest of postgreSQL
> > > features and there is not plan to fix that ?
> > 
> > Now who's trolling :)
> 
> Although I read his remark as sarcastic after reading the entire
> thread I have to say it may be a good idea to have the something in
> the docs about the limitation. I never think about it anymore
> because I am used to the behavior. I can see where and entity
> like skype who has I am sure thousands of procedures would have this
> as a constant irritant.
> 
> Do I think it should be pushed back to 8.3.x; no. It is a feature. I
> don't consider the existing behavior a bug. I consider it a limitation
> and we don't back patch fixes for limitations. 

The bottom line here is that we don't have the time to explain or
justify our backpatch policy every time someone shows up with a bug that
needs to be fixed.

If you want to create your own version of Postgres, go ahead;  no one is
stopping you.  But if we backpatched everything and we introduced bugs
or change behavior, more people would complain.

--  Bruce Momjian  <bruce@momjian.us>        http://momjian.us EnterpriseDB
http://enterprisedb.com
 + If your life is a hard drive, Christ can be your backup. +

On Tue, 19 Aug 2008 14:29:52 -0400 (EDT)
Bruce Momjian <bruce@momjian.us> wrote:


> > Do I think it should be pushed back to 8.3.x; no. It is a feature. I
> > don't consider the existing behavior a bug. I consider it a
> > limitation and we don't back patch fixes for limitations. 
> 
> The bottom line here is that we don't have the time to explain or
> justify our backpatch policy every time someone shows up with a bug
> that needs to be fixed.

Is our backpatch policy documented? It does not appear to be in
developer FAQ.

Joshua D. Drake


-- 
The PostgreSQL Company since 1997: http://www.commandprompt.com/ 
PostgreSQL Community Conference: http://www.postgresqlconference.org/
United States PostgreSQL Association: http://www.postgresql.us/
Donate to the PostgreSQL Project: http://www.postgresql.org/about/donate

Joshua Drake wrote:
> On Tue, 19 Aug 2008 14:29:52 -0400 (EDT)
> Bruce Momjian <bruce@momjian.us> wrote:
> 
> 
> > > Do I think it should be pushed back to 8.3.x; no. It is a feature. I
> > > don't consider the existing behavior a bug. I consider it a
> > > limitation and we don't back patch fixes for limitations. 
> > 
> > The bottom line here is that we don't have the time to explain or
> > justify our backpatch policy every time someone shows up with a bug
> > that needs to be fixed.
> 
> Is our backpatch policy documented? It does not appear to be in
> developer FAQ.

Seems we need to add it.

--  Bruce Momjian  <bruce@momjian.us>        http://momjian.us EnterpriseDB
http://enterprisedb.com
 + If your life is a hard drive, Christ can be your backup. +

>>> Joshua Drake <jd@commandprompt.com> wrote:
> Is our backpatch policy documented? It does not appear to be in
> developer FAQ.
It's mentioned here:
http://www.postgresql.org/support/versioning
"PostgreSQL minor releases fix only frequently-encountered, security,
and data corruption bugs to reduce the risk of upgrading."
-Kevin

Bruce Momjian <bruce@momjian.us> writes:
> Joshua Drake wrote:
>> Is our backpatch policy documented? It does not appear to be in
>> developer FAQ.

> Seems we need to add it.

I'm not sure that I *want* a formal written-down backpatch policy.
Whether (and how far) to backpatch has always been a best-judgment call
in the past, and we've gotten along fine with that.  I think having a
formal policy is just likely to lead to even more complaints: either
patching or not patching could result in second-guessing by someone
who feels he can construe the policy to match the result he prefers.
        regards, tom lane

Tom Lane wrote:
> Bruce Momjian <bruce@momjian.us> writes:
> > Joshua Drake wrote:
> >> Is our backpatch policy documented? It does not appear to be in
> >> developer FAQ.
> 
> > Seems we need to add it.
> 
> I'm not sure that I *want* a formal written-down backpatch policy.
> Whether (and how far) to backpatch has always been a best-judgment call
> in the past, and we've gotten along fine with that.  I think having a
> formal policy is just likely to lead to even more complaints: either
> patching or not patching could result in second-guessing by someone
> who feels he can construe the policy to match the result he prefers.

OK, agreed.
--  Bruce Momjian  <bruce@momjian.us>        http://momjian.us EnterpriseDB
http://enterprisedb.com
 + If your life is a hard drive, Christ can be your backup. +

Asko Oja escribió:
> > Another thing I do not understand well is how people are expected to work
> in
> > 8.3 with a function based API, without hitting Skype problems.
> People are expected to use same workarounds as Skype is using. For us
> another unneccessary downtime week ago was what set us moving/thinking :).
> When you use software with limitations then you learn to live with them.
> Good thing about postgres you can do something yourself to get some of the
> limitations removed.

Make sure you do not live with patches forever, i.e. that it gets into
8.4.  Otherwise it's going to be a pain for everyone.

-- 
Alvaro Herrera                                http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Hi,

Le 19 août 08 à 19:06, Tom Lane a écrit :
> Dimitri Fontaine <dfontaine@hi-media.com> writes:
>> Another thing I do not understand well is how people are expected
>> to work in
>> 8.3 with a function based API, without hitting Skype problems.
>
> What we've got at this point is a submitted patch for a new feature
> that hasn't even been accepted into HEAD yet.  Lobbying to get it
> back-patched is entirely inappropriate IMHO.

Well, there's a misunderstanding here. I certainly were lobbying for
considering a backpatch as I saw it as a bugfix. You told me it's a
new feature, I say ok for not backpatching, obviously.

This mail was a real attempt at learning some tips to be able to push
the functions usage as far as Skype is doing, in 8.3 release, and
avoiding the trap which has always existed in released PostgreSQL
version. This certainly was a bad attempt at it.

Now, my understanding is that rolling out new versions of functions
requires forcing dropping all current opened sessions as soon as
PostgreSQL considers you need to drop any function. I'll think about
it in next project design meetings.

Regards,
- --
dim


-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Darwin)

iEYEARECAAYFAkirHlEACgkQlBXRlnbh1bk4YQCgswDS1bu+P+N7yKJvwnRAWnL3
FYkAnRZQzqbEoahShh/Qz9mnrIm1e99y
=hIBt
-----END PGP SIGNATURE-----

Le 19 août 08 à 20:47, Tom Lane a écrit :
> I'm not sure that I *want* a formal written-down backpatch policy.
> Whether (and how far) to backpatch has always been a best-judgment
> call
> in the past, and we've gotten along fine with that.  I think having a
> formal policy is just likely to lead to even more complaints: either
> patching or not patching could result in second-guessing by someone
> who feels he can construe the policy to match the result he prefers.

Agreed.
The problem here (at least for me) was to understand why this (yet to
be reviewed) patch is about implementing a new feature and not about
bugfixing an existing one. So we're exactly in the fog around the
informal backpatch policy, and as long as we're able to continue
talking nicely about it, this seems the finest solution :)

Keep up the amazing work, regards,
--
dim

On Tue, 19 Aug 2008 14:47:13 -0400
Tom Lane <tgl@sss.pgh.pa.us> wrote:

> Bruce Momjian <bruce@momjian.us> writes:
> > Joshua Drake wrote:
> >> Is our backpatch policy documented? It does not appear to be in
> >> developer FAQ.
> 
> > Seems we need to add it.
> 
> I'm not sure that I *want* a formal written-down backpatch policy.

Then we write a formal guideline. It really isn't fair to new developers
to not have any idea how they are going to be able to get a patch
applied to older branches. Something like:

Generally speaking we adhere to the following guideline for patches.  * Security fixes are applied to all applicable
branches. * Bugfixes are applied to all applicable branches     * Note: A patch that addresses a known limitation is
generally
not backpatched  * New features are always applied to -HEAD only.

This is not a policy as much as a legend for developers to consider
before they submit their patch.

If we do this, we have the opportunity to just point to the FAQ when
there is no ambiguity. It also increases transparency of the process;
which is always a good thing.

Sincerely,

Joshua D. Drake



-- 
The PostgreSQL Company since 1997: http://www.commandprompt.com/ 
PostgreSQL Community Conference: http://www.postgresqlconference.org/
United States PostgreSQL Association: http://www.postgresql.us/
Donate to the PostgreSQL Project: http://www.postgresql.org/about/donate

On Tue, Aug 19, 2008 at 02:47:13PM -0400, Tom Lane wrote:

> Whether (and how far) to backpatch has always been a best-judgment call
> in the past, and we've gotten along fine with that.  I think having a
> formal policy is just likely to lead to even more complaints:

I completely agree with this.  If you formalise the back-patch policy,
then it will be necessary to invent classifications for bug severity
to determine whether to back patch.  This will inevitably lead to some
sort of false objectivity measure, where bugs get a "severity number"
that actually just means "we have already decided to back-patch".

A

-- 
Andrew Sullivan
ajs@commandprompt.com
+1 503 667 4564 x104
http://www.commandprompt.com/

> Another thing I do not understand well is how people are expected to work in
> 8.3 with a function based API, without hitting Skype problems. I'm having a

All database-driven applications have this problem.  Any time you have
a database on the backend and interface code on the front-end, you
need to keep in mind that it won't necessarily be possible to update
the two of them simultaneously, especially if you have multiple
back-ends and multiple front-ends, as you almost certainly do.  Even
if PostgreSQL invalidated plans in the particular situation you're
discussing, there would still be other problems.  You could add a new,
non-NULLable column to a table before updating the code that insert
into that table, or drop an old column that the code still counts on
being able to access.

I handle these problems all the time by ordering the changes
carefully. If I need to change a function API in an incompatible way,
I change the NAME of the function as well as the type signature (eg.
do_important_thing -> do_important_thing_v2).  Then I change the code.Then I remove the old function once everything
thatrelies on it is
 
dead.

Maybe in your particular environment plan invalidation for functions
will solve most of the cases you care about, but I respectfully submit
that there's no substitute for good release engineering.  If you don't
know exactly what functions are going to be created, modified, or
dropped on your production servers during each release before you
actually roll that release out...  you probably need to improve your
internal documentation.

...Robert

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Hi,

Le 19 août 08 à 22:03, Robert Haas a écrit :
> All database-driven applications have this problem.  Any time you have
> a database on the backend and interface code on the front-end, you
> need to keep in mind that it won't necessarily be possible to update
> the two of them simultaneously, especially if you have multiple
> back-ends and multiple front-ends, as you almost certainly do.  Even
> if PostgreSQL invalidated plans in the particular situation you're
> discussing, there would still be other problems.  You could add a new,
> non-NULLable column to a table before updating the code that insert
> into that table, or drop an old column that the code still counts on
> being able to access.

Using functions the way Skype uses them means not issuing a single
insert, update or delete directly from your code, but calling a
function which takes care about it.
So you use PostgreSQL transactionnal DDL to roll-out new function
versions at the same time you push the schema modifications, and
commit it all in one go.

> Maybe in your particular environment plan invalidation for functions
> will solve most of the cases you care about

When the code only is a client to an SQL functions API, which
effectively replaces SQL as the way to interact between code and
database, then I believe plan invalidation at function change is the
missing piece.

> , but I respectfully submit
> that there's no substitute for good release engineering.  If you don't
> know exactly what functions are going to be created, modified, or
> dropped on your production servers during each release before you
> actually roll that release out...  you probably need to improve your
> internal documentation.

Agreed :)
- --
dim


-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Darwin)

iEYEARECAAYFAkirK2kACgkQlBXRlnbh1bmxvQCgmowpfnZ5nFRml0mNfj2HRE+3
HJEAnR3G6Lhnb7R4+iSze8xGACwyk4D7
=of1o
-----END PGP SIGNATURE-----

On Tue, Aug 19, 2008 at 12:42:29PM -0700, Joshua Drake wrote:
> Generally speaking we adhere to the following guideline for patches.
>    * Security fixes are applied to all applicable branches.
>    * Bugfixes are applied to all applicable branches
>       * Note: A patch that addresses a known limitation is generally
> not backpatched
>    * New features are always applied to -HEAD only.
> 
> This is not a policy as much as a legend for developers to consider
> before they submit their patch.

But it's meaningless.  "Bugfixes are applied to all applicable
branches," is either false or trivially true.  It's trivially true if
you interpret "applicable branches" to mean "the ones that get the
patch".  It's false if you mean "bugfix" to mean "every patch that
fixes a bug".  I can think of bugs that we have lived with in older
releases because fixing them was too risky or because the bug was so
tiny or unusual as to make the risk greater than the reward.

A formal policy that's any more detailed than what's in the FAQ today
is a solution in search of a problem.

A

-- 
Andrew Sullivan
ajs@commandprompt.com
+1 503 667 4564 x104
http://www.commandprompt.com/

On Tue, 2008-08-19 at 21:26 +0200, Dimitri Fontaine wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
> 
> Hi,
> 
> Le 19 août 08 à 19:06, Tom Lane a écrit :
> > Dimitri Fontaine <dfontaine@hi-media.com> writes:
> >> Another thing I do not understand well is how people are expected  
> >> to work in
> >> 8.3 with a function based API, without hitting Skype problems.
> >
> > What we've got at this point is a submitted patch for a new feature
> > that hasn't even been accepted into HEAD yet.  Lobbying to get it
> > back-patched is entirely inappropriate IMHO.
> 
> Well, there's a misunderstanding here. I certainly were lobbying for  
> considering a backpatch as I saw it as a bugfix. You told me it's a  
> new feature, I say ok for not backpatching, obviously.
> 
> This mail was a real attempt at learning some tips to be able to push  
> the functions usage as far as Skype is doing, in 8.3 release, and  
> avoiding the trap which has always existed in released PostgreSQL  
> version. This certainly was a bad attempt at it.
> 
> Now, my understanding is that rolling out new versions of functions  
> requires forcing dropping all current opened sessions as soon as  
> PostgreSQL considers you need to drop any function. I'll think about  
> it in next project design meetings.

I think that another option is to manipulate pg_proc - just do a no-op
update to advance xmin for all functions that may have cached plans.

UPDATE pg_proc SET proname = proname;

then make sure that pg_proc is vacuumed often enough.

It's a bit wasteful, as it forces re-planning of all functions, but
should have similar effect than the patch.

It's also possible that updating pg_proc in bulk introduces some race
conditions which lock up the database.

------------------
Hannu

On Tue, 19 Aug 2008 16:22:43 -0400
Andrew Sullivan <ajs@commandprompt.com> wrote:

> A formal policy that's any more detailed than what's in the FAQ today
> is a solution in search of a problem.

Odd that the problem continues to rear its head though isn't it? This
certainly isn't the first time it has come up.  I have however made my
argument. I also tried to help solve the problem. If we aren't
interested in a solution, oh well. It doesn't make my life any harder.


Sincerely,

Joshua D. Drake


-- 
The PostgreSQL Company since 1997: http://www.commandprompt.com/ 
PostgreSQL Community Conference: http://www.postgresqlconference.org/
United States PostgreSQL Association: http://www.postgresql.us/
Donate to the PostgreSQL Project: http://www.postgresql.org/about/donate

Dimitri Fontaine escribió:

> The problem here (at least for me) was to understand why this (yet to be 
> reviewed) patch is about implementing a new feature and not about  
> bugfixing an existing one. So we're exactly in the fog around the  
> informal backpatch policy, and as long as we're able to continue talking 
> nicely about it, this seems the finest solution :)

The actual criterion is not really "new user-visible feature" versus
"bug fix".  It's more an attempt at measuring how large a potential
impact the change has.  The patch I saw was introducing a whole new
message type to go through the shared invalidation queue, which is not
something to be taken lightly (consider that there are three message
types of messages currently.)

It's possible that for the Skype usage this patch introduces the
behavior they want.  But for other people, perhaps this kind of
invalidation causes secondary effects that are completely unforeseen --
what if it breaks their apps and they must carry out a week's work to
fix it?  What if a serious security problem is discovered tomorrow and
they can't update because we've broken backwards compatibility for them?

-- 
Alvaro Herrera                                http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

Alvaro Herrera <alvherre@commandprompt.com> writes:
> The actual criterion is not really "new user-visible feature" versus
> "bug fix".  It's more an attempt at measuring how large a potential
> impact the change has.  The patch I saw was introducing a whole new
> message type to go through the shared invalidation queue, which is not
> something to be taken lightly (consider that there are three message
> types of messages currently.)

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.

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.
        regards, tom lane

On Tue, 2008-08-19 at 16:03 -0400, Robert Haas wrote:
> > Another thing I do not understand well is how people are expected to work in
> > 8.3 with a function based API, without hitting Skype problems. I'm having a
> 
> All database-driven applications have this problem.  Any time you have
> a database on the backend and interface code on the front-end, you
> need to keep in mind that it won't necessarily be possible to update
> the two of them simultaneously, especially if you have multiple
> back-ends and multiple front-ends, as you almost certainly do.  Even
> if PostgreSQL invalidated plans in the particular situation you're
> discussing, there would still be other problems.  You could add a new,
> non-NULLable column to a table before updating the code that insert
> into that table, or drop an old column that the code still counts on
> being able to access.
> 
> I handle these problems all the time by ordering the changes
> carefully. If I need to change a function API in an incompatible way,
> I change the NAME of the function as well as the type signature (eg.
> do_important_thing -> do_important_thing_v2).  Then I change the code.
>  Then I remove the old function once everything that relies on it is
> dead.

Not having plan invalidation forces you to have do_important_thing_v2
for do_important_thing even with no changes in source code, just for the
fact that do_part_of_important_thing() which it calls has changed.

An example -
you have functions

A) caller1() to callerN() which includes call to called1() 

B) one of these functions, say callerM() needs one more field returned
from called1(), so you either write a completely new function
called1_v2() with one more field and then update  callerM() to call
called1_v2()

C) now, to get rid of called1() you have to replace called1 with
called1_v2 also in all other functions caller1() to callerN() 

D) then you can drop called1()

if you missed one of callerx() functions (you can drop called1() even if
it is used, as postgreSQL does not check dependencies in functions) then
you have a non-functioning database, where even client reconnect won't
help, only putting called1() back.

If there is plan invalidation then you just change called1() to return
one more field and that's it - no juggling with C) and D) and generally
less things that can go wrong.

> Maybe in your particular environment plan invalidation for functions
> will solve most of the cases you care about, but I respectfully submit
> that there's no substitute for good release engineering. 

Nope, but the amount of release engineering (and deployment-time work)
you need to do depends a lot on fragility of the system.

The more arcane and fragile the system is, the more you need to rely on
external systems and procedures to keep it working.

Imagine how much harder it would be, if there were no transactions and
you had to ensure the right ordering of all changes by release process
only. You probably would end up doing several times more work and
temporary hacks and you would still be out of luck doing _any_
nontrivial updates while the systems are running 24/7.

> If you don't
> know exactly what functions are going to be created, modified, or
> dropped on your production servers during each release before you
> actually roll that release out...  

this is not about knowing this at all - this is about needing to change
less, about optimizing on work that does not need to be done if system
is smarter.

> you probably need to improve your internal documentation.

or improve the database system you use.

if you need to change less functions, you also need less documentation
about changes. if you can prove that "select a,b from f()" always
returns the same data as "select a,b from f_b2()" then you don't need to
write f_b2() at all, you just redefine f() and can also skip migrating
all callers of f() to f_v2() just to satisfy your databases quirks.

---------------
Hannu

On Tue, 2008-08-19 at 16:56 -0400, Tom Lane wrote:
> Alvaro Herrera <alvherre@commandprompt.com> writes:
> > The actual criterion is not really "new user-visible feature" versus
> > "bug fix".  It's more an attempt at measuring how large a potential
> > impact the change has.  The patch I saw was introducing a whole new
> > message type to go through the shared invalidation queue, which is not
> > something to be taken lightly (consider that there are three message
> > types of messages currently.)
> 
> 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.
> 
> 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.

Or maybe a simpler and smaller patch - just invalidate everything on
every schema change :)

It will have a momentary impact on performance at DDL time, but
otherways might be more robust and easier to check for errors.

-------------
Hannu

On Tue, 2008-08-19 at 16:56 -0400, Tom Lane wrote:
> Alvaro Herrera <alvherre@commandprompt.com> writes:
> > The actual criterion is not really "new user-visible feature" versus
> > "bug fix".  It's more an attempt at measuring how large a potential
> > impact the change has.  The patch I saw was introducing a whole new
> > message type to go through the shared invalidation queue, which is not
> > something to be taken lightly (consider that there are three message
> > types of messages currently.)
> 
> 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.

I have'nt looke at the patch either, but I suspect that what goes
through shared mem is the registration for invalidation, as dependent
function OIDs are only learned while compiling functions

so when f_caller() learns that it caches plan f_called() then it
registers through shared mem message its wish to invalidate this plan if
f_called() is dropped or redefined.

--------------
Hannu

>  you have functions
>
> A) caller1() to callerN() which includes call to called1()
>
> B) one of these functions, say callerM() needs one more field returned
> from called1(), so you either write a completely new function
> called1_v2() with one more field and then update  callerM() to call
> called1_v2()
>
> C) now, to get rid of called1() you have to replace called1 with
> called1_v2 also in all other functions caller1() to callerN()
>
> D) then you can drop called1()

True.  I complained about this same problem in the context of views -
you can add a column to a table in place but not to a view, or even a
type created via CREATE TYPE.  I even went so far as to develop a
patch[1] to improve the situation, which to my sadness was not met
with wild enthusiasm.

[1] http://archives.postgresql.org/pgsql-hackers/2008-08/msg00272.php

Does it help to do CREATE OR REPLACE FUNCTION on callerX() after
dropping and recreating called1()?  If so, you might want to just
recreate all of your functions every time you do a release.  I wrote a
perl script that does this and it's worked pretty well for me.
Besides possibly avoiding this problem, it means that I don't really
need to worry about which functions I've modified in this release
quite as much, since I'm just going to push out the most-current
definition for all of them.

> Nope, but the amount of release engineering (and deployment-time work)
> you need to do depends a lot on fragility of the system.

Also true, but I think comparing plan invalidation to transactional
semantics is quite unfair.  There's basically no amount of user code
which will compensate for the lack of an ACID-compliant database.  On
the other hand, working around the lack of plan invalidation (or the
inability to add columns to views without recreating them) just
requires being careful to catch all of the stray references in your
DDL and testing thoroughly before you roll out to production, which
are good things to do anyway.  That's not to say that we shouldn't
have plan invalidation, just that I don't think it's anywhere close to
the same level of broken.

...Robert

On Wed, 2008-08-20 at 00:11 +0300, Hannu Krosing wrote:
> On Tue, 2008-08-19 at 16:56 -0400, Tom Lane wrote:
> > 
> > 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.
> 
> Or maybe a simpler and smaller patch - just invalidate everything on
> every schema change :)
> 
> It will have a momentary impact on performance at DDL time, but
> otherways might be more robust and easier to check for errors.

I think Tom's main question is the right one: how much to invalidate?

ISTM that there must be some user defined control over how this occurs.
We have cascade and restrict as options in other places.

Being able to force replanning of everything when you know its the right
thing to do sounds sensible and useful. Being able to avoid it when you
know it isn't needed also sounds sensible and useful.

It would be useful to have an impact assessment tool, so we could say
"if I made this change, how many plans would it effect?". We can't do
that because the plans aren't shared. Perhaps that is a good argument
for a shared plan cache, or at least some way of defining whether some
plans are shared, some not.

-- Simon Riggs           www.2ndQuadrant.comPostgreSQL Training, Services and Support

Joshua Drake <jd@commandprompt.com> writes:
> Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> I'm not sure that I *want* a formal written-down backpatch policy.

> Then we write a formal guideline. It really isn't fair to new developers
> to not have any idea how they are going to be able to get a patch
> applied to older branches. Something like:

> Generally speaking we adhere to the following guideline for patches.
>    * Security fixes are applied to all applicable branches.
>    * Bugfixes are applied to all applicable branches
>       * Note: A patch that addresses a known limitation is generally
> not backpatched
>    * New features are always applied to -HEAD only.

That just begs the question of what's the difference between a "bug" and
a "limitation".  AFAICS, having such a policy/guideline/whatchacallit
in place wouldn't have done a single thing to stop the current flamewar,
because the people who want this thing back-patched are insisting that
it's a bug, while those who don't are saying it's a long-known
limitation.

Also, there are a whole lot more considerations in a backpatch decision
than just "is it a bug".  The (estimated) risk of creating new bugs and
the extent to which the patch will change behavior that apps might be
relying on are two big reasons why we might choose not to back-patch
a bug fix.
        regards, tom lane

Hannu Krosing <hannu@2ndQuadrant.com> writes:
> If there is plan invalidation then you just change called1() to return
> one more field and that's it - no juggling with C) and D) and generally
> less things that can go wrong.

That is a pure flight of fancy.  Adjusting a function's API generally
requires source-code changes on the caller side too.  There might be
a few limited cases where you can avoid that, but that doesn't leave
you with much of an argument that this is a critical bug fix.  It's
a corner case and little more.

FWIW, given that there will probably always be corner cases. I can see
the attraction in Simon's suggestion of providing a way to manually
issue a system-wide forced plan flush.
        regards, tom lane

Tom Lane wrote:
> Also, there are a whole lot more considerations in a backpatch decision
> than just "is it a bug".  The (estimated) risk of creating new bugs and
> the extent to which the patch will change behavior that apps might be
> relying on are two big reasons why we might choose not to back-patch
> a bug fix.
>
>
>   

Right. And even if it is a bug the question might be "what sort of bug 
is it?" We might well be prepared to take some risks with code stability 
to plug security or data corruption bugs, a lot more than we would for 
other sorts of bugs. Even if this were considered a bug instead of a 
limitation, it doesn't come into the class of things we should be 
rushing to fix in the stable branches, unless the fix is fairly obvious 
and of limited impact, which is clearly not the case.

cheers

andrew

On Tue, Aug 19, 2008 at 07:45:16PM -0400, Tom Lane wrote:
> Hannu Krosing <hannu@2ndQuadrant.com> writes:
> > If there is plan invalidation then you just change called1() to
> > return one more field and that's it - no juggling with C) and D)
> > and generally less things that can go wrong.
> 
> That is a pure flight of fancy.  Adjusting a function's API
> generally requires source-code changes on the caller side too.
> There might be a few limited cases where you can avoid that, but
> that doesn't leave you with much of an argument that this is a
> critical bug fix.  It's a corner case and little more.
> 
> FWIW, given that there will probably always be corner cases. I can
> see the attraction in Simon's suggestion of providing a way to
> manually issue a system-wide forced plan flush.

Would that require a system-wide plan cache to implement?

Cheers,
David.
-- 
David Fetter <david@fetter.org> http://fetter.org/
Phone: +1 415 235 3778  AIM: dfetter666  Yahoo!: dfetter
Skype: davidfetter      XMPP: david.fetter@gmail.com

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

Andrew Dunstan <andrew@dunslane.net> writes:
> Right. And even if it is a bug the question might be "what sort of bug 
> is it?" We might well be prepared to take some risks with code stability 
> to plug security or data corruption bugs, a lot more than we would for 
> other sorts of bugs.

As indeed we have done, and lost the bet more than once :-(.  Rev 8.2.2
and siblings being the most recent example.  A quick review of the
release history will show other cases where well-intentioned, seemingly
safe back-patches broke things.

Now security patches are the worst-case scenario for this, because they
typically go out with no significant public review.  But even a regular
bug-fix patch doesn't get all that much testing in the back branches
before it hits the streets as a supposedly-stable update.  By and large,
if we commit something into REL8_3_STABLE today, it's going to appear
in 8.3.4 with nothing more than buildfarm testing.  That is a sobering
prospect, and not one that makes me want to put nontrivial patches in
there except at great need.
        regards, tom lane

David Fetter <david@fetter.org> writes:
> On Tue, Aug 19, 2008 at 07:45:16PM -0400, Tom Lane wrote:
>> FWIW, given that there will probably always be corner cases. I can
>> see the attraction in Simon's suggestion of providing a way to
>> manually issue a system-wide forced plan flush.

> Would that require a system-wide plan cache to implement?

No, just a function that can issue a suitable sinval message.

plancache.c would already respond in the desired way to a relcache inval
message with OID = 0, though likely it'll be cleaner to invent an sinval
message type specifically for the purpose.

One thing to think about is whether the flush should be truly
system-wide or just database-wide.  I can see a lot more uses for the
latter than the former --- I don't think there's a reason for cached
plans to depend on any contents of the shared catalogs.
        regards, tom lane

<div dir="ltr">Every thread we are concerned in turns into something strange thing that is almost entirely differnet
fromthe original intention. First thread we started was with the intention to discuss how we should handle the problem.
Insteadof discussion it was trolled into oblivion. Then we thought so what if no discussion we will submit a patch
maybepeople will understand we are serious. Nothing relevant came up. Spent week more to refine patch into something
thatlooks good enough. And now we are having discusion what is bug and what s not in this thread. <br /><br />In the
firstmessage Martin asked <br />"There are probably a lot of details that I have overlooked. I'd be really<br />
thankfulfor some constructive comments and criticism. Especially, what needs<br /> to be done to have this in the core.
 Feedbackappreciated."<br /><br />Can we get back to the topic?<br /><br />PS: We have 10000+ functions (including lots
ofduplicates)<br />PS: We are able to be as arrogant as any of you but we can get more things done with constructive
comments.<br /><br /><br /><div class="gmail_quote">On Wed, Aug 20, 2008 at 2:53 AM, Andrew Dunstan <span
dir="ltr"><<ahref="mailto:andrew@dunslane.net">andrew@dunslane.net</a>></span> wrote:<br /><blockquote
class="gmail_quote"style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left:
1ex;"><divclass="Ih2E3d"><br /><br /> Tom Lane wrote:<br /><blockquote class="gmail_quote" style="border-left: 1px
solidrgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;"> Also, there are a whole lot more
considerationsin a backpatch decision<br /> than just "is it a bug".  The (estimated) risk of creating new bugs and<br
/>the extent to which the patch will change behavior that apps might be<br /> relying on are two big reasons why we
mightchoose not to back-patch<br /> a bug fix.<br /><br /><br />  <br /></blockquote><br /></div> Right. And even if it
isa bug the question might be "what sort of bug is it?" We might well be prepared to take some risks with code
stabilityto plug security or data corruption bugs, a lot more than we would for other sorts of bugs. Even if this were
considereda bug instead of a limitation, it doesn't come into the class of things we should be rushing to fix in the
stablebranches, unless the fix is fairly obvious and of limited impact, which is clearly not the case.<br /><br />
cheers<br/><font color="#888888"><br /> andrew<br /></font></blockquote></div><br /></div> 

On Tue, Aug 19, 2008 at 09:50:53PM -0400, Tom Lane wrote:
> David Fetter <david@fetter.org> writes:
> > On Tue, Aug 19, 2008 at 07:45:16PM -0400, Tom Lane wrote:
> >> FWIW, given that there will probably always be corner cases. I can
> >> see the attraction in Simon's suggestion of providing a way to
> >> manually issue a system-wide forced plan flush.
> 
> > Would that require a system-wide plan cache to implement?
> 
> No, just a function that can issue a suitable sinval message.
> 
> plancache.c would already respond in the desired way to a relcache inval
> message with OID = 0, though likely it'll be cleaner to invent an sinval
> message type specifically for the purpose.
> 
> One thing to think about is whether the flush should be truly
> system-wide or just database-wide.  I can see a lot more uses for the
> latter than the former --- I don't think there's a reason for cached
> plans to depend on any contents of the shared catalogs.

They might during an on-line upgrade.

Zdenek?

Cheers,
David.
-- 
David Fetter <david@fetter.org> http://fetter.org/
Phone: +1 415 235 3778  AIM: dfetter666  Yahoo!: dfetter
Skype: davidfetter      XMPP: david.fetter@gmail.com

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

Am 19.08.2008 um 20:47 schrieb Tom Lane:

> Bruce Momjian <bruce@momjian.us> writes:
>> Joshua Drake wrote:
>>> Is our backpatch policy documented? It does not appear to be in
>>> developer FAQ.
>
>> Seems we need to add it.
>
> I'm not sure that I *want* a formal written-down backpatch policy.
> Whether (and how far) to backpatch has always been a best-judgment  
> call
> in the past, and we've gotten along fine with that.  I think having a
> formal policy is just likely to lead to even more complaints: either
> patching or not patching could result in second-guessing by someone
> who feels he can construe the policy to match the result he prefers.

Agreeing to you and some later posters in this thread, I would not  
vote for a formal policy either. But IMHO there should be a general,  
informal note about backpatching in developer docs/faqs. A place where  
you can point to, and a chance for new people to read about the  
postgres way of handling backpatching.

Btw., how backpatching is handled here is one of the reasons I trust  
my data to postgres.

Best Regards
Michael

Le mercredi 20 août 2008, Tom Lane a écrit :
> That just begs the question of what's the difference between a "bug" and
> a "limitation".  AFAICS, having such a policy/guideline/whatchacallit
> in place wouldn't have done a single thing to stop the current flamewar,
> because the people who want this thing back-patched are insisting that
> it's a bug, while those who don't are saying it's a long-known
> limitation.

As a person who previously insisted it was a bug, I'd like to take the
opportunity to claim that I didn't realize this was a limitation of the
design of plan invalidation, which now seems related to DDL operations.
Realizing this earlier would have resulted in no mail at all on this thread
from here.

There's certainly a balance between -hackers readers not doing their homework
and people in the know choosing not to re-estate known things...

> Also, there are a whole lot more considerations in a backpatch decision
> than just "is it a bug".  The (estimated) risk of creating new bugs and
> the extent to which the patch will change behavior that apps might be
> relying on are two big reasons why we might choose not to back-patch
> a bug fix.

And this way the project works is what leads its users not to fear minor
upgrades, which is something I (we all?) highly value.

Regards,
--
dim

On Tue, 2008-08-19 at 19:45 -0400, Tom Lane wrote:
> Hannu Krosing <hannu@2ndQuadrant.com> writes:
> > If there is plan invalidation then you just change called1() to return
> > one more field and that's it - no juggling with C) and D) and generally
> > less things that can go wrong.
> 
> That is a pure flight of fancy.  

Nope, this is description of real situation when you have to maintain
lots and lots of functions.

> Adjusting a function's API generally
> requires source-code changes on the caller side too.  

Adding a column to table does not (even generally) require changing all
queries accessing that table, why should adding a column to functions
return type do ?

> There might be
> a few limited cases where you can avoid that, but that doesn't leave
> you with much of an argument that this is a critical bug fix.  It's
> a corner case and little more.

It is a corner case if you don't have a dynamic system, evolving over
time, which relies heavily on functions .

It is a complete non-issue if you don't use functions at all.

> FWIW, given that there will probably always be corner cases. I can see
> the attraction in Simon's suggestion of providing a way to manually
> issue a system-wide forced plan flush.

That was also what I suggested as one blanket way of solving the bigger
issue you brought up, that of not knowing where to stop tracking
dependencies for plan invalidation.

My thinking was, that this trades one-time inefficiency (replanning all
stored plans) against more general but spread in time inefficiency of
current patch (sending registration messages around for each function
OID you depend on at each time you plan ).

------------
Hannu

<div dir="ltr">The lack of plan invalidation is limitation that also has two bugs attached to it.<br />I agree that
fullfledged patch to fix all the isssues should not be done in 8.3. <br />I can't agree that effort to get the bugs
fixedalready in 8.3 should not be made.<br /> I can understand that hackers here have learned to live with these bugs
andlimitations but not all the users are reading these flame wars here and most of them are not even aware of these
bugsuntil they are hit by them. <br /><br />Sql function bug is such that users probably won't even understand what hit
themand how the data got mangled.<br />- If there is nothing that can be done in 8.3 at least warning should be added
intothe documentation.  It will be just one more don't in our long list don'ts for our developers.<br /><br />ERROR: 
cachelookup failed for function. <br />- Could the plan be marked as invalid so it would fail only once so the next
callto the function would get replanned and work again. At least it would be better than losing parts of application
forindeterminate time. <br /> - Should update pg_proc set proname = proname; be the current solution to the problem or
hassomeone something better to offer. We could scan released code for DROP FUNCTION and generate plan invalidation
statementas last item of transaction releasing the code.<br /> - Could some less dangerous looking mechanism be added
to8.3 that wouldn't make users not used to PostgreSQL limitations gasp for air when they see the workarounds :)<br
/>Callingthe problem limitation will not make it go away. I am quite sure that new users consider it a bug until thay
areconverted to perceive it as lmitation.<br /><br />No matter how many time the usage of functions in database is
calledcorner case it does not make it a corner case. In my experience it is quite common practice on all the database
systemsi have worked with. I do get the impression that Tom who would prefer to get all the pl's out of PostgreSQL and
livehappily ever after with pure SQL standard. <br /><br /><div class="gmail_quote">On Wed, Aug 20, 2008 at 11:27 AM,
DimitriFontaine <span dir="ltr"><<a href="mailto:dfontaine@hi-media.com">dfontaine@hi-media.com</a>></span>
wrote:<br/><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex;
padding-left:1ex;"> Le mercredi 20 août 2008, Tom Lane a écrit :<br /><div class="Ih2E3d">> That just begs the
questionof what's the difference between a "bug" and<br /> > a "limitation".  AFAICS, having such a
policy/guideline/whatchacallit<br/> > in place wouldn't have done a single thing to stop the current flamewar,<br />
>because the people who want this thing back-patched are insisting that<br /> > it's a bug, while those who don't
aresaying it's a long-known<br /> > limitation.<br /><br /></div>As a person who previously insisted it was a bug,
I'dlike to take the<br /> opportunity to claim that I didn't realize this was a limitation of the<br /> design of plan
invalidation,which now seems related to DDL operations.<br /> Realizing this earlier would have resulted in no mail at
allon this thread<br /> from here.<br /><br /> There's certainly a balance between -hackers readers not doing their
homework<br/> and people in the know choosing not to re-estate known things...<br /><div class="Ih2E3d"><br /> >
Also,there are a whole lot more considerations in a backpatch decision<br /> > than just "is it a bug".  The
(estimated)risk of creating new bugs and<br /> > the extent to which the patch will change behavior that apps might
be<br/> > relying on are two big reasons why we might choose not to back-patch<br /> > a bug fix.<br /><br
/></div>Andthis way the project works is what leads its users not to fear minor<br /> upgrades, which is something I
(weall?) highly value.<br /><br /> Regards,<br /><font color="#888888">--<br /> dim<br /></font></blockquote></div><br
/></div>

On Wed, Aug 20, 2008 at 03:12:43PM +0300, Asko Oja wrote:

> - If there is nothing that can be done in 8.3 at least warning should be
> added into the documentation.  It will be just one more don't in our long
> list don'ts for our developers.

I am in favour of that change in the 8.3 branch.

> 
> ERROR:  cache lookup failed for function.
> - Could the plan be marked as invalid so it would fail only once so the next
> call to the function would get replanned and work again. At least it would
> be better than losing parts of application for indeterminate time.

That seems to me to be a behaviour change, not a bug fix.  I agree
that the current behaviour is pretty annoying.  That is not the same
thing as "a bug" except in the loosest sense.  The system works as
specified, and therefore it's not a bug.  If the specification is
wrong, you need a new specification; that's a "bug fix" that is
usually pronounced "major release".

> - Could some less dangerous looking mechanism be added to 8.3 that wouldn't
> make users not used to PostgreSQL limitations gasp for air when they see the
> workarounds :)

I think it a very bad idea even to suggest that we start undertaking
things like adding mechanisms to minor releases, even with smileys at
the end of the sentence.  I appreciate (possibly more than many
hackers) the limitations that are imposed on users by some of the
decisions historically taken by developers in some of the previous
major releases.  But I very strongly agree with Dimitri: the
super-conservative approach to maintenance releases that this project
takes is a really big benefit to users, and is ultra important in
"mission critical" environments.  Otherwise, it becomes practically
impossible to get minor releases into production.  If you have to
worry about the possibility of major changes between minor versions,
you will have to treat every release as a major release.

I don't think we have sufficient commercial integration support yet
that we can follow the lead of the Linux kernel, where the system
vendor has the effective obligation to make sure your kernel actually
works.  

In addition, if someone wants to develop back-patches for 8.3 that
give it new functionality otherwise planned for 8.4, I see nothing
wrong with them doing so.  That's the advantage offered by having the
source.  But the idea that the new functionality should be patched
back by the project because one is impatient is not on.

A

-- 
Andrew Sullivan
ajs@commandprompt.com
+1 503 667 4564 x104
http://www.commandprompt.com/

On Wed, Aug 20, 2008 at 09:16:56AM -0400, Andrew Sullivan wrote:
> On Wed, Aug 20, 2008 at 03:12:43PM +0300, Asko Oja wrote:
> 
> > - If there is nothing that can be done in 8.3 at least warning should be
> > added into the documentation.  It will be just one more don't in our long
> > list don'ts for our developers.
> 
> I am in favour of that change in the 8.3 branch.
+1

> 
> > 
> > ERROR:  cache lookup failed for function.
> > - Could the plan be marked as invalid so it would fail only once so the next
> > call to the function would get replanned and work again. At least it would
> > be better than losing parts of application for indeterminate time.
> 
> That seems to me to be a behaviour change, not a bug fix.  I agree
> that the current behaviour is pretty annoying.  That is not the same
> thing as "a bug" except in the loosest sense.  The system works as
> specified, and therefore it's not a bug.  If the specification is
> wrong, you need a new specification; that's a "bug fix" that is
> usually pronounced "major release".
> 
> > - Could some less dangerous looking mechanism be added to 8.3 that wouldn't
> > make users not used to PostgreSQL limitations gasp for air when they see the
> > workarounds :)
> 
> I think it a very bad idea even to suggest that we start undertaking
> things like adding mechanisms to minor releases, even with smileys at
> the end of the sentence.  I appreciate (possibly more than many
> hackers) the limitations that are imposed on users by some of the
> decisions historically taken by developers in some of the previous
> major releases.  But I very strongly agree with Dimitri: the
> super-conservative approach to maintenance releases that this project
> takes is a really big benefit to users, and is ultra important in
> "mission critical" environments.  Otherwise, it becomes practically
> impossible to get minor releases into production.  If you have to
> worry about the possibility of major changes between minor versions,
> you will have to treat every release as a major release.
> 
+10

This policy has allowed us to upgrade to new minor releases with a
minimum of testing for critical systems and basically none for non-
critical systems. We would never upgrade for minor releases if this
changes. We do not have the resources to perform full regression
tests without having a very big carrot such as the new features a
major release contains.

Cheers,
Ken

Asko Oja wrote:
> I do get the impression that Tom who would prefer to get all the pl's 
> out of PostgreSQL and live happily ever after with pure SQL standard.
>
>

I have not seen the slightest evidence of this, and don't believe it for 
a minute.

I understand some of the frustration you are feeling, but statements 
like this don't help anything.

(And yes, I too have recently been bitten nastily by cached plan 
problems, and want to see them fixed. I rather like Simon's suggestion 
of a command or function that would clear the plan cache.)

cheers

andrew

Asko Oja escribió:

> In the first message Martin asked
> "There are probably a lot of details that I have overlooked. I'd be really
> thankful for some constructive comments and criticism. Especially, what
> needs
> to be done to have this in the core.  Feedback appreciated."
> 
> Can we get back to the topic?

This is where the interesting questions are:

http://archives.postgresql.org/message-id/10333.1219179364%40sss.pgh.pa.us

I think the efforts to get the patch in 8.3 are wasted time.  Better
concentrate on getting something good for everyone in 8.4.

-- 
Alvaro Herrera                                http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

Alvaro Herrera <alvherre@commandprompt.com> writes:
> This is where the interesting questions are:
> http://archives.postgresql.org/message-id/10333.1219179364%40sss.pgh.pa.us

Upthread, someone speculated about solving the problem by forcing plan
cache flush on *any* catalog change.  I think that's probably not
acceptable from an efficiency standpoint.  But maybe it'd be a good idea
to special-case common cases and fall back to a stupid flush for less
common cases, rather than invest all the work that'd be needed to track
every direct and indirect dependency of every plan.  My first thought
along these lines is:

* track table dependencies exactly (important for efficiency, plus we've got the code already)

* track function dependencies exactly (seems function definitions might change often enough to make it important for
efficiency;maybe only track PL function dependencies??)
 

* brute-force flush for any other catalog change that could affect plans

However I have no hard evidence to back up drawing the line there rather
than somewhere else.  Anyone have data on what sort of DDL changes are
common in their applications?
        regards, tom lane

David Fetter napsal(a):
> On Tue, Aug 19, 2008 at 09:50:53PM -0400, Tom Lane wrote:
>> David Fetter <david@fetter.org> writes:
>>> On Tue, Aug 19, 2008 at 07:45:16PM -0400, Tom Lane wrote:
>>>> FWIW, given that there will probably always be corner cases. I can
>>>> see the attraction in Simon's suggestion of providing a way to
>>>> manually issue a system-wide forced plan flush.
>>> Would that require a system-wide plan cache to implement?
>> No, just a function that can issue a suitable sinval message.
>>
>> plancache.c would already respond in the desired way to a relcache inval
>> message with OID = 0, though likely it'll be cleaner to invent an sinval
>> message type specifically for the purpose.
>>
>> One thing to think about is whether the flush should be truly
>> system-wide or just database-wide.  I can see a lot more uses for the
>> latter than the former --- I don't think there's a reason for cached
>> plans to depend on any contents of the shared catalogs.
> 
> They might during an on-line upgrade.
> 

At this moment we have offline catalog upgrade. On-line old catalog 
processing is nice idea but amount of work and impact is too high to do 
it. Catalog is usually small and its offline upgrade is fast.
    Zdenek

On Wed, 2008-08-20 at 08:50 -0400, Andrew Dunstan wrote:
> 
> Asko Oja wrote:
> > I do get the impression that Tom who would prefer to get all the pl's 
> > out of PostgreSQL and live happily ever after with pure SQL standard.
> >
> >
> 
> I have not seen the slightest evidence of this, and don't believe it for 
> a minute.
>
> I understand some of the frustration you are feeling, but statements 
> like this don't help anything.

Claiming that problems with functions are a "corner case" seems to
indicate that kind of attitude.

OTOH, it may still be, that building really large and complex live
(evolving) databases using postgreSQL is also still a "corner case", so
any bug/limitation that manifests itself when doing DDL under 24/7
database carrying big loads is a "corner case"

> (And yes, I too have recently been bitten nastily by cached plan 
> problems, and want to see them fixed. I rather like Simon's suggestion 
> of a command or function that would clear the plan cache.)

I guess this would be more robust.

Mostly we use _dependencies_ to forbid stuff or to do DROP CASCADE, that
is, to enforce user-visible behaviour.

Cache invalidation seems much lighter and safer operations.

We could even add an option to do a global cache invalidation at the end
of any transaction which does DDL. That would of course need automatic
re-planning the invalidated queries and keeping some intermediate form
of query (with original * expanded to col lists, maybe something else,
basically the same as is currently saved for view's) in order to do so.

-----
Hannu

On Wed, Aug 20, 2008 at 05:03:19PM +0300, Asko Oja wrote:
> 
> Lets get on with 8.4

Oh, I shoulda mentioned that, too -- I completely support doing this
work for 8.4.  (I can think of more than one case where this feature
alone would be worth the upgrade.)

A

-- 
Andrew Sullivan
ajs@commandprompt.com
+1 503 667 4564 x104
http://www.commandprompt.com/

On Aug 20, 2008, at 9:18 AM, Tom Lane wrote:
> However I have no hard evidence to back up drawing the line there  
> rather
> than somewhere else.  Anyone have data on what sort of DDL changes are
> common in their applications?


I've worked in environments where we used stored functions  
extensively and where we didn't. Table DDL is generally fairly common  
in both cases, and if stored functions or views are used, it's very  
common for table DDL to trigger updates in views and functions. It's  
fairly common to have to update just functions to kill bugs or change  
functionality. Trigger changes are a bit less frequent, and views are  
probably the least frequent.
-- 
Decibel!, aka Jim C. Nasby, Database Architect  decibel@decibel.org
Give your computer some brain candy! www.distributed.net Team #1828

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;

Martin Pihlak <martin.pihlak@gmail.com> writes:
> 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.

Applied after rather heavy revision.  Aside from the gripes I had
yesterday, I found out on closer inspection that the patch did things
all wrong for the case of a not-fully-planned cache item.  I ended up
discarding the existing code for that and instead using the planner
machinery to extract dependencies of a parsed querytree.
        regards, tom lane