plpgsql: check domain constraints
Attached is a patch that makes the following changes:
(1) refactor execQual.c to expose a function for checking an instance of
a domain value against a list of domain constraints
(2) adjust pl/pgsql to fetch the constraints associated with the
function's return value. Because this is expensive, the constraints are
fetched once per session, when the function is compiled. I then modified
pl/pgsql to check any applicable constraints on the return value of a
function before returning it to the backend.
(3) add some regression tests for #2
The patch does NOT add checking of domain constraints for other PLs, or
for intermediate values in PL/PgSQL -- I plan to take a look at doing
one or both of those soon.
I also made a few semi-related cleanups. In pl_exec.c, it seems to me
that estate.eval_econtext MUST be non-NULL during the guts of both
plpgsql_exec_trigger() and plpgsql_exec_function(). Therefore checking
that estate.eval_econtext is non-NULL when cleaning up is unnecessary
(line 649 and 417 in current sources, respectively), so I've removed the
checks. Am I missing something?
Barring any objections I'll apply this patch tomorrow some time.
-Neil
Attachments:
plpgsql_domain_constr-2.patchtext/x-patch; charset=us-ascii; name=plpgsql_domain_constr-2.patchDownload
============================================================
*** src/backend/executor/execQual.c eb1daef85341439578a3f6bb73549c4420292bc3
--- src/backend/executor/execQual.c 20fa43d2335a50a1663c8e3e1d7d9e5d091dc79f
***************
*** 2673,2679 ****
{
CoerceToDomain *ctest = (CoerceToDomain *) cstate->xprstate.expr;
Datum result;
- ListCell *l;
result = ExecEvalExpr(cstate->arg, econtext, isNull, isDone);
--- 2673,2678 ----
***************
*** 2680,2686 ****
if (isDone && *isDone == ExprEndResult)
return result; /* nothing to check */
! foreach(l, cstate->constraints)
{
DomainConstraintState *con = (DomainConstraintState *) lfirst(l);
--- 2679,2707 ----
if (isDone && *isDone == ExprEndResult)
return result; /* nothing to check */
! ExecCheckDomainConstraints(result, *isNull, cstate->constraints,
! ctest->resulttype, econtext);
!
! /* No constraints failed, so return the datum */
! return result;
! }
!
! /*
! * Check a list of domain constraints against an instance of the
! * domain type, 'value'. 'isnull' indicates whether the instance is
! * null. 'constraints' is a list of DomainConstraintState nodes
! * describing the constraints to check. 'valtype' is the OID of the
! * domain type. 'econtext' is the ExprContext in which the constraint
! * expressions are evaluated.
! */
! void
! ExecCheckDomainConstraints(Datum value, bool isnull,
! List *constraints, Oid valtype,
! ExprContext *econtext)
! {
! ListCell *l;
!
! foreach(l, constraints)
{
DomainConstraintState *con = (DomainConstraintState *) lfirst(l);
***************
*** 2687,2697 ****
switch (con->constrainttype)
{
case DOM_CONSTRAINT_NOTNULL:
! if (*isNull)
ereport(ERROR,
(errcode(ERRCODE_NOT_NULL_VIOLATION),
errmsg("domain %s does not allow null values",
! format_type_be(ctest->resulttype))));
break;
case DOM_CONSTRAINT_CHECK:
{
--- 2708,2718 ----
switch (con->constrainttype)
{
case DOM_CONSTRAINT_NOTNULL:
! if (isnull)
ereport(ERROR,
(errcode(ERRCODE_NOT_NULL_VIOLATION),
errmsg("domain %s does not allow null values",
! format_type_be(valtype))));
break;
case DOM_CONSTRAINT_CHECK:
{
***************
*** 2709,2726 ****
save_datum = econtext->domainValue_datum;
save_isNull = econtext->domainValue_isNull;
! econtext->domainValue_datum = result;
! econtext->domainValue_isNull = *isNull;
conResult = ExecEvalExpr(con->check_expr,
econtext, &conIsNull, NULL);
! if (!conIsNull &&
! !DatumGetBool(conResult))
ereport(ERROR,
(errcode(ERRCODE_CHECK_VIOLATION),
errmsg("value for domain %s violates check constraint \"%s\"",
! format_type_be(ctest->resulttype),
con->name)));
econtext->domainValue_datum = save_datum;
econtext->domainValue_isNull = save_isNull;
--- 2730,2746 ----
save_datum = econtext->domainValue_datum;
save_isNull = econtext->domainValue_isNull;
! econtext->domainValue_datum = value;
! econtext->domainValue_isNull = isnull;
conResult = ExecEvalExpr(con->check_expr,
econtext, &conIsNull, NULL);
! if (!conIsNull && !DatumGetBool(conResult))
ereport(ERROR,
(errcode(ERRCODE_CHECK_VIOLATION),
errmsg("value for domain %s violates check constraint \"%s\"",
! format_type_be(valtype),
con->name)));
econtext->domainValue_datum = save_datum;
econtext->domainValue_isNull = save_isNull;
***************
*** 2733,2741 ****
break;
}
}
-
- /* If all has gone well (constraints did not fail) return the datum */
- return result;
}
/*
--- 2753,2758 ----
============================================================
*** src/include/executor/executor.h 502cb98a7719bfb0236c04c8ea7aa999026dc584
--- src/include/executor/executor.h b111913a7ef4735321f09c7686dd95a03b86910f
***************
*** 136,141 ****
--- 136,144 ----
extern int ExecCleanTargetListLength(List *targetlist);
extern TupleTableSlot *ExecProject(ProjectionInfo *projInfo,
ExprDoneCond *isDone);
+ extern void ExecCheckDomainConstraints(Datum value, bool isnull,
+ List *constraints, Oid valtype,
+ ExprContext *econtext);
/*
* prototypes from functions in execScan.c
============================================================
*** src/pl/plpgsql/src/pl_comp.c 425488af1f2bb20c51747073b9837369c846fe38
--- src/pl/plpgsql/src/pl_comp.c baf5c26e94285de9529694a1d891e8c7c8119748
***************
*** 48,53 ****
--- 48,54 ----
#include "catalog/pg_class.h"
#include "catalog/pg_proc.h"
#include "catalog/pg_type.h"
+ #include "commands/typecmds.h"
#include "funcapi.h"
#include "nodes/makefuncs.h"
#include "parser/gramparse.h"
***************
*** 344,350 ****
switch (functype)
{
case T_FUNCTION:
-
/*
* Fetch info about the procedure's parameters. Allocations aren't
* needed permanently, so make them in tmp cxt.
--- 345,350 ----
***************
*** 534,539 ****
--- 534,547 ----
true);
}
}
+
+ /*
+ * If the function returns a domain value, lookup and
+ * store any constraints associated with the domain.
+ */
+ if (typeStruct->typtype == 'd')
+ function->domain_constr = GetDomainConstraints(rettypeid);
+
ReleaseSysCache(typeTup);
break;
============================================================
*** src/pl/plpgsql/src/pl_exec.c 36a133fa66455f08b44cda267840ad15017b2c6c
--- src/pl/plpgsql/src/pl_exec.c fa72a8b7cea04dc747541f4a4fb4c6fde6c3b2e3
***************
*** 413,421 ****
}
}
/* Clean up any leftover temporary memory */
! if (estate.eval_econtext != NULL)
! FreeExprContext(estate.eval_econtext);
estate.eval_econtext = NULL;
exec_eval_cleanup(&estate);
--- 413,426 ----
}
}
+ /* Check any applicable domain constraints */
+ if (func->domain_constr != NIL)
+ ExecCheckDomainConstraints(estate.retval, estate.retisnull,
+ func->domain_constr, func->fn_rettype,
+ estate.eval_econtext);
+
/* Clean up any leftover temporary memory */
! FreeExprContext(estate.eval_econtext);
estate.eval_econtext = NULL;
exec_eval_cleanup(&estate);
***************
*** 646,653 ****
}
/* Clean up any leftover temporary memory */
! if (estate.eval_econtext != NULL)
! FreeExprContext(estate.eval_econtext);
estate.eval_econtext = NULL;
exec_eval_cleanup(&estate);
--- 651,657 ----
}
/* Clean up any leftover temporary memory */
! FreeExprContext(estate.eval_econtext);
estate.eval_econtext = NULL;
exec_eval_cleanup(&estate);
***************
*** 3195,3201 ****
errdetail("The tuple structure of a not-yet-assigned record is indeterminate.")));
/*
! * Get the number of the records field to change and the
* number of attributes in the tuple.
*/
fno = SPI_fnumber(rec->tupdesc, recfield->fieldname);
--- 3199,3205 ----
errdetail("The tuple structure of a not-yet-assigned record is indeterminate.")));
/*
! * Get the number of the record's field to change and the
* number of attributes in the tuple.
*/
fno = SPI_fnumber(rec->tupdesc, recfield->fieldname);
***************
*** 4100,4107 ****
if (!isnull)
{
/*
! * If the type of the queries return value isn't that of the variable,
! * convert it.
*/
if (valtype != reqtype || reqtypmod != -1)
{
--- 4104,4111 ----
if (!isnull)
{
/*
! * If the type of the query's return value isn't that of the
! * variable, convert it.
*/
if (valtype != reqtype || reqtypmod != -1)
{
============================================================
*** src/pl/plpgsql/src/plpgsql.h 40743f1288e49eb6a6aedf3ddb0846b598d7319b
--- src/pl/plpgsql/src/plpgsql.h 064138e4024ab55c179f1999327b0dd7324bdc79
***************
*** 568,573 ****
--- 568,574 ----
int fn_functype;
PLpgSQL_func_hashkey *fn_hashkey; /* back-link to hashtable key */
MemoryContext fn_cxt;
+ List *domain_constr; /* Domain constraints on the return value */
Oid fn_rettype;
int fn_rettyplen;
============================================================
*** src/test/regress/expected/plpgsql.out 3e546643943140cd0a07045bccf1c59f51d63fa2
--- src/test/regress/expected/plpgsql.out 9897920b146f632ba4b77ee425f75709bd1f1710
***************
*** 2721,2723 ****
--- 2721,2750 ----
$$ language plpgsql;
ERROR: end label "outer_label" specified for unlabelled block
CONTEXT: compile of PL/pgSQL function "end_label4" near line 5
+ -- check that domain constraints on the function's return value are
+ -- enforced
+ CREATE DOMAIN foo_domain AS INT4 NOT NULL CHECK (VALUE < 10);
+ CREATE FUNCTION domain_check() RETURNS foo_domain AS $$
+ DECLARE
+ x int;
+ y int;
+ BEGIN
+ x := 35;
+ y := 15;
+ RETURN x - y;
+ END;
+ $$ LANGUAGE plpgsql;
+ SELECT domain_check(); -- should fail
+ ERROR: value for domain foo_domain violates check constraint "foo_domain_check"
+ CONTEXT: PL/pgSQL function "domain_check" while casting return value to function's return type
+ CREATE FUNCTION domain_check_null() RETURNS foo_domain AS $$
+ BEGIN
+ RETURN NULL;
+ END;
+ $$ LANGUAGE plpgsql;
+ SELECT domain_check_null(); -- should fail
+ ERROR: domain foo_domain does not allow null values
+ CONTEXT: PL/pgSQL function "domain_check_null" while casting return value to function's return type
+ DROP DOMAIN foo_domain CASCADE;
+ NOTICE: drop cascades to function domain_check_null()
+ NOTICE: drop cascades to function domain_check()
============================================================
*** src/test/regress/sql/plpgsql.sql e8f2eb786ca15e60b817fc839cc412500ee5a693
--- src/test/regress/sql/plpgsql.sql 3d3ce2e48d253638b307d4bc14f549348491a299
***************
*** 2280,2282 ****
--- 2280,2309 ----
end loop outer_label;
end;
$$ language plpgsql;
+
+ -- check that domain constraints on the function's return value are
+ -- enforced
+ CREATE DOMAIN foo_domain AS INT4 NOT NULL CHECK (VALUE < 10);
+
+ CREATE FUNCTION domain_check() RETURNS foo_domain AS $$
+ DECLARE
+ x int;
+ y int;
+ BEGIN
+ x := 35;
+ y := 15;
+ RETURN x - y;
+ END;
+ $$ LANGUAGE plpgsql;
+
+ SELECT domain_check(); -- should fail
+
+ CREATE FUNCTION domain_check_null() RETURNS foo_domain AS $$
+ BEGIN
+ RETURN NULL;
+ END;
+ $$ LANGUAGE plpgsql;
+
+ SELECT domain_check_null(); -- should fail
+
+ DROP DOMAIN foo_domain CASCADE;
Neil Conway <neilc@samurai.com> writes:
(2) adjust pl/pgsql to fetch the constraints associated with the
function's return value. Because this is expensive, the constraints are
fetched once per session, when the function is compiled.
We have gone out of our way to make sure that domain constraint checking
responds promptly (ie, within one query) to updates of the domain
definition. Caching at the session level in plpgsql would be a
significant regression from that, and I don't think it's acceptable.
If you had a way of invalidating the cache when needed, it'd be great
... but not without that.
I also made a few semi-related cleanups. In pl_exec.c, it seems to me
that estate.eval_econtext MUST be non-NULL during the guts of both
plpgsql_exec_trigger() and plpgsql_exec_function(). Therefore checking
that estate.eval_econtext is non-NULL when cleaning up is unnecessary
(line 649 and 417 in current sources, respectively), so I've removed the
checks. Am I missing something?
The code doesn't currently assume that, and it doesn't seem to me that
saving one simple if-test is a reason to add the assumption ...
regards, tom lane
On Sun, 2006-01-08 at 23:56 -0500, Tom Lane wrote:
We have gone out of our way to make sure that domain constraint checking
responds promptly (ie, within one query) to updates of the domain
definition. Caching at the session level in plpgsql would be a
significant regression from that, and I don't think it's acceptable.
If you had a way of invalidating the cache when needed, it'd be great
... but not without that.
GetDomainConstraints() looks fairly expensive, so it would be nice to do
some caching. What would the best way to implement this be? I had
thought that perhaps the typcache would work, but there seems to be no
method to flush stale typcache data. Perhaps we could add support for
typcache invalidation (via a new sinval message), and then add the
domain constraint information to the typcache. Or is there an easier
way?
I also made a few semi-related cleanups. In pl_exec.c, it seems to me
that estate.eval_econtext MUST be non-NULL during the guts of both
plpgsql_exec_trigger() and plpgsql_exec_function(). Therefore checking
that estate.eval_econtext is non-NULL when cleaning up is unnecessary
(line 649 and 417 in current sources, respectively), so I've removed the
checks. Am I missing something?The code doesn't currently assume that, and it doesn't seem to me that
saving one simple if-test is a reason to add the assumption ...
It's not a matter of "saving an if-test", it's a matter of code clarity.
Code ought to be consistent about whether any given pointer variable
might be NULL. This makes it easier for a programmer to tell if new code
ought to check for NULL-ness before using the pointer. (In fact, when
modifying plpgsql_exec_function() for this patch, I had to spend a few
minutes checking for the situations in which estate.eval_econtext might
be NULL.)
Some languages (e.g. ML) actually distinguish in the type system between
"references that might be NULL" and those that cannot be. That's not
possible in C, but consistently treating NULL-ness makes it easier to
determine this by hand.
-Neil
Neil Conway <neilc@samurai.com> writes:
GetDomainConstraints() looks fairly expensive, so it would be nice to do
some caching. What would the best way to implement this be? I had
thought that perhaps the typcache would work, but there seems to be no
method to flush stale typcache data. Perhaps we could add support for
typcache invalidation (via a new sinval message), and then add the
domain constraint information to the typcache. Or is there an easier
way?
Yeah, I had been thinking of exactly the same thing a few months ago
after noting that GetDomainConstraints() can be pretty dang slow ---
it seemed to be a major bottleneck for Kevin Grittner here:
http://archives.postgresql.org/pgsql-hackers/2005-09/msg01135.php
Unfortunately the rest of that conversation was unintentionally
off-list, but we identified heavy use of pg_constraint_contypid_index
as the source of his issue, and I said
: Hmm. The only commonly-used code path I can see that would touch
: pg_constraint_contypid_index is GetDomainConstraints(), which would be
: called (once) during startup of a command that involves a CoerceToDomain
: expression node. So if the heavily-hit table has any domain-type
: columns, it's possible that a steady stream of inserts or updates
: could have kept that index tied up.
:
: It might be worth introducing a system cache that could be used to
: extract the constraints for a domain without going to the catalog for
: every single command. There's been very little work done to date on
: optimizing operations on domains :-(
The lack of typcache invalidation is something that will eventually
bite us in other ways, so we need to add the facility anyway. We
don't really need a "new sinval message", as an inval on the pg_type
row will serve perfectly well --- what we need is something comparable
to CacheInvalidateRelcache() to cause such a message to be sent when
we haven't actually changed the pg_type row itself.
Do you want to work on this? I can if you don't.
BTW, in connection with the lookup_rowtype_tupdesc fiasco, it's pretty
obvious that any data structure returned by this function will need
to be either copied or reference-counted.
regards, tom lane