From cd328fe399d5c322dfab0d52eac8280c0b1e1068 Mon Sep 17 00:00:00 2001 From: "Chao Li (Evan)" Date: Thu, 30 Jul 2026 16:03:49 +0800 Subject: [PATCH v1] Add find_all_inheritors_ordered() Add an inheritance traversal that guarantees every ancestor appears before its descendants. find_all_inheritors() does not provide this ordering when multiple inheritance creates both direct and indirect paths to a relation. Use Kahn's topological sorting algorithm to produce the ordered list without recursion. Use the new function to simplify the inherited CHECK constraint enforceability handling committed by 0cd17fdd3c0. Since parents are processed before their children, checking the current state of a child's direct parents is sufficient. Extend the direct-plus-indirect inheritance test with another intermediate level. Author: Chao Li --- src/backend/catalog/pg_inherits.c | 128 ++++++++++++++++++++++++++ src/backend/commands/tablecmds.c | 110 +++++----------------- src/include/catalog/pg_inherits.h | 1 + src/test/regress/expected/inherit.out | 9 +- src/test/regress/sql/inherit.sql | 3 +- 5 files changed, 159 insertions(+), 92 deletions(-) diff --git a/src/backend/catalog/pg_inherits.c b/src/backend/catalog/pg_inherits.c index 4b9802aafcc..db38ddf9466 100644 --- a/src/backend/catalog/pg_inherits.c +++ b/src/backend/catalog/pg_inherits.c @@ -29,6 +29,7 @@ #include "utils/builtins.h" #include "utils/fmgroids.h" #include "utils/hsearch.h" +#include "utils/memutils.h" #include "utils/snapmgr.h" #include "utils/syscache.h" @@ -41,6 +42,16 @@ typedef struct SeenRelsEntry int list_index; /* its position in output list(s) */ } SeenRelsEntry; +/* + * Entry of a hash table used in find_all_inheritors_ordered. + */ +typedef struct OrderedSeenRelsEntry +{ + Oid rel_id; + List *children; + int indegree; +} OrderedSeenRelsEntry; + /* * find_inheritance_children * @@ -335,6 +346,123 @@ find_all_inheritors(Oid parentrelId, LOCKMODE lockmode, List **numparents) return rels_list; } +/* + * find_all_inheritors_ordered - + * Same as find_all_inheritors(), except that an ancestor is always listed + * before its descendants. + * + * The ordering is produced using Kahn's topological sorting algorithm. + */ +List * +find_all_inheritors_ordered(Oid parentrelId, LOCKMODE lockmode) +{ + MemoryContext temp_context; + MemoryContext old_context; + HTAB *seen_rels; + HASHCTL ctl; + List *agenda; + List *worklist = NIL; + List *ordered = NIL; + List *result = NIL; + ListCell *lc; + OrderedSeenRelsEntry *node; + bool found; + + /* Use a temporary memory context to simplify cleanup */ + temp_context = AllocSetContextCreate(CurrentMemoryContext, + "find_all_inheritors_ordered", + ALLOCSET_SMALL_SIZES); + old_context = MemoryContextSwitchTo(temp_context); + + ctl.keysize = sizeof(Oid); + ctl.entrysize = sizeof(OrderedSeenRelsEntry); + ctl.hcxt = temp_context; + + seen_rels = hash_create("find_all_inheritors_ordered temporary table", + 32, /* start small and extend */ + &ctl, + HASH_ELEM | HASH_BLOBS | HASH_CONTEXT); + + node = hash_search(seen_rels, &parentrelId, HASH_ENTER, &found); + Assert(!found); + node->children = NIL; + node->indegree = 0; + + /* + * Use the agenda to find every descendant and record every inheritance + * edge. The indegree is the number of direct parents within this + * inheritance graph. + */ + agenda = list_make1_oid(parentrelId); + foreach(lc, agenda) + { + Oid current_oid = lfirst_oid(lc); + List *children; + OrderedSeenRelsEntry *current; + + current = hash_search(seen_rels, ¤t_oid, HASH_FIND, NULL); + Assert(current != NULL); + + children = find_inheritance_children(current_oid, lockmode); + foreach_oid(child_oid, children) + { + node = hash_search(seen_rels, &child_oid, HASH_ENTER, &found); + if (!found) + { + node->children = NIL; + node->indegree = 0; + agenda = lappend_oid(agenda, child_oid); + } + + current->children = lappend_oid(current->children, child_oid); + node->indegree++; + } + list_free(children); + } + + /* + * Emit nodes in topological order. A node enters the worklist only after + * all of its direct parents have been emitted. + */ + foreach_oid(rel_oid, agenda) + { + node = hash_search(seen_rels, &rel_oid, HASH_FIND, NULL); + Assert(node != NULL); + if (node->indegree == 0) + worklist = lappend_oid(worklist, rel_oid); + } + + /* + * Move nodes from the worklist to the output list, and add their children + * to the worklist when all of their parents have been emitted. + */ + foreach_oid(rel_oid, worklist) + { + node = hash_search(seen_rels, &rel_oid, HASH_FIND, NULL); + Assert(node != NULL); + ordered = lappend_oid(ordered, rel_oid); + + foreach_oid(child_oid, node->children) + { + OrderedSeenRelsEntry *child; + + child = hash_search(seen_rels, &child_oid, HASH_FIND, NULL); + Assert(child != NULL); + Assert(child->indegree > 0); + if (--child->indegree == 0) + worklist = lappend_oid(worklist, child_oid); + } + } + + Assert(list_length(ordered) == list_length(agenda)); + + MemoryContextSwitchTo(old_context); + result = list_copy(ordered); + MemoryContextDelete(temp_context); + + return result; +} + /* * has_subclass - does this relation have any children? diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index 6d4c457b820..2aa5724dcae 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -437,7 +437,6 @@ static bool ATExecAlterFKConstrEnforceability(List **wqueue, ATAlterConstraint * static bool ATExecAlterCheckConstrEnforceability(List **wqueue, ATAlterConstraint *cmdcon, Relation conrel, HeapTuple contuple, bool recurse, bool recursing, - List *changing_conids, LOCKMODE lockmode); static bool ATExecAlterConstrDeferrability(List **wqueue, ATAlterConstraint *cmdcon, Relation conrel, Relation tgrel, Relation rel, @@ -460,7 +459,6 @@ static void AlterFKConstrEnforceabilityRecurse(List **wqueue, ATAlterConstraint static void AlterCheckConstrEnforceabilityRecurse(List **wqueue, ATAlterConstraint *cmdcon, Relation conrel, Oid conrelid, bool recurse, bool recursing, - List *changing_conids, LOCKMODE lockmode); static void AlterConstrDeferrabilityRecurse(List **wqueue, ATAlterConstraint *cmdcon, Relation conrel, Relation tgrel, Relation rel, @@ -470,7 +468,6 @@ static void AlterConstrUpdateConstraintEntry(ATAlterConstraint *cmdcon, Relation HeapTuple contuple); static bool ATCheckCheckConstrHasEnforcedParent(Relation conrel, Relation rel, HeapTuple contuple, - List *changing_conids, Oid *enforced_parentoid); static ObjectAddress ATExecValidateConstraint(List **wqueue, Relation rel, char *constrName, @@ -12529,7 +12526,7 @@ ATExecAlterConstraintInternal(List **wqueue, ATAlterConstraint *cmdcon, else if (currcon->contype == CONSTRAINT_CHECK) changed = ATExecAlterCheckConstrEnforceability(wqueue, cmdcon, conrel, contuple, recurse, false, - NIL, lockmode); + lockmode); } else if (cmdcon->alterDeferrability && ATExecAlterConstrDeferrability(wqueue, cmdcon, conrel, tgrel, rel, @@ -12717,7 +12714,6 @@ static bool ATExecAlterCheckConstrEnforceability(List **wqueue, ATAlterConstraint *cmdcon, Relation conrel, HeapTuple contuple, bool recurse, bool recursing, - List *changing_conids, LOCKMODE lockmode) { Form_pg_constraint currcon; @@ -12759,7 +12755,6 @@ ATExecAlterCheckConstrEnforceability(List **wqueue, ATAlterConstraint *cmdcon, if (!cmdcon->is_enforced && (!recursing || !rel->rd_rel->relispartition) && ATCheckCheckConstrHasEnforcedParent(conrel, rel, contuple, - changing_conids, &enforced_parentoid)) { if (!recursing) @@ -12786,6 +12781,14 @@ ATExecAlterCheckConstrEnforceability(List **wqueue, ATAlterConstraint *cmdcon, updatecon.is_enforced = target_enforced; AlterConstrUpdateConstraintEntry(&updatecon, conrel, contuple); changed = true; + + /* + * A later descendant must see this change when checking its direct + * parents. The ordered inheritance traversal guarantees that all + * affected parents are processed before their children. + */ + if (!cmdcon->is_enforced) + CommandCounterIncrement(); } /* @@ -12806,55 +12809,20 @@ ATExecAlterCheckConstrEnforceability(List **wqueue, ATAlterConstraint *cmdcon, */ if (!recursing && !currcon->connoinherit) { - Assert(changing_conids == NIL); - - children = find_all_inheritors(RelationGetRelid(rel), - lockmode, NULL); + children = find_all_inheritors_ordered(RelationGetRelid(rel), + lockmode); /* - * When setting NOT ENFORCED, build the set of equivalent CHECK - * constraints that this command will attempt to change before - * visiting descendants. The root itself has already been checked - * above. + * If we are told not to recurse, there had better not be any + * child tables, because we can't change constraint enforceability + * on the parent unless we have changed enforceability for all + * children. */ - if (!cmdcon->is_enforced) - changing_conids = list_make1_oid(currcon->oid); - - foreach_oid(childoid, children) - { - if (childoid == RelationGetRelid(rel)) - continue; - - /* - * If we are told not to recurse, there had better not be any - * child tables, because we can't change constraint - * enforceability on the parent unless we have changed - * enforceability for all child. - */ - if (!recurse) - ereport(ERROR, - errcode(ERRCODE_INVALID_TABLE_DEFINITION), - errmsg("constraint must be altered on child tables too"), - errhint("Do not specify the ONLY keyword.")); - - /* - * It is sufficient to look up the constraint by name here. - * Supported DDL ensures that inheritable CHECK constraints - * with the same name have equivalent definitions when they - * are propagated to children or when inheritance is - * established. All descendants returned by - * find_all_inheritors must have this constraint: inherited - * CHECK constraints propagate to all children at - * inheritance-link creation time and cannot be dropped - * independently on child tables. - */ - if (!cmdcon->is_enforced) - changing_conids = - list_append_unique_oid(changing_conids, - get_relation_constraint_oid(childoid, - cmdcon->conname, - false)); - } + if (!recurse && list_length(children) > 1) + ereport(ERROR, + errcode(ERRCODE_INVALID_TABLE_DEFINITION), + errmsg("constraint must be altered on child tables too"), + errhint("Do not specify the ONLY keyword.")); } foreach_oid(childoid, children) @@ -12864,7 +12832,6 @@ ATExecAlterCheckConstrEnforceability(List **wqueue, ATAlterConstraint *cmdcon, AlterCheckConstrEnforceabilityRecurse(wqueue, cmdcon, conrel, childoid, false, true, - changing_conids, lockmode); } } @@ -12916,7 +12883,6 @@ static void AlterCheckConstrEnforceabilityRecurse(List **wqueue, ATAlterConstraint *cmdcon, Relation conrel, Oid conrelid, bool recurse, bool recursing, - List *changing_conids, LOCKMODE lockmode) { SysScanDesc pscan; @@ -12946,21 +12912,18 @@ AlterCheckConstrEnforceabilityRecurse(List **wqueue, ATAlterConstraint *cmdcon, cmdcon->conname, get_rel_name(conrelid))); ATExecAlterCheckConstrEnforceability(wqueue, cmdcon, conrel, childtup, - recurse, recursing, changing_conids, - lockmode); + recurse, recursing, lockmode); systable_endscan(pscan); } /* * When setting an inherited CHECK constraint to NOT ENFORCED, look for a - * matching parent constraint that remains ENFORCED and is not part of the same - * ALTER. + * matching parent constraint that remains ENFORCED. */ static bool ATCheckCheckConstrHasEnforcedParent(Relation conrel, Relation rel, HeapTuple contuple, - List *changing_conids, Oid *enforced_parentoid) { Form_pg_constraint currcon; @@ -12990,7 +12953,6 @@ ATCheckCheckConstrHasEnforcedParent(Relation conrel, Relation rel, while (HeapTupleIsValid(inheritsTuple = systable_getnext(scan))) { Oid parentoid; - Relation parentrel = NULL; SysScanDesc pscan; ScanKeyData pkey[3]; HeapTuple parenttup; @@ -13034,41 +12996,13 @@ ATCheckCheckConstrHasEnforcedParent(Relation conrel, Relation rel, RelationGetRelationName(rel), NameStr(parentcon->conname)); - /* - * A parent listed in changing_conids is being changed by the same - * ALTER, but it may not have been updated yet. For regular - * inheritance, recurse upward to check whether an equivalent - * enforced parent outside the ALTER will make it remain enforced. - * Partitions cannot have multiple parents, so they do not need - * this check. - */ - if (!rel->rd_rel->relispartition && - list_member_oid(changing_conids, parentcon->oid)) - { - Oid parent_enforced_parentoid = InvalidOid; - - if (parentrel == NULL) - parentrel = table_open(parentoid, NoLock); - - if (!ATCheckCheckConstrHasEnforcedParent(conrel, - parentrel, - parenttup, - changing_conids, - &parent_enforced_parentoid)) - continue; - } - *enforced_parentoid = parentoid; - if (parentrel != NULL) - table_close(parentrel, NoLock); systable_endscan(pscan); systable_endscan(scan); table_close(inhrel, AccessShareLock); return true; } - if (parentrel != NULL) - table_close(parentrel, NoLock); systable_endscan(pscan); } diff --git a/src/include/catalog/pg_inherits.h b/src/include/catalog/pg_inherits.h index cc874abaabb..66a811e50fa 100644 --- a/src/include/catalog/pg_inherits.h +++ b/src/include/catalog/pg_inherits.h @@ -58,6 +58,7 @@ extern List *find_inheritance_children_extended(Oid parentrelId, bool omit_detac extern List *find_all_inheritors(Oid parentrelId, LOCKMODE lockmode, List **numparents); +extern List *find_all_inheritors_ordered(Oid parentrelId, LOCKMODE lockmode); extern bool has_subclass(Oid relationId); extern bool has_superclass(Oid relationId); extern bool typeInheritsFrom(Oid subclassTypeId, Oid superclassTypeId); diff --git a/src/test/regress/expected/inherit.out b/src/test/regress/expected/inherit.out index 0136aa53c96..38b057a5e8e 100644 --- a/src/test/regress/expected/inherit.out +++ b/src/test/regress/expected/inherit.out @@ -1554,7 +1554,8 @@ drop cascades to table p1_c1 create table gp(a int constraint gp_a_check check (a > 0) enforced); create table p1_c1() inherits (gp); create table p1() inherits (gp); -alter table p1_c1 inherit p1; +create table p2() inherits (p1); +alter table p1_c1 inherit p2; alter table gp alter constraint gp_a_check not enforced; --ok select conname, conenforced, convalidated, conrelid::regclass from pg_constraint @@ -1565,11 +1566,13 @@ order by conrelid::regclass::text collate "C"; gp_a_check | f | f | gp gp_a_check | f | f | p1 gp_a_check | f | f | p1_c1 -(3 rows) + gp_a_check | f | f | p2 +(4 rows) drop table gp cascade; -NOTICE: drop cascades to 2 other objects +NOTICE: drop cascades to 3 other objects DETAIL: drop cascades to table p1 +drop cascades to table p2 drop cascades to table p1_c1 --for "no inherit" check constraint, it will not recurse to child table create table p1(f1 int constraint p1_a_check check (f1 > 0) no inherit not enforced); diff --git a/src/test/regress/sql/inherit.sql b/src/test/regress/sql/inherit.sql index 072fca13c13..db4f4abc709 100644 --- a/src/test/regress/sql/inherit.sql +++ b/src/test/regress/sql/inherit.sql @@ -579,7 +579,8 @@ drop table gp cascade; create table gp(a int constraint gp_a_check check (a > 0) enforced); create table p1_c1() inherits (gp); create table p1() inherits (gp); -alter table p1_c1 inherit p1; +create table p2() inherits (p1); +alter table p1_c1 inherit p2; alter table gp alter constraint gp_a_check not enforced; --ok select conname, conenforced, convalidated, conrelid::regclass from pg_constraint -- 2.50.1 (Apple Git-155)