Re: Fix bug of CHECK constraint enforceability recursion

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:39:21
Message-ID: 9A3D388C-1DF4-4C43-9AB6-83529A8F48C8@gmail.com
Views: Whole Thread | Raw Message | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

> On Jun 2, 2026, at 14:24, Chao Li <li(dot)evan(dot)chao(at)gmail(dot)com> wrote:
>
>
>
>> 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/
>
>
>
>
> <v5-0001-Prevent-inherited-CHECK-constraints-from-being-we.patch><v5-0002-doc-Clarify-inherited-constraint-behavior.patch><v5-0003-doc-Clarify-ALTER-CONSTRAINT-enforceability-behav.patch>

Oops! I just found that I forgot to commit a tiny comment tuning in 0001. So posting v6.

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

Attachment Content-Type Size
v6-0001-Prevent-inherited-CHECK-constraints-from-being-we.patch application/octet-stream 23.0 KB
v6-0002-doc-Clarify-inherited-constraint-behavior.patch application/octet-stream 3.7 KB
v6-0003-doc-Clarify-ALTER-CONSTRAINT-enforceability-behav.patch application/octet-stream 2.1 KB

In response to

Browse pgsql-hackers by date

  From Date Subject
Next Message Dilip Kumar 2026-06-02 06:46:50 Re: Proposal: Conflict log history table for Logical Replication
Previous Message Michael Paquier 2026-06-02 06:30:40 Re: Improve pg_stat_statements scalability