Re: NOT ENFORCED constraint feature

On 03.12.24 13:00, Amul Sul wrote:
>>> 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.

Let's make it so that ruleutils.c doesn't print the NOT VALID when it's
already printing NOT ENFORCED. Otherwise, it gets unnecessarily verbose
and confusing.

>>> 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.
>>
>
> Ok. Initially, I was doing it the same way, but to maintain consistency
> with the pg_constraint column and avoid negation in multiple places, I
> chose that approach. However, I agree that having the default to false
> would be safer. I’ve renamed the flag to is_not_enforced. Other names
> I considered were not_enforced or is_unenforced, but since we already
> have existing flags with two underscores, is_not_enforced shouldn't be
> a problem.

I was initially thinking about this as well, but after seeing it now, I
don't think this is a good change. Because now we have both "enforced"
and "not_enforced" sprinkled around the code. If we were to do this
consistently everywhere, then it might make sense, but this way it's
just confusing. The Constraint struct is only initialized in a few
places, so I think we can be careful there. Also note that the field
initially_valid is equally usually true.

I could of other notes on patch 0001:

Update information_schema table_constraint.enforced (see
src/backend/catalog/information_schema.sql and
doc/src/sgml/information_schema.sgml).

The handling of merging check constraints seems incomplete. What should
be the behavior of this:

=> create table p1 (a int check (a > 0) not enforced);
CREATE TABLE
=> create table c1 (a int check (a > 0) enforced) inherits (p1);
CREATE TABLE

Or this?

=> create table p2 (a int check (a > 0) enforced);
CREATE TABLE
=> create table c2 () inherits (p1, p2);
CREATE TABLE

Should we catch these and error?