Thread: Re: NOT ENFORCED constraint feature
On Tue, Oct 8, 2024, at 11:06, Amul Sul wrote: > The attached patch proposes adding the ability to define CHECK and > FOREIGN KEY constraints as NOT ENFORCED. Thanks for working on this! > Adding NOT ENFORCED to CHECK constraints is simple, see 0001 patch, I've looked at the 0001 patch and think it looks simple and straight forward. > but implementing it for FOREIGN KEY constraints requires more code due > to triggers, see 0002 - 0005 patches. I can't say that much yet about the code changes in 0002 - 0005 yet, but I've tested the patches and successfully experimented with the feature. Also think the documentation is good and sound. Only found a minor typo: - Adding a enforced <literal>CHECK</literal> or <literal>NOT NULL</literal> + Adding an enforced <literal>CHECK</literal> or <literal>NOT NULL</literal> > There are various approaches for > implementing NOT ENFORCED foreign keys, what I thought of: > > 1. When defining a NOT ENFORCED foreign key, skip the creation of > triggers used for referential integrity check, while defining an > ENFORCED foreign key, remain the same as the current behaviour. If an > existing foreign key is changed to NOT ENFORCED, the triggers are > dropped, and when switching it back to ENFORCED, the triggers are > recreated. > > 2. Another approach could be to create the NOT ENFORCED constraint > with the triggers as usual, but disable those triggers by updating the > pg_trigger catalog so that they are never executed for the check. And > enable them when the constraint is changed back to ENFORCED. > > 3. Similarly, a final approach would involve updating the logic where > trigger execution is decided and skipping the execution if the > constraint is not enforced, rather than modifying the pg_trigger > catalog. > > In the attached patch, the first approach has been implemented. This > requires more code changes but prevents unused triggers from being > left in the database and avoids the need for changes all over the > place to skip trigger execution, which could be missed in future code > additions. I also like the first approach, since I think it's nice the pg_trigger entires are inserted / deleted upon enforced / not enforced. > The ALTER CONSTRAINT operation in the patch added code to handle > dropping and recreating triggers. An alternative approach would be to > simplify the process by dropping and recreating the FK constraint, > which would automatically handle skipping or creating triggers for NOT > ENFORCED or ENFORCED FK constraints. However, I wasn't sure if this > was the right approach, as I couldn't find any existing ALTER > operations that follow this pattern. I think the current approach of dropping and recreating the triggers is best, since if we would instead be dropping and recreating the FK constraint, that would cause problems if some other future SQL feature would need to introduce dependencies on the FK constraints via pg_depend. Best regards, Joel
On 2024-10-09 We 5:14 AM, Joel Jacobson wrote:
On Tue, Oct 8, 2024, at 11:06, Amul Sul wrote:The attached patch proposes adding the ability to define CHECK and FOREIGN KEY constraints as NOT ENFORCED.Thanks for working on this!Adding NOT ENFORCED to CHECK constraints is simple, see 0001 patch,I've looked at the 0001 patch and think it looks simple and straight forward.but implementing it for FOREIGN KEY constraints requires more code due to triggers, see 0002 - 0005 patches.I can't say that much yet about the code changes in 0002 - 0005 yet, but I've tested the patches and successfully experimented with the feature. Also think the documentation is good and sound. Only found a minor typo: - Adding a enforced <literal>CHECK</literal> or <literal>NOT NULL</literal> + Adding an enforced <literal>CHECK</literal> or <literal>NOT NULL</literal>There are various approaches for implementing NOT ENFORCED foreign keys, what I thought of: 1. When defining a NOT ENFORCED foreign key, skip the creation of triggers used for referential integrity check, while defining an ENFORCED foreign key, remain the same as the current behaviour. If an existing foreign key is changed to NOT ENFORCED, the triggers are dropped, and when switching it back to ENFORCED, the triggers are recreated. 2. Another approach could be to create the NOT ENFORCED constraint with the triggers as usual, but disable those triggers by updating the pg_trigger catalog so that they are never executed for the check. And enable them when the constraint is changed back to ENFORCED. 3. Similarly, a final approach would involve updating the logic where trigger execution is decided and skipping the execution if the constraint is not enforced, rather than modifying the pg_trigger catalog. In the attached patch, the first approach has been implemented. This requires more code changes but prevents unused triggers from being left in the database and avoids the need for changes all over the place to skip trigger execution, which could be missed in future code additions.I also like the first approach, since I think it's nice the pg_trigger entires are inserted / deleted upon enforced / not enforced.
I also prefer this, as it gets us out from any possible dance with enabling / disabling triggers.
cheers
andrew
-- Andrew Dunstan EDB: https://www.enterprisedb.com
On Wed, Oct 9, 2024 at 2:44 PM Joel Jacobson <joel@compiler.org> wrote: > > On Tue, Oct 8, 2024, at 11:06, Amul Sul wrote: > > The attached patch proposes adding the ability to define CHECK and > > FOREIGN KEY constraints as NOT ENFORCED. > > Thanks for working on this! > > > Adding NOT ENFORCED to CHECK constraints is simple, see 0001 patch, > > I've looked at the 0001 patch and think it looks simple and straight forward. > Thanks for looking into it. > > but implementing it for FOREIGN KEY constraints requires more code due > > to triggers, see 0002 - 0005 patches. > > I can't say that much yet about the code changes in 0002 - 0005 yet, > but I've tested the patches and successfully experimented with the feature. > > Also think the documentation is good and sound. Only found a minor typo: > - Adding a enforced <literal>CHECK</literal> or <literal>NOT NULL</literal> > + Adding an enforced <literal>CHECK</literal> or <literal>NOT NULL</literal> > Ok, will fix it in the next version. > > There are various approaches for > > implementing NOT ENFORCED foreign keys, what I thought of: > > > > 1. When defining a NOT ENFORCED foreign key, skip the creation of > > triggers used for referential integrity check, while defining an > > ENFORCED foreign key, remain the same as the current behaviour. If an > > existing foreign key is changed to NOT ENFORCED, the triggers are > > dropped, and when switching it back to ENFORCED, the triggers are > > recreated. > > > > 2. Another approach could be to create the NOT ENFORCED constraint > > with the triggers as usual, but disable those triggers by updating the > > pg_trigger catalog so that they are never executed for the check. And > > enable them when the constraint is changed back to ENFORCED. > > > > 3. Similarly, a final approach would involve updating the logic where > > trigger execution is decided and skipping the execution if the > > constraint is not enforced, rather than modifying the pg_trigger > > catalog. > > > > In the attached patch, the first approach has been implemented. This > > requires more code changes but prevents unused triggers from being > > left in the database and avoids the need for changes all over the > > place to skip trigger execution, which could be missed in future code > > additions. > > I also like the first approach, since I think it's nice the pg_trigger > entires are inserted / deleted upon enforced / not enforced. > > > The ALTER CONSTRAINT operation in the patch added code to handle > > dropping and recreating triggers. An alternative approach would be to > > simplify the process by dropping and recreating the FK constraint, > > which would automatically handle skipping or creating triggers for NOT > > ENFORCED or ENFORCED FK constraints. However, I wasn't sure if this > > was the right approach, as I couldn't find any existing ALTER > > operations that follow this pattern. > > I think the current approach of dropping and recreating the triggers is best, > since if we would instead be dropping and recreating the FK constraint, > that would cause problems if some other future SQL feature would need to > introduce dependencies on the FK constraints via pg_depend. > Yes, that was my initial thought as well, and recreating the dependencies would be both painful and prone to bugs. Regards, Amul
On Wed, Oct 9, 2024 at 6:45 PM Andrew Dunstan <andrew@dunslane.net> wrote: > > > On 2024-10-09 We 5:14 AM, Joel Jacobson wrote: > > On Tue, Oct 8, 2024, at 11:06, Amul Sul wrote: > > The attached patch proposes adding the ability to define CHECK and > FOREIGN KEY constraints as NOT ENFORCED. > > Thanks for working on this! > > Adding NOT ENFORCED to CHECK constraints is simple, see 0001 patch, > > I've looked at the 0001 patch and think it looks simple and straight forward. > > but implementing it for FOREIGN KEY constraints requires more code due > to triggers, see 0002 - 0005 patches. > > I can't say that much yet about the code changes in 0002 - 0005 yet, > but I've tested the patches and successfully experimented with the feature. > > Also think the documentation is good and sound. Only found a minor typo: > - Adding a enforced <literal>CHECK</literal> or <literal>NOT NULL</literal> > + Adding an enforced <literal>CHECK</literal> or <literal>NOT NULL</literal> > > There are various approaches for > implementing NOT ENFORCED foreign keys, what I thought of: > > 1. When defining a NOT ENFORCED foreign key, skip the creation of > triggers used for referential integrity check, while defining an > ENFORCED foreign key, remain the same as the current behaviour. If an > existing foreign key is changed to NOT ENFORCED, the triggers are > dropped, and when switching it back to ENFORCED, the triggers are > recreated. > > 2. Another approach could be to create the NOT ENFORCED constraint > with the triggers as usual, but disable those triggers by updating the > pg_trigger catalog so that they are never executed for the check. And > enable them when the constraint is changed back to ENFORCED. > > 3. Similarly, a final approach would involve updating the logic where > trigger execution is decided and skipping the execution if the > constraint is not enforced, rather than modifying the pg_trigger > catalog. > > In the attached patch, the first approach has been implemented. This > requires more code changes but prevents unused triggers from being > left in the database and avoids the need for changes all over the > place to skip trigger execution, which could be missed in future code > additions. > > I also like the first approach, since I think it's nice the pg_trigger > entires are inserted / deleted upon enforced / not enforced. > > > > I also prefer this, as it gets us out from any possible dance with enabling / disabling triggers. > Agreed. Thanks for the inputs. Regards, Amul.
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).
On 18.11.24 13:42, jian he wrote:
> i only played around with
> v4-0001-Add-support-for-NOT-ENFORCED-in-CHECK-constraints.patch.
>
> create table t(a int);
> alter table t ADD CONSTRAINT the_constraint CHECK (a > 0) NOT ENFORCED;
> insert into t select -1;
> select conname, contype,condeferrable,condeferred, convalidated,
> conenforced,conkey,connoinherit
> from pg_constraint
> where conrelid = 't'::regclass;
>
> pg_constraint->convalidated should be set to false for NOT ENFORCED constraint?
> Am I missing something?
The "validated" status is irrelevant when the constraint is set to not
enforced. But it's probably still a good idea to make sure the field is
set consistently. I'm also leaning toward setting it to false. One
advantage of that would be that if you set the constraint to enforced
later, then it's automatically in the correct "not validated" state.
> <varlistentry id="sql-createtable-parms-enforce">
> <term><literal>ENFORCED</literal></term>
> <term><literal>NOT ENFORCED</literal></term>
> <listitem>
> <para>
> This is currently only allowed for <literal>CHECK</literal> constraints.
> If the constraint is <literal>NOT ENFORCED</literal>, this clause
> specifies that the constraint check will be skipped. When the constraint
> is <literal>ENFORCED</literal>, check is performed after each statement.
> This is the default.
> </para>
> </listitem>
> </varlistentry>
> "This is the default." kind of ambiguous?
> I think you mean: by default, all constraints are implicit ENFORCED.
Maybe "the latter is the default" would be clearer.
> + ereport(ERROR,
> + (errcode(ERRCODE_SYNTAX_ERROR),
> + errmsg("misplaced ENFORCED clause"),
> + parser_errposition(cxt->pstate, con->location)));
>
> + ereport(ERROR,
> + (errcode(ERRCODE_SYNTAX_ERROR),
> + errmsg("misplaced NOT ENFORCED clause"),
> + parser_errposition(cxt->pstate, con->location)));
>
> https://www.merriam-webster.com/dictionary/misplace
> says:
> "to put in a wrong or inappropriate place"
>
> I found the "misplaced" error message is not helpful.
> for example:
> CREATE TABLE UNIQUE_EN_TBL(i int UNIQUE ENFORCED);
> ERROR: misplaced ENFORCED clause
> the error message only tells us thatspecify ENFORCED is wrong.
> but didn't say why it's wrong.
>
> we can saying that
> "ENFORCED clauses can only be used for CHECK constraints"
This handling is similar to other error messages in
transformConstraintAttrs(). It could be slightly improved, but it's not
essential for this patch.
> ------------------------------------------------------------------
> the following queries is a bug?
>
> drop table t;
> create table t(a int);
> alter table t ADD CONSTRAINT cc CHECK (a > 0) NOT ENFORCED;
> insert into t select -1;
> alter table t add constraint cc1 check (a > 1) not ENFORCED not ENFORCED;
> ERROR: check constraint "cc1" of relation "t" is violated by some row
> alter table t add constraint cc1 check (a > 1) not ENFORCED;
> ERROR: check constraint "cc1" of relation "t" is violated by some row
>
> ------------------------------------------------------------------
> drop table t;
> create table t(a int);
> alter table t ADD CONSTRAINT cc CHECK (a > 0) NOT ENFORCED not enforced;
>
> seems not easy to make it fail with alter table multiple "not enforced".
> I guess it should be fine.
> since we disallow a mix of "not enforced" and "enforced".
>
> alter table t ADD CONSTRAINT cc CHECK (a > 0) NOT ENFORCED enforced;
> ------------------------------------------------------------------
Hmm, these duplicate clauses should have been caught by
transformConstraintAttrs().
> typedef struct Constraint
> {
> NodeTag type;
> ConstrType contype; /* see above */
> char *conname; /* Constraint name, or NULL if unnamed */
> bool deferrable; /* DEFERRABLE? */
> bool initdeferred; /* INITIALLY DEFERRED? */
> bool skip_validation; /* skip validation of existing rows? */
> bool initially_valid; /* mark the new constraint as valid? */
> bool is_enforced; /* enforced constraint? */
> }
> makeNode(Constraint) will default is_enforced to false.
> Which makes the default value not what we want.
> That means we may need to pay more attention for the trip from
> makeNode(Constraint) to finally insert the constraint to the catalog.
>
> if we change it to is_not_enforced, makeNode will default to false.
> is_not_enforced is false, means the constraint is enforced.
> which is not that intuitive...
Yes, it could be safer to make the field so that the default is false.
I guess the skip_validation field is like that for a similar reason, but
I'm not sure.
> ------------------------------------------------------------------
> do we need to update for "enforced" in
> https://www.postgresql.org/docs/current/sql-keywords-appendix.html
> ?
> ------------------------------------------------------------------
That is generated automatically.
> seems don't have
> ALTER TABLE <name> VALIDATE CONSTRAINT
> interacts with not forced sql tests.
> for example:
>
> drop table if exists t;
> create table t(a int);
> alter table t add constraint cc check (a <> 1) not enforced NOT VALID;
> insert into t values(1); ---success.
> alter table t validate constraint cc;
>
> select conname,convalidated, conenforced
> from pg_constraint
> where conrelid = 't'::regclass;
>
> returns:
> conname | convalidated | conenforced
> ---------+--------------+-------------
> cc | t | f
>
> Now we have a value in the table "t" that violates the check
> constraint, while convalidated is true.
> ----------------------------------------------------------------------------
I think we should prevent running VALIDATE for not enforced constraints.
I don't know what that would otherwise mean.
It's also questionable whether NOT VALID makes sense to specify.