Re: Fix bug of CHECK constraint enforceability recursion - Mailing list pgsql-hackers
| From | Chao Li |
|---|---|
| Subject | Re: Fix bug of CHECK constraint enforceability recursion |
| Date | |
| Msg-id | 9A3D388C-1DF4-4C43-9AB6-83529A8F48C8@gmail.com Whole thread |
| In response to | Re: Fix bug of CHECK constraint enforceability recursion (Chao Li <li.evan.chao@gmail.com>) |
| Responses |
Re: Fix bug of CHECK constraint enforceability recursion
|
| List | pgsql-hackers |
> On Jun 2, 2026, at 14:24, Chao Li <li.evan.chao@gmail.com> wrote: > > > >> On Jun 1, 2026, at 23:51, jian he <jian.universality@gmail.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 constraintsby name alone was not good enough. But now I realize that users have no supported way to define a conflictingconstraint on a child with the same name as a parent’s inheritable CHECK constraint. In v5, I removed ATGetEquivalentCheckConstraintOidand 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() isnot 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 casescorrectly. 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 aswell. 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 createdbefore 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 changedto 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 changeseasier 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
pgsql-hackers by date: