> On Jun 2, 2026, at 14:24, Chao Li <[email protected]> wrote: > > > >> On Jun 1, 2026, at 23:51, jian he <[email protected]> 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/
v6-0001-Prevent-inherited-CHECK-constraints-from-being-we.patch
Description: Binary data
v6-0002-doc-Clarify-inherited-constraint-behavior.patch
Description: Binary data
v6-0003-doc-Clarify-ALTER-CONSTRAINT-enforceability-behav.patch
Description: Binary data
