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.