Re: NOT ENFORCED constraint feature - Mailing list pgsql-hackers

I started reviewing patch 0001 for check constraints.  I think it's a
good idea how you structured it so that we can start with this
relatively simple feature and get all the syntax parsing etc. right.

I also looked over the remaining patches a bit.  The general structure
looks right to me.  But I haven't done a detailed review yet.

The 0001 patch needs a rebase over the recently re-committed patch for
catalogued not-null constraints.  This might need a little work to
verify that everything still makes sense.

(I suppose technically we could support not-enforced not-null
constraints.  But I would stay away from that for now.  That not-null
constraints business is very complicated, don't get dragged into
it. ;-) )


Some more detailed comments on the code:

* src/backend/access/common/tupdesc.c

Try to keep the order of the fields consistent.  In tupdesc.h you have
ccenforced before ccnoinherit, here you have it after.  Either way is
fine, but let's keep it consistent.  (If you change it in tupdesc.h,
also check relcache.c.)


* src/backend/commands/tablecmds.c

             cooked->skip_validation = false;
+           cooked->is_enforced = true;
             cooked->is_local = true;    /* not used for defaults */
             cooked->inhcount = 0;   /* ditto */

Add a comment like "not used for defaults" to the new line.

Or maybe this should be rewritten slightly.  There might be more
fields that are not used for defaults, like "skip_validation"?  Maybe
they just shouldn't be set here, seems useless and confusing.

@@ -9481,6 +9484,9 @@ ATAddCheckConstraint(List **wqueue, 
AlteredTableInfo *tab, Relation rel,
     {
         CookedConstraint *ccon = (CookedConstraint *) lfirst(lcon);

+       /* Only CHECK constraint can be not enforced */
+       Assert(ccon->is_enforced || ccon->contype == CONSTRAINT_CHECK);
+

Is this assertion useful, since we are already in a function named
ATAddCheckConstraint()?

@@ -11947,7 +11961,9 @@ ATExecValidateConstraint(List **wqueue, Relation 
rel, char *constrName,
         }

         /*
-        * Now update the catalog, while we have the door open.
+        * Now update the catalog regardless of enforcement; the validated
+        * flag will not take effect until the constraint is marked as
+        * enforced.
          */

Can you clarify what you mean here?  Is the enforced flag set later?
I don't see that in the code.  What is the interaction between
constraint validation and the enforced flag?


* src/backend/commands/typecmds.c

You should also check and error if CONSTR_ATTR_ENFORCED is specified
(even though it's effectively the default).  This matches SQL standard
language: "For every <domain constraint> specified: ... If <constraint
characteristics> is specified, then neither ENFORCED nor NOT ENFORCED
shall be specified."

The error code should be something like
ERRCODE_INVALID_OBJECT_DEFINITION instead of
ERRCODE_FEATURE_NOT_SUPPORTED.  The former is more for features that
are impossible, the latter for features we haven't gotten to yet.


* src/backend/parser/gram.y

Same as above, in processCASbits(), you should add a similar check for
CAS_ENFORCED, meaning that for example specifying UNIQUE ENFORCED is
not allowed (even though it's the default).  This matches SQL standard
language: "If <unique constraint definition> is specified, then
<constraint characteristics> shall not specify a <constraint
enforcement>."


* src/backend/parser/parse_utilcmd.c

@@ -1317,6 +1321,7 @@ expandTableLikeClause(RangeVar *heapRel, 
TableLikeClause *table_like_clause)
             n->is_no_inherit = ccnoinherit;
             n->raw_expr = NULL;
             n->cooked_expr = nodeToString(ccbin_node);
+           n->is_enforced = true;

This has the effect that if you use the LIKE clause with INCLUDING
CONSTRAINTS, the new constraint is always ENFORCED.  Is this what we
want?  Did you have a reason?  I'm not sure what the ideal behavior
might be.  But if we want it like this, maybe we should document this
or at least put a comment here or something.


* src/backend/utils/adt/ruleutils.c

The syntax requires the NOT ENFORCED clause to be after DEFERRABLE
etc., but this code does it the other way around.  You should move the
new code after the switch statement and below the DEFERRABLE stuff.

I wouldn't worry about restricting it based on constraint type.  The
DEFERRABLE stuff doesn't do that either.  We can assume that the
catalog contents are sane.


* src/include/catalog/pg_constraint.h

There needs to be an update in catalogs.sgml for the new catalog column.


* src/test/regress/sql/constraints.sql

Possible additional test cases:
- trying [NOT] ENFORCED with a domain (CREATE and ALTER cases)
- trying [NOT] ENFORCED with an unsupported constraint type (maybe UNIQUE)


A note for the later patches: With patches 0001 through 0005 applied,
I get compiler warnings:

../src/backend/commands/tablecmds.c:10918:17: error: 'deleteTriggerOid' 
may be used uninitialized [-Werror=maybe-uninitialized]
../src/backend/commands/tablecmds.c:10918:17: error: 'updateTriggerOid' 
may be used uninitialized [-Werror=maybe-uninitialized]

(both with gcc and clang).