Fix domain_in() bug exhibited by Darcy Buskermolen. The idea of an EState
authorTom Lane
Fri, 4 Aug 2006 21:33:36 +0000 (21:33 +0000)
committerTom Lane
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 */