From c68489863c91a23821c4fcbfd9cfb5bce3220e8f Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Fri, 4 Aug 2006 21:33:36 +0000 Subject: Fix domain_in() bug exhibited by Darcy Buskermolen. The idea of an EState 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/utils/adt/domains.c | 59 +++++++++++++++++++++++------------------ 1 file changed, 33 insertions(+), 26 deletions(-) (limited to 'src/backend/utils/adt/domains.c') diff --git a/src/backend/utils/adt/domains.c b/src/backend/utils/adt/domains.c index 7c13175e45..2126c6b87b 100644 --- a/src/backend/utils/adt/domains.c +++ b/src/backend/utils/adt/domains.c @@ -11,17 +11,13 @@ * * 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); } -- cgit v1.2.1