Re: NOT ENFORCED constraint feature

On 21.03.25 06:58, Amul Sul wrote:
>>>> I think the next step here is that you work to fix Álvaro's concerns
>>>> about the recursion structure.
>>>
>>> Yes, I worked on that in the attached version. I refactored
>>> ATExecAlterConstraintInternal() and moved the code that updates the
>>> pg_constraint entry into a separate function (see 0001), so it can be
>>> called from the places where the entry needs to be updated, rather
>>> than revisiting ATExecAlterConstraintInternal(). In 0002,
>>> ATExecAlterConstraintInternal() is split into two functions:
>>> ATExecAlterConstrDeferrability() and
>>> ATExecAlterConstrInheritability(), which handle altering deferrability
>>> and inheritability, respectively. These functions are expected to
>>> recurse on themselves, rather than revisiting
>>> ATExecAlterConstraintInternal() as before. This approach simplifies
>>> things. Similarly can add ATExecAlterConstrEnforceability() which
>>> recurses itself.
>>
>> Yeah, I gave this a look and I think this code layout is good.  There
>> are more functions now, but the code flow is simpler.
>>
> 
> Thank you !
> 
> Attached is the updated version, where the commit messages for patch
> 0005 and 0006 have been slightly corrected. Additionally, a few code
> comments have been updated to consistently use the ENFORCED/NOT
> ENFORCED keywords. The rest of the patches and all the code are
> unchanged.

I have committed patches 0001 through 0003.  I made some small changes:

In 0001, I renamed the function UpdateConstraintEntry() to
AlterConstrUpdateConstraintEntry() so the context is clearer.

In 0002, you had this change:

@@ -12113,75 +12154,89 @@ ATExecAlterConstraintInternal(List **wqueue, ATAlterConstraint *cmdcon,
          * If the table at either end of the constraint is partitioned, we need to
          * handle every constraint that is a child of this one.
          */
-       if (recurse && changed &&
+       if (recurse &&
                 (rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE ||
-                (OidIsValid(refrelid) &&
-                 get_rel_relkind(refrelid) == RELKIND_PARTITIONED_TABLE)))
-               ATExecAlterChildConstr(wqueue, cmdcon, conrel, tgrel, rel, contuple,
-                                                          recurse, otherrelids, lockmode);
+                get_rel_relkind(refrelid) == RELKIND_PARTITIONED_TABLE))
+               AlterConstrDeferrabilityRecurse(wqueue, cmdcon, conrel, tgrel, rel,
+                                                                               contuple, recurse, otherrelids,
+                                                                               lockmode);

AFAICT, dropping the "changed" from the conditional was not correct.  Or at
least, it would do redundant work if nothing was "changed".  So I put that
back.  Let me know if that change was intentional or there is something else
going on.

I will work through the remaining patches.  It looks good to me so far.