Re: NOT ENFORCED constraint feature

On Tue, Nov 12, 2024 at 3:48 PM Peter Eisentraut <peter(at)eisentraut(dot)org> wrote:
>
> 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.
>
Thank you for your feedback and suggestions.

> 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. ;-) )
>
True. I had a quick conversation with Álvaro at PGConf.EU about my
current work and the pending ALTER CONSTRAINT support for CHECK
constraints. He mentioned that we might also need the same support for
NULL constraints. I'll look into that as well while working on ALTER
CONSTRAINT.

>
> 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.)
>
Done.

>
> * 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.
>
Yeah, setting here is confusing, I removed that we do not need.

> @@ -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()?
>
Yes, Removed this. Assertion in CreateConstraintEntry() is more than enough.

> @@ -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?
>
I revised the comment in the 0005 patch for clarity. My intent is
that, to trigger validation, the constraint must be enforced.
Additionally, when changing a constraint from non-enforced to
enforced, similar validation is triggered only if the constraint is
valid; otherwise, we simply update the constraint enforceability only.

>
> * 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.
>
Understood. Fixed.

>
> * 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>."
>
Done. In processCASbits(), the error code will be
ERRCODE_FEATURE_NOT_SUPPORTED for consistency with the previous error.
Additionally, is_enforced = true is updated again in the CAS_ENFORCED
check block to align with the existing code style, which I believe is
reasonable.

>
> * 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.
>
You are correct; this is a bug. It has been fixed in the attached
version, and tests have been added for it.

>
> * 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.
>
Done.

>
> * src/include/catalog/pg_constraint.h
>
> There needs to be an update in catalogs.sgml for the new catalog column.
>
Done.

>
> * 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)
>
Added. I thought about adding tests for all other constraints, but it
seemed excessive, so I decided not to.

>
> 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).
>
For some reason, my GCC 11.5 on the CentOS machine isn’t showing this
warning. However, I found the unassigned variable, fixed it, and tried
compiling on macOS, where it's now clean.

Updated version attached.

Regards,
Amul