8000 Avoid caching expression state trees for domain constraints across qu… · mathiasrw/postgres@ec7eef6 · GitHub
[go: up one dir, main page]
Skip to content

Commit ec7eef6

Browse files
tglsfdctglsfdc
committed
Avoid caching expression state trees for domain constraints across queries.
In commit 8abb3cd I attempted to cache the expression state trees constructed for domain CHECK constraints for the life of the backend (assuming the domain's constraints don't get redefined). However, this turns out not to work very well, because execQual.c will run those state trees with ecxt_per_query_memory pointing to a query-lifespan context, and in some situations we'll end up with pointers into that context getting stored into the state trees. This happens in particular with SQL-language functions, as reported by Emre Hasegeli, but there are many other cases. To fix, keep only the expression plan trees for domain CHECK constraints in the typcache's data structure, and revert to performing ExecInitExpr (at least) once per query to set up expression state trees in the query's context. Eventually it'd be nice to undo this, but that will require some careful thought about memory management for expression state trees, and it seems far too late for any such redesign in 9.5. This way is still much more efficient than what happened before 8abb3cd.
1 parent 8d32717 commit ec7eef6

File tree

4 files changed

+108
-4
lines changed

4 files changed

+108
-4
lines changed

‎src/backend/utils/cache/typcache.c

Lines changed: 58 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -94,6 +94,13 @@ static TypeCacheEntry *firstDomainTypeEntry = NULL;
9494
* Data stored about a domain type's constraints. Note that we do not create
9595
* this struct for the common case of a constraint-less domain; we just set
9696
* domainData to NULL to indicate that.
97+
*
98+
* Within a DomainConstraintCache, we abuse the DomainConstraintState node
99+
* type a bit: check_expr fields point to expression plan trees, not plan
100+
* state trees. When needed, expression state trees are built by flat-copying
101+
* the DomainConstraintState nodes and applying ExecInitExpr to check_expr.
102+
* Such a state tree is not part of the DomainConstraintCache, but is
103+
* considered to belong to a DomainConstraintRef.
97104
*/
98105
struct DomainConstraintCache
99106
{
@@ -152,6 +159,7 @@ static void load_domaintype_info(TypeCacheEntry *typentry);
152159
static int dcs_cmp(const void *a, const void *b);
153160
static void decr_dcc_refcount(DomainConstraintCache *dcc);
154161
static void dccref_deletion_callback(void *arg);
162+
static List *prep_domain_constraints(List *constraints, MemoryContext execctx);
155163
static bool array_element_has_equality(TypeCacheEntry *typentry);
156164
static bool array_element_has_compare(TypeCacheEntry *typentry);
157165
static bool array_element_has_hashing(TypeCacheEntry *typentry);
@@ -762,13 +770,14 @@ load_domaintype_info(TypeCacheEntry *typentry)
762770

763771
check_expr = (Expr *) stringToNode(constring);
764772

765-
/* ExecInitExpr assumes we've planned the expression */
773+
/* ExecInitExpr will assume we've planned the expression */
766774
check_expr = expression_planner(check_expr);
767775

768776
r = makeNode(DomainConstraintState);
769777
r->constrainttype = DOM_CONSTRAINT_CHECK;
770778
r->name = pstrdup(NameStr(c->conname));
771-
r->check_expr = ExecInitExpr(check_expr, NULL);
779+
/* Must cast here because we're not storing an expr state node */
780+
r->check_expr = (ExprState *) check_expr;
772781

773782
MemoryContextSwitchTo(oldcxt);
774783

@@ -913,6 +922,40 @@ dccref_deletion_callback(void *arg)
913922
}
914923
}
915924

925+
/*
926+
* prep_domain_constraints --- prepare domain constraints for execution
927+
*
928+
* The expression trees stored in the DomainConstraintCache's list are
929+
* converted to executable expression state trees stored in execctx.
930+
*/
931+
static List *
932+
prep_domain_constraints(List *constraints, MemoryContext execctx)
933+
{
934+
List *result = NIL;
935+
MemoryContext oldcxt;
936+
ListCell *lc;
937+
938+
oldcxt = MemoryContextSwitchTo(execctx);
939+
940+
foreach(lc, constraints)
941+
{
942+
DomainConstraintState *r = (DomainConstraintState *) lfirst(lc);
943+
DomainConstraintState *newr;
944+
945+
newr = makeNode(DomainConstraintState);
946+
newr->constrainttype = r->constrainttype;
947+
newr->name = r->name;
948+
/* Must cast here because cache items contain expr plan trees */
949+
newr->check_expr = ExecInitExpr((Expr *) r->check_expr, NULL);
950+
951+
result = lappend(result, newr);
952+
}
953+
954+
MemoryContextSwitchTo(oldcxt);
955+
956+
return result;
957+
}
958+
916959
/*
917960
* InitDomainConstraintRef --- initialize a DomainConstraintRef struct
918961
*
@@ -926,6 +969,7 @@ InitDomainConstraintRef(Oid type_id, DomainConstraintRef *ref,
926969
/* Look up the typcache entry --- we assume it survives indefinitely */
927970
ref->tcache = lookup_type_cache(type_id, TYPECACHE_DOMAIN_INFO);
928971
/* For safety, establish the callback before acquiring a refcount */
972+
ref->refctx = refctx;
929973
ref->dcc = NULL;
930974
ref->callback.func = dccref_deletion_callback;
931975
ref->callback.arg = (void *) ref;
@@ -935,7 +979,8 @@ InitDomainConstraintRef(Oid type_id, DomainConstraintRef *ref,
935979
{
936980
ref->dcc = ref->tcache->domainData;
937981
ref->dcc->dccRefCount++;
938-
ref->constraints = ref->dcc->constraints;
982+
ref->constraints = prep_domain_constraints(ref->dcc->constraints,
983+
ref->refctx);
939984
}
940985
else
941986
ref->constraints = NIL;
@@ -969,6 +1014,14 @@ UpdateDomainConstraintRef(DomainConstraintRef *ref)
9691014

9701015
if (dcc)
9711016
{
1017+
/*
1018+
* Note: we just leak the previous list of executable domain
1019+
* constraints. Alternatively, we could keep those in a child
1020+
* context of ref->refctx and free that context at this point.
1021+
* However, in practice this code path will be taken so seldom
1022+
* that the extra bookkeeping for a child context doesn't seem
1023+
* worthwhile; we'll just allow a leak for the lifespan of refctx.
1024+
*/
9721025
ref->constraints = NIL;
9731026
ref->dcc = NULL;
9741027
decr_dcc_refcount(dcc);
@@ -978,7 +1031,8 @@ UpdateDomainConstraintRef(DomainConstraintRef *ref)
9781031
{
9791032
ref->dcc = dcc;
9801033
dcc->dccRefCount++;
981-
ref->constraints = dcc->constraints;
1034+
ref->constraints = prep_domain_constraints(dcc->constraints,
1035+
ref->refctx);
9821036
}
9831037
}
9841038
}

‎src/include/utils/typcache.h

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -130,6 +130,7 @@ typedef struct TypeCacheEntry
130130
typedef struct DomainConstraintRef
131131
{
132132
List *constraints; /* list of DomainConstraintState nodes */
133+
MemoryContext refctx; /* context holding DomainConstraintRef */
133134

134135
/* Management data --- treat these fields as private to typcache.c */
135136
TypeCacheEntry *tcache; /* owning typcache entry */

‎src/test/regress/expected/domain.out

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -682,6 +682,31 @@ select dom_check(0);
682682
drop function dom_check(int);
683683
drop domain di;
684684
--
685+
-- Check use of a (non-inline-able) SQL function in a domain constraint;
686+
-- this has caused issues in the past
687+
--
688+
create function sql_is_distinct_from(anyelement, anyelement)
689+
returns boolean language sql
690+
as 'select $1 is distinct from $2 limit 1';
691+
create domain inotnull int
692+
check (sql_is_distinct_from(value, null));
693+
select 1::inotnull;
694+
inotnull
695+
----------
696+
1
697+
(1 row)
698+
699+
select null::inotnull;
700+
ERROR: value for domain inotnull violates check constraint "inotnull_check"
701+
create table dom_table (x inotnull);
702+
insert into dom_table values ('1');
703+
insert into dom_table values (1);
704+
insert into dom_table values (null);
705+
ERROR: value for domain inotnull violates check constraint "inotnull_check"
706+
drop table dom_table;
707+
drop domain inotnull;
708+
drop function sql_is_distinct_from(anyelement, anyelement);
709+
--
685710
-- Renaming
686711
--
687712
create domain testdomain1 as int;

‎src/test/regress/sql/domain.sql

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -515,6 +515,30 @@ drop function dom_check(int);
515515

516516
drop domain di;
517517

518+
--
519+
-- Check use of a (non-inline-able) SQL function in a domain constraint;
520+
-- this has caused issues in the past
521+
--
522+
523+
create function sql_is_distinct_from(anyelement, anyelement)
524+
returns boolean language sql
525+
as 'select $1 is distinct from $2 limit 1';
526+
527+
create domain inotnull int
528+
check (sql_is_distinct_from(value, null));
529+
530+
select 1::inotnull;
531+
select null::inotnull;
532+
533+
create table dom_table (x inotnull);
534+
insert into dom_table values ('1');
535+
insert into dom_table values (1);
536+
insert into dom_table values (null);
537+
538+
drop table dom_table;
539+
drop domain inotnull;
540+
drop function sql_is_distinct_from(anyelement, anyelement);
541+
518542
--
519543
-- Renaming
520544
--

0 commit comments

Comments
 (0)
0