Re: [PATCH] validate inherited check constraints when enabling enforcement

> On Sep 13, 2026, at 19:32, Nikolay Samokhvalov <nik(at)postgres(dot)ai> wrote:
>
> Hi,
>
> I found a case where add constraint marks an inherited not enforced check
> as validated without checking the old rows. With psql -X in a fresh
> database:
>
> create table p (a int constraint ck check (a > 0) not enforced);
> create table c () inherits (p);
> insert into c values (-1);
> alter table c add constraint ck check (a > 0);
> select conenforced, convalidated from pg_constraint
> where conrelid = 'c'::regclass and conname = 'ck';
>
> On unpatched pg19, the alter succeeds and the query returns t, t. It
> should reject the -1 row. Adding not valid to the alter also leaves
> convalidated true, which is wrong too.
>
> MergeWithExistingConstraint() updates the flags, but the merged
> constraint isn't returned to the normal validation path. The attached
> patch queues that check and carries enforcement through descendants,
> without changing inheritance counts. It preserves not valid, only, and
> the descendant ownership checks. This replaces the direct-case patch
> in my fork.
>
> My AI harness built and tested the attachment on REL_19_STABLE at
> 6bc236c8a5b (19beta3), with assertions:
> the example fails at the alter as expected, and the focused inheritance
> test, 240 core regression tests, and 133 isolation tests pass.
>
> The same merge code is present in current pg18 and master. I haven't
> adapted this patch for pg18 yet; it lacks the check enforceability
> traversal used here.
>
> Nik
> <0001-validate-inherited-check-promotion.patch>

Wow, working with inheritance is really complicated.

For this patch, I think one of the trickiest cases is something like this:

Initial state:
```
P (not enforced, not valid), say the constraint name is cc
|
-> c1 (not enforced, not valid)
|
-> c2 (enforce, not valid), c2 has enforced the constraint individually
```

Then we run:
```
ALTER TABLE c1 ADD CONSTRAINT cc (…);
```

The command needs to recurse to c2. Since c2 is already enforced, no additional enforcement change is needed there. However, because c2 is still not validated, it still needs to be validated as part of this command.

Overall, the fix looks good to me, and the new tests seem to cover pretty much all of the interesting scenarios.

I have a few small comments:

1
```
+ /*
+ * If the command names a check constraint and an identically-named NOT
+ * ENFORCED check constraint already exists on this relation, then (if
+ * things go well) AddRelationNewConstraints will merge the new constraint
+ * into the existing one, additionally marking it enforced. Existing rows
+ * have never been checked against a NOT ENFORCED constraint, so unlike
+ * ordinary merges this one requires the existing rows to be verified when
+ * the new constraint is to be valid. Take note of the pre-merge state,
+ * so that we can queue that work below.
+ */
+ if (constr->contype == CONSTR_CHECK &&
+ constr->conname != NULL &&
+ constr->is_enforced)
+ {
+ Oid conoid;
+
+ conoid = get_relation_constraint_oid(RelationGetRelid(rel),
+ constr->conname, true);
+ if (OidIsValid(conoid))
+ {
+ HeapTuple contup;
+ Form_pg_constraint conform;
+
+ contup = SearchSysCache1(CONSTROID, ObjectIdGetDatum(conoid));
+ if (!HeapTupleIsValid(contup))
+ elog(ERROR, "cache lookup failed for constraint %u", conoid);
+ conform = (Form_pg_constraint) GETSTRUCT(contup);
+ if (conform->contype == CONSTRAINT_CHECK && !conform->conenforced)
+ enforcing_conoid = conoid;
```

My understanding is that enforcing_conoid is used to remember an inherited constraint that is about to become locally enforced through the merge. If so, would it make sense to also check conform->coninhcount > 0 here?

2
```
static bool
ATExecAlterCheckConstrEnforceability(List **wqueue, ATAlterConstraint *cmdcon,
Relation conrel, HeapTuple contuple,
bool recurse, bool recursing,
List *changing_conids,
+ bool skip_validation,
+ bool force_recurse,
+ bool force_validation,
LOCKMODE lockmode)
```

When force_validation is true, I think skip_validation should necessarily be false. If that is an invariant of this function, would it be worth adding an assertion to make that explicit, such as:
```
Assert(!force_validation || !skip_validation);
```

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