Re: Fix bug of CHECK constraint enforceability recursion

> On May 26, 2026, at 16:32, jian he <jian(dot)universality(at)gmail(dot)com> wrote:
>
> On Tue, May 26, 2026 at 3:47 PM Chao Li <li(dot)evan(dot)chao(at)gmail(dot)com> wrote:
>>
>>>
>>> I think this is a bug that we need to fix in 19 as well — I mean we should reject the ALTER TABLE.
>>>
>>> --
>>> Álvaro Herrera
>>
>> Thanks for your comment. Let me rework the patch.
>>
>
> Hi.
> Here are the comments placed in ATExecAlterCheckConstrEnforceability I
> came up with:
>
> + /*
> + * If the check constraint qual definitions match but their enforcement
> + * statuses conflict (parent enforced, child unenforced), it creates
> + * ambiguity around how insert operations should handle the mismatch.
> + * Therefore, we should avoid states where the parent check constraint is
> + * enforced while the child is not. We actually enforced this within
> + * MergeConstraintsIntoExisting and MergeWithExistingConstraint.
> + */
> + if (currcon->coninhcount > 0 && !recursing)
> + ereport(ERROR,
> + errcode(ERRCODE_INVALID_TABLE_DEFINITION),
> + errmsg("cannot alter inherited constraint \"%s\" of
> relation \"%s\" enforciability",
> + NameStr(currcon->conname),
> RelationGetRelationName(rel)));
>
>
>
> --
> jian
> https://www.enterprisedb.com/
> <v1-0001-disallow-alter-enforciability-of-inherited-check-constraint.patch>

Hi Jian,

Thanks for your help. Your implementation is simple and clever:
```
+ if (currcon->coninhcount > 0 && !recursing)
+ ereport(ERROR,
+ errcode(ERRCODE_INVALID_TABLE_DEFINITION),
+ errmsg("cannot alter inherited constraint \"%s\" of relation \"%s\" enforciability",
+ NameStr(currcon->conname), RelationGetRelationName(rel)));
```

Basically, it disallows all enforceability changes on inherited constraints (currcon->coninhcount > 0) at the recursion root (!recursing), or in other words, it disallows the operation on any child table.

But I see several problems with this implementation:

1. As you pointed out earlier, when a parent is ENFORCED, changing a child from ENFORCED to NOT ENFORCED should not be allowed. But when a parent is NOT ENFORCED, changing a child from NOT ENFORCED to ENFORCED should be allowed. The existing phase 3 checking also proves that.

2. Suppose a parent table is NOT ENFORCED, and a user changes a child from NOT ENFORCED to ENFORCED, which is allowed. Later, if the user wants to change the child back from ENFORCED to NOT ENFORCED, that should also be allowed. But with your v1 patch, the user would have to do the change through the parent table, which I think hurts the user experience.

3. Suppose a child table is already ENFORCED, and a user issues a command to change it to ENFORCED again, which is actually a no-op. PostgreSQL usually allows this kind of no-op, but with your v1 patch, this no-op would get an error as well, which I think also hurts the user experience.

4. It cannot handle some complicated inheritance hierarchies. For example, the following test passes with your v1:
```
evantest=# CREATE TABLE p1 (a int CONSTRAINT c CHECK (a > 0) ENFORCED);
CREATE TABLE
evantest=# CREATE TABLE p2 (a int CONSTRAINT c CHECK (a > 0) ENFORCED);
CREATE TABLE
evantest=#
evantest=# CREATE TABLE ch () INHERITS (p1, p2);
NOTICE: merging multiple inherited definitions of column "a"
CREATE TABLE
evantest=# ALTER TABLE p1 ALTER CONSTRAINT c NOT ENFORCED;
ALTER TABLE
```

I originally thought this should fail, but it now changes ch.c to NOT ENFORCED, so it breaks the rule because its parent p2 is still ENFORCED:
```
evantest=# SELECT conrelid::regclass, conname, conenforced, coninhcount, conislocal
evantest-# FROM pg_constraint WHERE conname = 'c';
conrelid | conname | conenforced | coninhcount | conislocal
----------+---------+-------------+-------------+------------
p1 | c | f | 0 | t
p2 | c | t | 0 | t
ch | c | f | 2 | f
(3 rows)
```

Then I realized that the initial CREATE TABLE case passes:
```
evantest=# CREATE TABLE p1 (a int CONSTRAINT c CHECK (a > 0) NOT ENFORCED);
CREATE TABLE
evantest=# CREATE TABLE p2 (a int CONSTRAINT c CHECK (a > 0) ENFORCED);
CREATE TABLE
evantest=# CREATE TABLE ch () INHERITS (p1, p2);
NOTICE: merging multiple inherited definitions of column "a"
CREATE TABLE
evantest=# SELECT conrelid::regclass, conname, conenforced, coninhcount, conislocal
evantest-# FROM pg_constraint WHERE conname = ‘c';
conrelid | conname | conenforced | coninhcount | conislocal
----------+---------+-------------+-------------+------------
ch | c | t | 2 | f
p1 | c | f | 0 | t
p2 | c | t | 0 | t
(3 rows)
```

When the two parents have different enforceability, the stricter one is applied to the child. So I think the test above in item 4 should also perform similar merge logic rather than fail. This seems to uncover a new issue in the original feature patch.

For the fix, my design is:

* Directly reject changing an inherited child CHECK constraint to NOT ENFORCED if an equivalent parent constraint remains ENFORCED.
* Changing a child to ENFORCED is allowed.
* During recursing, if a child also inherits an equivalent ENFORCED constraint from another parent outside the current ALTER, the child keeps the stricter ENFORCED state.

Please see my implementation in the attached v2 patch.

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/