| From: | Chao Li <li(dot)evan(dot)chao(at)gmail(dot)com> |
|---|---|
| To: | jian he <jian(dot)universality(at)gmail(dot)com> |
| Cc: | Álvaro Herrera <alvherre(at)kurilemu(dot)de>, "L(dot) pgsql-hackers" <pgsql-hackers(at)lists(dot)postgresql(dot)org>, Andrew Dunstan <andrew(at)dunslane(dot)net> |
| Subject: | Re: Fix bug of CHECK constraint enforceability recursion |
| Date: | 2026-06-02 06:24:01 |
| Message-ID: | 14E223D8-8425-446A-A36C-6B62BC334656@gmail.com |
| Views: | Whole Thread | Raw Message | Download mbox | Resend email |
| Thread: | |
| Lists: | pgsql-hackers |
> On Jun 1, 2026, at 23:51, jian he <jian(dot)universality(at)gmail(dot)com> wrote:
>
> Hi.
>
> I did some refactoring based on your v3, I didn't change your comments.
>
> I don't think ATGetEquivalentCheckConstraintOid is necessary.
> ATCheckCheckConstrHasEnforcedParent,
> AlterCheckConstrEnforceabilityRecurse is enough for this context.
I agree that ATGetEquivalentCheckConstraintOid is not needed. I was too worried that looking up descendant constraints by name alone was not good enough. But now I realize that users have no supported way to define a conflicting constraint on a child with the same name as a parent’s inheritable CHECK constraint. In v5, I removed ATGetEquivalentCheckConstraintOid and switched to get_relation_constraint_oid.
>
> + changing_conids = lappend_oid(list_copy(changing_conids),
> + currcon->oid);
> Refactor changing_conids to List ** to allow direct modification,
> aligning with existing code conventions.
The v3 code was a little confusing. In fact, changing_conids only needs to be built at the top level, so list_copy() is not needed, and List ** is not needed either. I refactored this part in v5.
>
> CREATE TABLE ... PARTITION OF automatically copies the parent's status, and
> ALTER TABLE ... ATTACH PARTITION already rejects cases where the parent is
> enforced but the child is not. If we also reject directly altering a partition's
> constraint enforcement status, then we need no longer worry about cases where
> parent being enforced while a child is not. Therefore, invoking
> ATCheckCheckConstrHasEnforcedParent just once for table partitioning is safe.
>
>
>
> --
> jian
> https://www.enterprisedb.com/
> <v4-0001-Prevent-inherited-CHECK-constraints-from-being-weakened.patch>
v4 is similar to my original implementation when I worked on v2, and it does not handle some complicated inheritance cases correctly. The following 2 tests are based on v4.
1
```
evantest=# create table root(a int constraint ck check (a > 0) enforced);
CREATE TABLE
evantest=# create table ext(a int constraint ck check (a > 0) enforced);
CREATE TABLE
evantest=# create table mixed() inherits (root, ext);
NOTICE: merging multiple inherited definitions of column "a"
CREATE TABLE
evantest=# create table ordinary() inherits (root);
CREATE TABLE
evantest=# alter table root alter constraint ck not enforced;
ALTER TABLE
evantest=# select conrelid::regclass, conname, conenforced, coninhcount, conislocal from pg_constraint where conname='ck';
conrelid | conname | conenforced | coninhcount | conislocal
----------+---------+-------------+-------------+------------
root | ck | f | 0 | t
ext | ck | t | 0 | t
mixed | ck | t | 2 | f
ordinary | ck | t | 1 | f
(4 rows)
```
In this test, ordinary only inherits from root, so when root is changed to NOT ENFORCED, ordinary should be changed as well. However, ordinary incorrectly remains ENFORCED.
2
```
evantest=# create table root(a int constraint ck check (a > 0) enforced);
CREATE TABLE
evantest=# create table child() inherits (root);
CREATE TABLE
evantest=# create table intermediate() inherits (root);
CREATE TABLE
evantest=# alter table child inherit intermediate;
ALTER TABLE
evantest=# alter table root alter constraint ck not enforced;
ALTER TABLE
evantest=# select conrelid::regclass, conname, conenforced, coninhcount, conislocal from pg_constraint where conname='ck';
conrelid | conname | conenforced | coninhcount | conislocal
--------------+---------+-------------+-------------+------------
root | ck | f | 0 | t
child | ck | t | 2 | f
intermediate | ck | t | 1 | f
(3 rows)
```
This is a more complicated case. The tricky part is that child is altered to inherit from intermediate, but child was created before intermediate, so child has a smaller OID than intermediate, which affects the recursion order.
Ultimately, both child and intermediate inherit from root, so after root is changed to NOT ENFORCED, they should be changed to NOT ENFORCED as well. But in this test, they incorrectly remain ENFORCED.
Both tests pass with v3 and v5:
PFA v5:
0001:
* Removed ATGetEquivalentCheckConstraintOid
* Refactored how changing_conids is built
* Added two test cases that Jian added to v4
* Added the complicated inheritance test cases shown above
0002: Same as v3-0002; fixes the documentation issue Alvaro pointed out earlier
0003: The documentation change from [1], which clarifies the ALTER TABLE doc about constraint enforceability
BTW, Jian, next time, would you mind attaching a diff on top of the previous patch version? That would make your changes easier to identify.
[1] https://www.postgresql.org/message-id/711B1ED3-1781-4B6C-A573-B58AF20770E5%40gmail.com
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
| Attachment | Content-Type | Size |
|---|---|---|
| v5-0001-Prevent-inherited-CHECK-constraints-from-being-we.patch | application/octet-stream | 23.0 KB |
| v5-0002-doc-Clarify-inherited-constraint-behavior.patch | application/octet-stream | 3.7 KB |
| v5-0003-doc-Clarify-ALTER-CONSTRAINT-enforceability-behav.patch | application/octet-stream | 2.1 KB |
| From | Date | Subject | |
|---|---|---|---|
| Next Message | Andrei Lepikhov | 2026-06-02 06:25:46 | Re: hashjoins vs. Bloom filters (yet again) |
| Previous Message | Amit Kapila | 2026-06-02 06:14:46 | Re: pg_createsubscriber: allow duplicate publication names |