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.
