Fix domain_in() bug exhibited by Darcy Buskermolen. The idea of an EState
authorTom Lane <tgl@sss.pgh.pa.us>
Fri, 4 Aug 2006 21:33:36 +0000 (21:33 +0000)
committerTom Lane <tgl@sss.pgh.pa.us>
Fri, 4 Aug 2006 21:33:36 +0000 (21:33 +0000)
that's shorter-lived than the expression state being evaluated in it really
doesn't work :-( --- we end up with fn_extra caches getting deleted while
still in use.  Rather than abandon the notion of caching expression state
across domain_in calls altogether, I chose to make domain_in a bit cozier
with ExprContext.  All we really need for evaluating variable-free
expressions is an ExprContext, not an EState, so I invented the notion of a
"standalone" ExprContext.  domain_in can prevent resource leakages by doing
a ReScanExprContext on this rather than having to free it entirely; so we
can make the ExprContext have the same lifespan (and particularly the same
per_query memory context) as the expression state structs.

src/backend/executor/execUtils.c
src/backend/utils/adt/domains.c
src/include/executor/executor.h
src/include/nodes/execnodes.h

index c879de3bbc173f3a8cdcb839836a6aca3b825a5b..aa5aeb57f3644c1a4f508baa5da0b65bd13c5dc4 100644 (file)
@@ -8,7 +8,7 @@
  *
  *
  * IDENTIFICATION
- *       $PostgreSQL: pgsql/src/backend/executor/execUtils.c,v 1.138 2006/07/31 20:09:04 tgl Exp $
+ *       $PostgreSQL: pgsql/src/backend/executor/execUtils.c,v 1.139 2006/08/04 21:33:36 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -17,6 +17,7 @@
  *             CreateExecutorState             Create/delete executor working state
  *             FreeExecutorState
  *             CreateExprContext
+ *             CreateStandaloneExprContext
  *             FreeExprContext
  *             ReScanExprContext
  *
@@ -331,6 +332,68 @@ CreateExprContext(EState *estate)
        return econtext;
 }
 
+/* ----------------
+ *             CreateStandaloneExprContext
+ *
+ *             Create a context for standalone expression evaluation.
+ *
+ * An ExprContext made this way can be used for evaluation of expressions
+ * that contain no Params, subplans, or Var references (it might work to
+ * put tuple references into the scantuple field, but it seems unwise).
+ *
+ * The ExprContext struct is allocated in the caller's current memory
+ * context, which also becomes its "per query" context.
+ *
+ * It is caller's responsibility to free the ExprContext when done,
+ * or at least ensure that any shutdown callbacks have been called
+ * (ReScanExprContext() is suitable).  Otherwise, non-memory resources
+ * might be leaked.
+ * ----------------
+ */
+ExprContext *
+CreateStandaloneExprContext(void)
+{
+       ExprContext *econtext;
+
+       /* Create the ExprContext node within the caller's memory context */
+       econtext = makeNode(ExprContext);
+
+       /* Initialize fields of ExprContext */
+       econtext->ecxt_scantuple = NULL;
+       econtext->ecxt_innertuple = NULL;
+       econtext->ecxt_outertuple = NULL;
+
+       econtext->ecxt_per_query_memory = CurrentMemoryContext;
+
+       /*
+        * Create working memory for expression evaluation in this context.
+        */
+       econtext->ecxt_per_tuple_memory =
+               AllocSetContextCreate(CurrentMemoryContext,
+                                                         "ExprContext",
+                                                         ALLOCSET_DEFAULT_MINSIZE,
+                                                         ALLOCSET_DEFAULT_INITSIZE,
+                                                         ALLOCSET_DEFAULT_MAXSIZE);
+
+       econtext->ecxt_param_exec_vals = NULL;
+       econtext->ecxt_param_list_info = NULL;
+
+       econtext->ecxt_aggvalues = NULL;
+       econtext->ecxt_aggnulls = NULL;
+
+       econtext->caseValue_datum = (Datum) 0;
+       econtext->caseValue_isNull = true;
+
+       econtext->domainValue_datum = (Datum) 0;
+       econtext->domainValue_isNull = true;
+
+       econtext->ecxt_estate = NULL;
+
+       econtext->ecxt_callbacks = NULL;
+
+       return econtext;
+}
+
 /* ----------------
  *             FreeExprContext
  *
@@ -352,9 +415,11 @@ FreeExprContext(ExprContext *econtext)
        ShutdownExprContext(econtext);
        /* And clean up the memory used */
        MemoryContextDelete(econtext->ecxt_per_tuple_memory);
-       /* Unlink self from owning EState */
+       /* Unlink self from owning EState, if any */
        estate = econtext->ecxt_estate;
-       estate->es_exprcontexts = list_delete_ptr(estate->es_exprcontexts, econtext);
+       if (estate)
+               estate->es_exprcontexts = list_delete_ptr(estate->es_exprcontexts,
+                                                                                                 econtext);
        /* And delete the ExprContext node */
        pfree(econtext);
 }
index 7c13175e45276762159f240f0dd543a75f116d5a..2126c6b87b2ede32c3e4bfff75749fd4a47a58ea 100644 (file)
  *
  * The overhead required for constraint checking can be high, since examining
  * the catalogs to discover the constraints for a given domain is not cheap.
- * We have two mechanisms for minimizing this cost:
+ * We have three mechanisms for minimizing this cost:
  *     1.      In a nest of domains, we flatten the checking of all the levels
  *             into just one operation.
  *     2.      We cache the list of constraint items in the FmgrInfo struct
  *             passed by the caller.
- *
- * We also have to create an EState to evaluate CHECK expressions in.
- * Creating and destroying an EState is somewhat expensive, and so it's
- * tempting to cache the EState too.  However, that would mean that the
- * EState never gets an explicit FreeExecutorState call, which is a bad idea
- * because it risks leaking non-memory resources.
+ *     3.      If there are CHECK constraints, we cache a standalone ExprContext
+ *             to evaluate them in.
  *
  *
  * Portions Copyright (c) 1996-2006, PostgreSQL Global Development Group
@@ -29,7 +25,7 @@
  *
  *
  * IDENTIFICATION
- *       $PostgreSQL: pgsql/src/backend/utils/adt/domains.c,v 1.2 2006/07/14 14:52:24 momjian Exp $
+ *       $PostgreSQL: pgsql/src/backend/utils/adt/domains.c,v 1.3 2006/08/04 21:33:36 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -54,6 +50,10 @@ typedef struct DomainIOData
        FmgrInfo        proc;
        /* List of constraint items to check */
        List       *constraint_list;
+       /* Context for evaluating CHECK constraints in */
+       ExprContext *econtext;
+       /* Memory context this cache is in */
+       MemoryContext mcxt;
 } DomainIOData;
 
 
@@ -95,6 +95,10 @@ domain_state_setup(DomainIOData *my_extra, Oid domainType, bool binary,
        my_extra->constraint_list = GetDomainConstraints(domainType);
        MemoryContextSwitchTo(oldcontext);
 
+       /* We don't make an ExprContext until needed */
+       my_extra->econtext = NULL;
+       my_extra->mcxt = mcxt;
+
        /* Mark cache valid */
        my_extra->domain_type = domainType;
 }
@@ -107,7 +111,7 @@ domain_state_setup(DomainIOData *my_extra, Oid domainType, bool binary,
 static void
 domain_check_input(Datum value, bool isnull, DomainIOData *my_extra)
 {
-       EState     *estate = NULL;
+       ExprContext *econtext = my_extra->econtext;
        ListCell   *l;
 
        foreach(l, my_extra->constraint_list)
@@ -125,25 +129,26 @@ domain_check_input(Datum value, bool isnull, DomainIOData *my_extra)
                                break;
                        case DOM_CONSTRAINT_CHECK:
                                {
-                                       ExprContext *econtext;
                                        Datum           conResult;
                                        bool            conIsNull;
-                                       Datum           save_datum;
-                                       bool            save_isNull;
 
-                                       if (estate == NULL)
-                                               estate = CreateExecutorState();
-                                       econtext = GetPerTupleExprContext(estate);
+                                       /* Make the econtext if we didn't already */
+                                       if (econtext == NULL)
+                                       {
+                                               MemoryContext oldcontext;
+
+                                               oldcontext = MemoryContextSwitchTo(my_extra->mcxt);
+                                               econtext = CreateStandaloneExprContext();
+                                               MemoryContextSwitchTo(oldcontext);
+                                               my_extra->econtext = econtext;
+                                       }
 
                                        /*
                                         * Set up value to be returned by CoerceToDomainValue
-                                        * nodes. We must save and restore prior setting of
-                                        * econtext's domainValue fields, in case this node is
-                                        * itself within a check expression for another domain.
+                                        * nodes.  Unlike ExecEvalCoerceToDomain, this econtext
+                                        * couldn't be shared with anything else, so no need
+                                        * to save and restore fields.
                                         */
-                                       save_datum = econtext->domainValue_datum;
-                                       save_isNull = econtext->domainValue_isNull;
-
                                        econtext->domainValue_datum = value;
                                        econtext->domainValue_isNull = isnull;
 
@@ -158,9 +163,6 @@ domain_check_input(Datum value, bool isnull, DomainIOData *my_extra)
                                                                 errmsg("value for domain %s violates check constraint \"%s\"",
                                                                                format_type_be(my_extra->domain_type),
                                                                                con->name)));
-                                       econtext->domainValue_datum = save_datum;
-                                       econtext->domainValue_isNull = save_isNull;
-
                                        break;
                                }
                        default:
@@ -170,8 +172,13 @@ domain_check_input(Datum value, bool isnull, DomainIOData *my_extra)
                }
        }
 
-       if (estate)
-               FreeExecutorState(estate);
+       /*
+        * Before exiting, call any shutdown callbacks and reset econtext's
+        * per-tuple memory.  This avoids leaking non-memory resources,
+        * if anything in the expression(s) has any.
+        */
+       if (econtext)
+               ReScanExprContext(econtext);
 }
 
 
index d926d8b5c5c5249f0a863da90ee98bf171111cef..b2baec841bc8c9dd59b9ee0c60343614ed712888 100644 (file)
@@ -7,7 +7,7 @@
  * Portions Copyright (c) 1996-2006, PostgreSQL Global Development Group
  * Portions Copyright (c) 1994, Regents of the University of California
  *
- * $PostgreSQL: pgsql/src/include/executor/executor.h,v 1.127 2006/06/16 18:42:23 tgl Exp $
+ * $PostgreSQL: pgsql/src/include/executor/executor.h,v 1.128 2006/08/04 21:33:36 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -224,6 +224,7 @@ extern void end_tup_output(TupOutputState *tstate);
 extern EState *CreateExecutorState(void);
 extern void FreeExecutorState(EState *estate);
 extern ExprContext *CreateExprContext(EState *estate);
+extern ExprContext *CreateStandaloneExprContext(void);
 extern void FreeExprContext(ExprContext *econtext);
 extern void ReScanExprContext(ExprContext *econtext);
 
index fbf70f5152622530f5cd5fb5a43a186aed320b54..30b3fce41653b105c0dd01692e84c50cb095d051 100644 (file)
@@ -7,7 +7,7 @@
  * Portions Copyright (c) 1996-2006, PostgreSQL Global Development Group
  * Portions Copyright (c) 1994, Regents of the University of California
  *
- * $PostgreSQL: pgsql/src/include/nodes/execnodes.h,v 1.157 2006/08/02 18:58:21 tgl Exp $
+ * $PostgreSQL: pgsql/src/include/nodes/execnodes.h,v 1.158 2006/08/04 21:33:36 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -118,7 +118,7 @@ typedef struct ExprContext
        Datum           domainValue_datum;
        bool            domainValue_isNull;
 
-       /* Link to containing EState */
+       /* Link to containing EState (NULL if a standalone ExprContext) */
        struct EState *ecxt_estate;
 
        /* Functions to call back when ExprContext is shut down */