Use the typcache to cache constraints for domain types.

Previously, we cached domain constraints for the life of a query, or
really for the life of the FmgrInfo struct that was used to invoke
domain_in() or domain_check().  But plpgsql (and probably other places)
are set up to cache such FmgrInfos for the whole lifespan of a session,
which meant they could be enforcing really stale sets of constraints.
On the other hand, searching pg_constraint once per query gets kind of
expensive too: testing says that as much as half the runtime of a
trivial query such as "SELECT 0::domaintype" went into that.

To fix this, delegate the responsibility for tracking a domain's
constraints to the typcache, which has the infrastructure needed to
detect syscache invalidation events that signal possible changes.
This not only removes unnecessary repeat reads of pg_constraint,
but ensures that we never apply stale constraint data: whatever we
use is the current data according to syscache rules.

Unfortunately, the current configuration of the system catalogs means
we have to flush cached domain-constraint data whenever either pg_type
or pg_constraint changes, which happens rather a lot (eg, creation or
deletion of a temp table will do it).  It might be worth rearranging
things to split pg_constraint into two catalogs, of which the domain
constraint one would probably be very low-traffic.  That's a job for
another patch though, and in any case this patch should improve matters
materially even with that handicap.

This patch makes use of the recently-added memory context reset callback
feature to manage the lifespan of domain constraint caches, so that we
don't risk deleting a cache that might be in the midst of evaluation.

Although this is a bug fix as well as a performance improvement, no
back-patch.  There haven't been many if any field complaints about
stale domain constraint checks, so it doesn't seem worth taking the
risk of modifying data structures as basic as MemoryContexts in back
branches.

src/backend/commands/tablecmds.c

@@ -4824,7 +4824,7 @@ ATExecAddColumn(List **wqueue, AlteredTableInfo *tab, Relation rel,
 	{
 		defval = (Expr *) build_column_default(rel, attribute.attnum);
 
-		if (!defval && GetDomainConstraints(typeOid) != NIL)
+		if (!defval && DomainHasConstraints(typeOid))
 		{
 			Oid			baseTypeId;
 			int32		baseTypeMod;
@@ -7778,7 +7778,7 @@ ATColumnChangeRequiresRewrite(Node *expr, AttrNumber varattno)
 		{
 			CoerceToDomain *d = (CoerceToDomain *) expr;
 
-			if (GetDomainConstraints(d->resulttype) != NIL)
+			if (DomainHasConstraints(d->resulttype))
 				return true;
 			expr = (Node *) d->arg;
 		}

src/backend/commands/typecmds.c

@@ -31,15 +31,11 @@
  */
 #include "postgres.h"
 
-#include "access/genam.h"
-#include "access/heapam.h"
 #include "access/htup_details.h"
 #include "access/xact.h"
 #include "catalog/binary_upgrade.h"
 #include "catalog/catalog.h"
-#include "catalog/dependency.h"
 #include "catalog/heap.h"
-#include "catalog/indexing.h"
 #include "catalog/objectaccess.h"
 #include "catalog/pg_authid.h"
 #include "catalog/pg_collation.h"
@@ -59,14 +55,12 @@
 #include "executor/executor.h"
 #include "miscadmin.h"
 #include "nodes/makefuncs.h"
-#include "optimizer/planner.h"
 #include "optimizer/var.h"
 #include "parser/parse_coerce.h"
 #include "parser/parse_collate.h"
 #include "parser/parse_expr.h"
 #include "parser/parse_func.h"
 #include "parser/parse_type.h"
-#include "utils/acl.h"
 #include "utils/builtins.h"
 #include "utils/fmgroids.h"
 #include "utils/lsyscache.h"
@@ -75,7 +69,6 @@
 #include "utils/ruleutils.h"
 #include "utils/snapmgr.h"
 #include "utils/syscache.h"
-#include "utils/tqual.h"
 
 
 /* result structure for get_rels_with_domain() */
@@ -3081,126 +3074,6 @@ domainAddConstraint(Oid domainOid, Oid domainNamespace, Oid baseTypeOid,
 	return ccbin;
 }
 
-/*
- * GetDomainConstraints - get a list of the current constraints of domain
- *
- * Returns a possibly-empty list of DomainConstraintState nodes.
- *
- * This is called by the executor during plan startup for a CoerceToDomain
- * expression node.  The given constraints will be checked for each value
- * passed through the node.
- *
- * We allow this to be called for non-domain types, in which case the result
- * is always NIL.
- */
-List *
-GetDomainConstraints(Oid typeOid)
-{
-	List	   *result = NIL;
-	bool		notNull = false;
-	Relation	conRel;
-
-	conRel = heap_open(ConstraintRelationId, AccessShareLock);
-
-	for (;;)
-	{
-		HeapTuple	tup;
-		HeapTuple	conTup;
-		Form_pg_type typTup;
-		ScanKeyData key[1];
-		SysScanDesc scan;
-
-		tup = SearchSysCache1(TYPEOID, ObjectIdGetDatum(typeOid));
-		if (!HeapTupleIsValid(tup))
-			elog(ERROR, "cache lookup failed for type %u", typeOid);
-		typTup = (Form_pg_type) GETSTRUCT(tup);
-
-		if (typTup->typtype != TYPTYPE_DOMAIN)
-		{
-			/* Not a domain, so done */
-			ReleaseSysCache(tup);
-			break;
-		}
-
-		/* Test for NOT NULL Constraint */
-		if (typTup->typnotnull)
-			notNull = true;
-
-		/* Look for CHECK Constraints on this domain */
-		ScanKeyInit(&key[0],
-					Anum_pg_constraint_contypid,
-					BTEqualStrategyNumber, F_OIDEQ,
-					ObjectIdGetDatum(typeOid));
-
-		scan = systable_beginscan(conRel, ConstraintTypidIndexId, true,
-								  NULL, 1, key);
-
-		while (HeapTupleIsValid(conTup = systable_getnext(scan)))
-		{
-			Form_pg_constraint c = (Form_pg_constraint) GETSTRUCT(conTup);
-			Datum		val;
-			bool		isNull;
-			Expr	   *check_expr;
-			DomainConstraintState *r;
-
-			/* Ignore non-CHECK constraints (presently, shouldn't be any) */
-			if (c->contype != CONSTRAINT_CHECK)
-				continue;
-
-			/*
-			 * Not expecting conbin to be NULL, but we'll test for it anyway
-			 */
-			val = fastgetattr(conTup, Anum_pg_constraint_conbin,
-							  conRel->rd_att, &isNull);
-			if (isNull)
-				elog(ERROR, "domain \"%s\" constraint \"%s\" has NULL conbin",
-					 NameStr(typTup->typname), NameStr(c->conname));
-
-			check_expr = (Expr *) stringToNode(TextDatumGetCString(val));
-
-			/* ExecInitExpr assumes we've planned the expression */
-			check_expr = expression_planner(check_expr);
-
-			r = makeNode(DomainConstraintState);
-			r->constrainttype = DOM_CONSTRAINT_CHECK;
-			r->name = pstrdup(NameStr(c->conname));
-			r->check_expr = ExecInitExpr(check_expr, NULL);
-
-			/*
-			 * use lcons() here because constraints of lower domains should be
-			 * applied earlier.
-			 */
-			result = lcons(r, result);
-		}
-
-		systable_endscan(scan);
-
-		/* loop to next domain in stack */
-		typeOid = typTup->typbasetype;
-		ReleaseSysCache(tup);
-	}
-
-	heap_close(conRel, AccessShareLock);
-
-	/*
-	 * Only need to add one NOT NULL check regardless of how many domains in
-	 * the stack request it.
-	 */
-	if (notNull)
-	{
-		DomainConstraintState *r = makeNode(DomainConstraintState);
-
-		r->constrainttype = DOM_CONSTRAINT_NOTNULL;
-		r->name = pstrdup("NOT NULL");
-		r->check_expr = NULL;
-
-		/* lcons to apply the nullness check FIRST */
-		result = lcons(r, result);
-	}
-
-	return result;
-}
-
 
 /*
  * Execute ALTER TYPE RENAME

src/backend/executor/execQual.c

@@ -41,7 +41,6 @@
 #include "access/tupconvert.h"
 #include "catalog/objectaccess.h"
 #include "catalog/pg_type.h"
-#include "commands/typecmds.h"
 #include "executor/execdebug.h"
 #include "executor/nodeSubplan.h"
 #include "funcapi.h"
@@ -3929,7 +3928,10 @@ ExecEvalCoerceToDomain(CoerceToDomainState *cstate, ExprContext *econtext,
 	if (isDone && *isDone == ExprEndResult)
 		return result;			/* nothing to check */
 
-	foreach(l, cstate->constraints)
+	/* Make sure we have up-to-date constraints */
+	UpdateDomainConstraintRef(cstate->constraint_ref);
+
+	foreach(l, cstate->constraint_ref->constraints)
 	{
 		DomainConstraintState *con = (DomainConstraintState *) lfirst(l);
 
@@ -5050,7 +5052,12 @@ ExecInitExpr(Expr *node, PlanState *parent)
 
 				cstate->xprstate.evalfunc = (ExprStateEvalFunc) ExecEvalCoerceToDomain;
 				cstate->arg = ExecInitExpr(ctest->arg, parent);
-				cstate->constraints = GetDomainConstraints(ctest->resulttype);
+				/* We spend an extra palloc to reduce header inclusions */
+				cstate->constraint_ref = (DomainConstraintRef *)
+					palloc(sizeof(DomainConstraintRef));
+				InitDomainConstraintRef(ctest->resulttype,
+										cstate->constraint_ref,
+										CurrentMemoryContext);
 				state = (ExprState *) cstate;
 			}
 			break;

src/backend/utils/adt/domains.c

@@ -12,10 +12,9 @@
  * 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 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.
+ *	1.  We rely on the typcache to keep up-to-date copies of the constraints.
+ *	2.  In a nest of domains, we flatten the checking of all the levels
+ *		into just one operation (the typcache does this for us).
  *	3.  If there are CHECK constraints, we cache a standalone ExprContext
  *		to evaluate them in.
  *
@@ -33,12 +32,12 @@
 
 #include "access/htup_details.h"
 #include "catalog/pg_type.h"
-#include "commands/typecmds.h"
 #include "executor/executor.h"
 #include "lib/stringinfo.h"
 #include "utils/builtins.h"
 #include "utils/lsyscache.h"
 #include "utils/syscache.h"
+#include "utils/typcache.h"
 
 
 /*
@@ -52,8 +51,8 @@ typedef struct DomainIOData
 	Oid			typioparam;
 	int32		typtypmod;
 	FmgrInfo	proc;
-	/* List of constraint items to check */
-	List	   *constraint_list;
+	/* Reference to cached list of constraint items to check */
+	DomainConstraintRef constraint_ref;
 	/* Context for evaluating CHECK constraints in */
 	ExprContext *econtext;
 	/* Memory context this cache is in */
@@ -63,16 +62,19 @@ typedef struct DomainIOData
 
 /*
  * domain_state_setup - initialize the cache for a new domain type.
+ *
+ * Note: we can't re-use the same cache struct for a new domain type,
+ * since there's no provision for releasing the DomainConstraintRef.
+ * If a call site needs to deal with a new domain type, we just leak
+ * the old struct for the duration of the query.
  */
-static void
-domain_state_setup(DomainIOData *my_extra, Oid domainType, bool binary,
-				   MemoryContext mcxt)
+static DomainIOData *
+domain_state_setup(Oid domainType, bool binary, MemoryContext mcxt)
 {
+	DomainIOData *my_extra;
 	Oid			baseType;
-	MemoryContext oldcontext;
 
-	/* Mark cache invalid */
-	my_extra->domain_type = InvalidOid;
+	my_extra = (DomainIOData *) MemoryContextAlloc(mcxt, sizeof(DomainIOData));
 
 	/* Find out the base type */
 	my_extra->typtypmod = -1;
@@ -95,9 +97,7 @@ domain_state_setup(DomainIOData *my_extra, Oid domainType, bool binary,
 	fmgr_info_cxt(my_extra->typiofunc, &my_extra->proc, mcxt);
 
 	/* Look up constraints for domain */
-	oldcontext = MemoryContextSwitchTo(mcxt);
-	my_extra->constraint_list = GetDomainConstraints(domainType);
-	MemoryContextSwitchTo(oldcontext);
+	InitDomainConstraintRef(domainType, &my_extra->constraint_ref, mcxt);
 
 	/* We don't make an ExprContext until needed */
 	my_extra->econtext = NULL;
@@ -105,6 +105,8 @@ domain_state_setup(DomainIOData *my_extra, Oid domainType, bool binary,
 
 	/* Mark cache valid */
 	my_extra->domain_type = domainType;
+
+	return my_extra;
 }
 
 /*
@@ -118,7 +120,10 @@ domain_check_input(Datum value, bool isnull, DomainIOData *my_extra)
 	ExprContext *econtext = my_extra->econtext;
 	ListCell   *l;
 
-	foreach(l, my_extra->constraint_list)
+	/* Make sure we have up-to-date constraints */
+	UpdateDomainConstraintRef(&my_extra->constraint_ref);
+
+	foreach(l, my_extra->constraint_ref.constraints)
 	{
 		DomainConstraintState *con = (DomainConstraintState *) lfirst(l);
 
@@ -215,20 +220,16 @@ domain_in(PG_FUNCTION_ARGS)
 
 	/*
 	 * We arrange to look up the needed info just once per series of calls,
-	 * assuming the domain type doesn't change underneath us.
+	 * assuming the domain type doesn't change underneath us (which really
+	 * shouldn't happen, but cope if it does).
 	 */
 	my_extra = (DomainIOData *) fcinfo->flinfo->fn_extra;
-	if (my_extra == NULL)
+	if (my_extra == NULL || my_extra->domain_type != domainType)
 	{
-		my_extra = (DomainIOData *) MemoryContextAlloc(fcinfo->flinfo->fn_mcxt,
-													   sizeof(DomainIOData));
-		domain_state_setup(my_extra, domainType, false,
-						   fcinfo->flinfo->fn_mcxt);
+		my_extra = domain_state_setup(domainType, false,
+									  fcinfo->flinfo->fn_mcxt);
 		fcinfo->flinfo->fn_extra = (void *) my_extra;
 	}
-	else if (my_extra->domain_type != domainType)
-		domain_state_setup(my_extra, domainType, false,
-						   fcinfo->flinfo->fn_mcxt);
 
 	/*
 	 * Invoke the base type's typinput procedure to convert the data.
@@ -275,20 +276,16 @@ domain_recv(PG_FUNCTION_ARGS)
 
 	/*
 	 * We arrange to look up the needed info just once per series of calls,
-	 * assuming the domain type doesn't change underneath us.
+	 * assuming the domain type doesn't change underneath us (which really
+	 * shouldn't happen, but cope if it does).
 	 */
 	my_extra = (DomainIOData *) fcinfo->flinfo->fn_extra;
-	if (my_extra == NULL)
+	if (my_extra == NULL || my_extra->domain_type != domainType)
 	{
-		my_extra = (DomainIOData *) MemoryContextAlloc(fcinfo->flinfo->fn_mcxt,
-													   sizeof(DomainIOData));
-		domain_state_setup(my_extra, domainType, true,
-						   fcinfo->flinfo->fn_mcxt);
+		my_extra = domain_state_setup(domainType, true,
+									  fcinfo->flinfo->fn_mcxt);
 		fcinfo->flinfo->fn_extra = (void *) my_extra;
 	}
-	else if (my_extra->domain_type != domainType)
-		domain_state_setup(my_extra, domainType, true,
-						   fcinfo->flinfo->fn_mcxt);
 
 	/*
 	 * Invoke the base type's typreceive procedure to convert the data.
@@ -326,20 +323,17 @@ domain_check(Datum value, bool isnull, Oid domainType,
 
 	/*
 	 * We arrange to look up the needed info just once per series of calls,
-	 * assuming the domain type doesn't change underneath us.
+	 * assuming the domain type doesn't change underneath us (which really
+	 * shouldn't happen, but cope if it does).
 	 */
 	if (extra)
 		my_extra = (DomainIOData *) *extra;
-	if (my_extra == NULL)
+	if (my_extra == NULL || my_extra->domain_type != domainType)
 	{
-		my_extra = (DomainIOData *) MemoryContextAlloc(mcxt,
-													   sizeof(DomainIOData));
-		domain_state_setup(my_extra, domainType, true, mcxt);
+		my_extra = domain_state_setup(domainType, true, mcxt);
 		if (extra)
 			*extra = (void *) my_extra;
 	}
-	else if (my_extra->domain_type != domainType)
-		domain_state_setup(my_extra, domainType, true, mcxt);
 
 	/*
 	 * Do the necessary checks to ensure it's a valid domain value.

src/include/commands/typecmds.h

@@ -39,8 +39,6 @@ extern Oid AlterDomainDropConstraint(List *names, const char *constrName,
 
 extern void checkDomainOwner(HeapTuple tup);
 
-extern List *GetDomainConstraints(Oid typeOid);
-
 extern Oid	RenameType(RenameStmt *stmt);
 extern Oid	AlterTypeOwner(List *names, Oid newOwnerId, ObjectType objecttype);
 extern void AlterTypeOwnerInternal(Oid typeOid, Oid newOwnerId,

src/include/nodes/execnodes.h

@@ -942,8 +942,9 @@ typedef struct CoerceToDomainState
 {
 	ExprState	xprstate;
 	ExprState  *arg;			/* input expression */
-	/* Cached list of constraints that need to be checked */
-	List	   *constraints;	/* list of DomainConstraintState nodes */
+	/* Cached set of constraints that need to be checked */
+	/* use struct pointer to avoid including typcache.h here */
+	struct DomainConstraintRef *constraint_ref;
 } CoerceToDomainState;
 
 /*

src/include/utils/typcache.h

@@ -20,6 +20,9 @@
 #include "fmgr.h"
 
 
+/* DomainConstraintCache is an opaque struct known only within typcache.c */
+typedef struct DomainConstraintCache DomainConstraintCache;
+
 /* TypeCacheEnumData is an opaque struct known only within typcache.c */
 struct TypeCacheEnumData;
 
@@ -84,6 +87,12 @@ typedef struct TypeCacheEntry
 	FmgrInfo	rng_canonical_finfo;	/* canonicalization function, if any */
 	FmgrInfo	rng_subdiff_finfo;		/* difference function, if any */
 
+	/*
+	 * Domain constraint data if it's a domain type.  NULL if not domain, or
+	 * if domain has no constraints, or if information hasn't been requested.
+	 */
+	DomainConstraintCache *domainData;
+
 	/* Private data, for internal use of typcache.c only */
 	int			flags;			/* flags about what we've computed */
 
@@ -92,6 +101,9 @@ typedef struct TypeCacheEntry
 	 * information hasn't been requested.
 	 */
 	struct TypeCacheEnumData *enumData;
+
+	/* We also maintain a list of all known domain-type cache entries */
+	struct TypeCacheEntry *nextDomain;
 } TypeCacheEntry;
 
 /* Bit flags to indicate which fields a given caller needs to have set */
@@ -107,9 +119,34 @@ typedef struct TypeCacheEntry
 #define TYPECACHE_BTREE_OPFAMILY	0x0200
 #define TYPECACHE_HASH_OPFAMILY		0x0400
 #define TYPECACHE_RANGE_INFO		0x0800
+#define TYPECACHE_DOMAIN_INFO		0x1000
+
+/*
+ * Callers wishing to maintain a long-lived reference to a domain's constraint
+ * set must store it in one of these.  Use InitDomainConstraintRef() and
+ * UpdateDomainConstraintRef() to manage it.  Note: DomainConstraintState is
+ * considered an executable expression type, so it's defined in execnodes.h.
+ */
+typedef struct DomainConstraintRef
+{
+	List	   *constraints;	/* list of DomainConstraintState nodes */
+
+	/* Management data --- treat these fields as private to typcache.c */
+	TypeCacheEntry *tcache;		/* owning typcache entry */
+	DomainConstraintCache *dcc; /* current constraints, or NULL if none */
+	MemoryContextCallback callback;		/* used to release refcount when done */
+} DomainConstraintRef;
+
 
 extern TypeCacheEntry *lookup_type_cache(Oid type_id, int flags);
 
+extern void InitDomainConstraintRef(Oid type_id, DomainConstraintRef *ref,
+						MemoryContext refctx);
+
+extern void UpdateDomainConstraintRef(DomainConstraintRef *ref);
+
+extern bool DomainHasConstraints(Oid type_id);
+
 extern TupleDesc lookup_rowtype_tupdesc(Oid type_id, int32 typmod);
 
 extern TupleDesc lookup_rowtype_tupdesc_noerror(Oid type_id, int32 typmod,

src/test/regress/expected/domain.out

@@ -652,6 +652,36 @@ ERROR:  value for domain orderedpair violates check constraint "orderedpair_chec
 CONTEXT:  PL/pgSQL function array_elem_check(integer) line 5 at assignment
 drop function array_elem_check(int);
 --
+-- Check enforcement of changing constraints in plpgsql
+--
+create domain di as int;
+create function dom_check(int) returns di as $$
+declare d di;
+begin
+  d := $1;
+  return d;
+end
+$$ language plpgsql immutable;
+select dom_check(0);
+ dom_check 
+-----------
+         0
+(1 row)
+
+alter domain di add constraint pos check (value > 0);
+select dom_check(0); -- fail
+ERROR:  value for domain di violates check constraint "pos"
+CONTEXT:  PL/pgSQL function dom_check(integer) line 4 at assignment
+alter domain di drop constraint pos;
+select dom_check(0);
+ dom_check 
+-----------
+         0
+(1 row)
+
+drop function dom_check(int);
+drop domain di;
+--
 -- Renaming
 --
 create domain testdomain1 as int;

src/test/regress/sql/domain.sql

@@ -487,6 +487,33 @@ select array_elem_check(-1);
 
 drop function array_elem_check(int);
 
+--
+-- Check enforcement of changing constraints in plpgsql
+--
+
+create domain di as int;
+
+create function dom_check(int) returns di as $$
+declare d di;
+begin
+  d := $1;
+  return d;
+end
+$$ language plpgsql immutable;
+
+select dom_check(0);
+
+alter domain di add constraint pos check (value > 0);
+
+select dom_check(0); -- fail
+
+alter domain di drop constraint pos;
+
+select dom_check(0);
+
+drop function dom_check(int);
+
+drop domain di;
 
 --
 -- Renaming