Thread
Commits
Same data as JSON:
GET /api/v1/messages/:b64id/commits
the thread's linked commits as JSON, with link sources.
API reference →
-
Add support for altering CHECK constraint enforceability
- 342051d73b38 19 (unreleased) landed
-
rename alter constraint enforceability related functions
- a9747153e14d 19 (unreleased) landed
-
alter check constraint enforceability
jian he <jian.universality@gmail.com> — 2025-06-02T13:57:33Z
hi. Currently in pg18, we can add not enforced check constraints. but we can not do ALTER TABLE ALTER CONSTRAINT [NOT] ENFORCED for check constraint. The attached patch is implementation of changing enforceability of check constraint.
-
Re: alter check constraint enforceability
jian he <jian.universality@gmail.com> — 2025-07-04T11:59:47Z
On Mon, Jun 2, 2025 at 9:57 PM jian he <jian.universality@gmail.com> wrote: > > Currently in pg18, we can add not enforced check constraints. > but we can not do ALTER TABLE ALTER CONSTRAINT [NOT] ENFORCED > for check constraint. > > The attached patch is implementation of changing enforceability of > check constraint. hi. rebase and minor refactoring.
-
Re: alter check constraint enforceability
Robert Treat <rob@xzilla.net> — 2025-08-06T23:35:08Z
On Fri, Jul 4, 2025 at 8:00 AM jian he <jian.universality@gmail.com> wrote: > > On Mon, Jun 2, 2025 at 9:57 PM jian he <jian.universality@gmail.com> wrote: > > > > Currently in pg18, we can add not enforced check constraints. > > but we can not do ALTER TABLE ALTER CONSTRAINT [NOT] ENFORCED > > for check constraint. > > > > The attached patch is implementation of changing enforceability of > > check constraint. > Initial look and testing looks good. There are some odd parts to work through with partitioned tables and recursion (for example, if you have a parent unenforced, and a child enforced, setting a parent enforced and then not enforced will recurse to the child, so you end up in a different state. that could be surprising, but the alternative is not obviously more sensicle). Some minor items below: + errhint("Only foreign key, check constraint can change enforceability")); "Only foreign key and check constraints can change enforceability" -- + /* + * If we are told not to recurse, there had better not be any child + * tables, because we can't changing constraint enforceability on + * the parent unless we have chaned enforceability for all child + * tables. + */ * tables, because we can't change constraint enforceability on * the parent unless we have changed enforceability for all child -- + if (rel->rd_rel->relkind == RELKIND_RELATION && + cmdcon->is_enforced && + !currcon->conenforced) i think I have convinced myself that this is correct, but maybe I will ask you if you had any concerns that this needed to also consider RELKIND_PARTITIONED_TABLE as well? Robert Treat https://xzilla.net -
Re: alter check constraint enforceability
jian he <jian.universality@gmail.com> — 2025-08-11T09:53:08Z
On Thu, Aug 7, 2025 at 7:35 AM Robert Treat <rob@xzilla.net> wrote: > > + if (rel->rd_rel->relkind == RELKIND_RELATION && > + cmdcon->is_enforced && > + !currcon->conenforced) > > i think I have convinced myself that this is correct, but maybe I will > ask you if you had any concerns that this needed to also consider > RELKIND_PARTITIONED_TABLE as well? > ATExecAlterCheckConstrEnforceability itself will be recursive to all the children. AlterConstrUpdateConstraintEntry is responsible for changing the catalog state. except the changing the catalog state, if we change the check constraint from NOT ENFORCED to ENFORCED, we also need to verify it in phase 3. that's the purpose of > + if (rel->rd_rel->relkind == RELKIND_RELATION && > + cmdcon->is_enforced && > + !currcon->conenforced) partitioned tables don't have storage, phase3 table scan to verify check constraint on partitioned table don't have effect. also partitioned table check constraint (name, definition (pg_constraint.conbin) must match with partition otherwise partition can be attached to the partitioned table. so here you don't need to consider RELKIND_PARTITIONED_TABLE.
-
Re: alter check constraint enforceability
Kirill Reshke <reshkekirill@gmail.com> — 2025-08-11T13:59:53Z
On Mon, 11 Aug 2025 at 14:53, jian he <jian.universality@gmail.com> wrote: > > On Thu, Aug 7, 2025 at 7:35 AM Robert Treat <rob@xzilla.net> wrote: > > > > + if (rel->rd_rel->relkind == RELKIND_RELATION && > > + cmdcon->is_enforced && > > + !currcon->conenforced) > > > > i think I have convinced myself that this is correct, but maybe I will > > ask you if you had any concerns that this needed to also consider > > RELKIND_PARTITIONED_TABLE as well? > > > > ATExecAlterCheckConstrEnforceability itself will be recursive to all > the children. > AlterConstrUpdateConstraintEntry is responsible for changing the catalog state. > except the changing the catalog state, if we change the check > constraint from NOT ENFORCED > to ENFORCED, we also need to verify it in phase 3. > that's the purpose of > > + if (rel->rd_rel->relkind == RELKIND_RELATION && > > + cmdcon->is_enforced && > > + !currcon->conenforced) > > partitioned tables don't have storage, phase3 table scan to verify > check constraint on partitioned table > don't have effect. > > also partitioned table check constraint (name, definition > (pg_constraint.conbin) must match with partition > otherwise partition can be attached to the partitioned table. > so here you don't need to consider RELKIND_PARTITIONED_TABLE. Hi! I looked at v3. Should we rename `ATExecAlterConstrEnforceability` to `ATExecAlterFKConstrEnforceability `? -- Best regards, Kirill Reshke
-
Re: alter check constraint enforceability
Robert Treat <rob@xzilla.net> — 2025-11-06T23:29:28Z
On Mon, Aug 11, 2025 at 10:00 AM Kirill Reshke <reshkekirill@gmail.com> wrote: > On Mon, 11 Aug 2025 at 14:53, jian he <jian.universality@gmail.com> wrote: > > On Thu, Aug 7, 2025 at 7:35 AM Robert Treat <rob@xzilla.net> wrote: > > > > > > + if (rel->rd_rel->relkind == RELKIND_RELATION && > > > + cmdcon->is_enforced && > > > + !currcon->conenforced) > > > > > > i think I have convinced myself that this is correct, but maybe I will > > > ask you if you had any concerns that this needed to also consider > > > RELKIND_PARTITIONED_TABLE as well? > > > > > > > ATExecAlterCheckConstrEnforceability itself will be recursive to all > > the children. > > AlterConstrUpdateConstraintEntry is responsible for changing the catalog state. > > except the changing the catalog state, if we change the check > > constraint from NOT ENFORCED > > to ENFORCED, we also need to verify it in phase 3. > > that's the purpose of > > > + if (rel->rd_rel->relkind == RELKIND_RELATION && > > > + cmdcon->is_enforced && > > > + !currcon->conenforced) > > > > partitioned tables don't have storage, phase3 table scan to verify > > check constraint on partitioned table > > don't have effect. > > > > also partitioned table check constraint (name, definition > > (pg_constraint.conbin) must match with partition > > otherwise partition can be attached to the partitioned table. > > so here you don't need to consider RELKIND_PARTITIONED_TABLE. > > > Hi! > I looked at v3. > > Should we rename `ATExecAlterConstrEnforceability` to > `ATExecAlterFKConstrEnforceability `? > +1 Robert Treat https://xzilla.net
-
Re: alter check constraint enforceability
jian he <jian.universality@gmail.com> — 2025-12-04T06:51:54Z
On Fri, Nov 7, 2025 at 7:29 AM Robert Treat <rob@xzilla.net> wrote: > > > Hi! > > I looked at v3. > > > > Should we rename `ATExecAlterConstrEnforceability` to > > `ATExecAlterFKConstrEnforceability `? > > > > +1 > > Robert Treat > https://xzilla.net hi. AlterConstrEnforceabilityRecurse renamed to AlterFKConstrEnforceabilityRecurse ATExecAlterConstrEnforceability renamed to ATExecAlterFKConstrEnforceability. There seem to be no tests for cases where a partitioned table’s check constraint is not enforced, but the partition’s constraint is enforced. I’ve added tests for this case. ATExecAlterCheckConstrEnforceability ``rel = table_open(currcon->conrelid, NoLock);`` NoLock is ok, because parent is already locked, obviously, ``find_all_inheritors(RelationGetRelid(rel), lockmode, NULL); `` will lock all the children with lockmode. -- jian https://www.enterprisedb.com
-
Re: alter check constraint enforceability
amul sul <sulamul@gmail.com> — 2025-12-08T09:58:11Z
On Thu, Dec 4, 2025 at 12:22 PM jian he <jian.universality@gmail.com> wrote: > > On Fri, Nov 7, 2025 at 7:29 AM Robert Treat <rob@xzilla.net> wrote: > > The v4 patch is quite good. Here are a few comments/suggestions for the cosmetic fixes: + created. Currently <literal>FOREIGN KEY</literal> and + <literal>CHECK</literal> constraints may be altered in this fashion, but see below. Although documents may not strictly follow an 80-column length restriction all the places, it is better to adhere to it as much as possible. -- + errhint("Only foreign key and check constraints can change enforceability")); Missing a full stop (.) at the end. -- + /* + * parent relation already locked by called, children will be locked by + * find_all_inheritors. So NoLock is fine here. + */ + rel = table_open(currcon->conrelid, NoLock); + if (currcon->conenforced != cmdcon->is_enforced) + { Add a newline between these. Also, start comment with capital letter: s/parent/Parent -- -static bool ATExecAlterConstrEnforceability(List **wqueue, ... +static bool ATExecAlterFKConstrEnforceability(List **wqueue, I suggest the renaming patch be separated. -- - * Currently only works for Foreign Key and not null constraints. + * Currently works for Foreign Key, CHECK, and not null constraints. Not consistent naming format, should be: s/CHECK/Check. -- + if (cmdcon->alterEnforceability) + { + if (currcon->contype == CONSTRAINT_FOREIGN) + { + ATExecAlterFKConstrEnforceability(wqueue, cmdcon, conrel, tgrel, + currcon->conrelid, currcon->confrelid, + contuple, lockmode, InvalidOid, + InvalidOid, InvalidOid, InvalidOid); + changed = true; + } + else if (currcon->contype == CONSTRAINT_CHECK) + { + ATExecAlterCheckConstrEnforceability(wqueue, cmdcon, conrel, contuple, + recurse, false, lockmode); + changed = true; + } + } Don't need inner curly braces; set changed = true; once for both. -- + * conrel is the pg_constraint catalog relation. Not sure why we need to mention conrel here only? -- + if (!cmdcon->is_enforced || changed) + { The reason for recursing for the non-enforced constraint (like the FK constraint) is mentioned in the function prolog. However, since two conditions are involved here, I was initially confused about the change. Could you please add a short comment explaining why we enter for the not-enforced constraint irrespective of whether it was changed or not, or simply move the relevant note from the prolog here? -- +static void +AlterCheckConstrEnforceabilityRecurse(List **wqueue, ATAlterConstraint *cmdcon, + Relation conrel, Oid conrelid, + bool recurse, bool recursing, + LOCKMODE lockmode) +{ Kindly add a prolog comment. Regards, Amul -
Re: alter check constraint enforceability
jian he <jian.universality@gmail.com> — 2025-12-12T07:54:55Z
On Mon, Dec 8, 2025 at 5:58 PM Amul Sul <sulamul@gmail.com> wrote: > > > The v4 patch is quite good. Here are a few comments/suggestions for > the cosmetic fixes: > > + created. Currently <literal>FOREIGN KEY</literal> and > + <literal>CHECK</literal> constraints may be altered in this > fashion, but see below. > > Although documents may not strictly follow an 80-column length > restriction all the places, it is better to adhere to it as much as possible. > -- > > + errhint("Only foreign key and check constraints can > change enforceability")); > > Missing a full stop (.) at the end. > -- > > + /* > + * parent relation already locked by called, children will be locked by > + * find_all_inheritors. So NoLock is fine here. > + */ > + rel = table_open(currcon->conrelid, NoLock); > + if (currcon->conenforced != cmdcon->is_enforced) > + { > > Add a newline between these. Also, start comment with capital letter: > s/parent/Parent > -- > > -static bool ATExecAlterConstrEnforceability(List **wqueue, > ... > +static bool ATExecAlterFKConstrEnforceability(List **wqueue, > > I suggest the renaming patch be separated. > -- > > - * Currently only works for Foreign Key and not null constraints. > + * Currently works for Foreign Key, CHECK, and not null constraints. > > Not consistent naming format, should be: s/CHECK/Check. > -- > > + if (cmdcon->alterEnforceability) > + { > + if (currcon->contype == CONSTRAINT_FOREIGN) > + { > + ATExecAlterFKConstrEnforceability(wqueue, cmdcon, conrel, tgrel, > + currcon->conrelid, > currcon->confrelid, > + contuple, lockmode, InvalidOid, > + InvalidOid, InvalidOid, > InvalidOid); > + changed = true; > + } > + else if (currcon->contype == CONSTRAINT_CHECK) > + { > + ATExecAlterCheckConstrEnforceability(wqueue, cmdcon, > conrel, contuple, > + recurse, false, lockmode); > + changed = true; > + } > + } > > Don't need inner curly braces; set changed = true; once for both. > -- > > + * conrel is the pg_constraint catalog relation. > > Not sure why we need to mention conrel here only? > -- > hi. I have addressed all your points mentioned above. > + if (!cmdcon->is_enforced || changed) > + { > > The reason for recursing for the non-enforced constraint (like the FK > constraint) is mentioned in the function prolog. However, since two > conditions are involved here, I was initially confused about the > change. Could you please add a short comment explaining why we enter > for the not-enforced constraint irrespective of whether it was changed > or not, or simply move the relevant note from the prolog here? > -- moving the prolog to the IF check seems easier. > > +static void > +AlterCheckConstrEnforceabilityRecurse(List **wqueue, ATAlterConstraint *cmdcon, > + Relation conrel, Oid conrelid, > + bool recurse, bool recursing, > + LOCKMODE lockmode) > +{ > > Kindly add a prolog comment. > /* * Invokes ATExecAlterCheckConstrEnforceability for each CHECK constraint that * is a child of the specified constraint. * * We rely on the parent and child tables having identical CHECK constraint * names to retrieve the child's pg_constraint tuple. * * The arguments to this function have the same meaning as the arguments to * ATExecAlterCheckConstrEnforceability. */ The above comments are what I came up with. v5-0001: AlterConstrEnforceabilityRecurse renamed to AlterFKConstrEnforceabilityRecurse ATExecAlterConstrEnforceability renamed to ATExecAlterFKConstrEnforceability. comments slightly adjusted, no other changes. v5-0002: alter check constraint enforceability -- jian https://www.enterprisedb.com/ -
Re: alter check constraint enforceability
amul sul <sulamul@gmail.com> — 2025-12-15T11:49:00Z
On Fri, Dec 12, 2025 at 1:25 PM jian he <jian.universality@gmail.com> wrote: > > On Mon, Dec 8, 2025 at 5:58 PM Amul Sul <sulamul@gmail.com> wrote: >> [....] > > v5-0001: > AlterConstrEnforceabilityRecurse renamed to AlterFKConstrEnforceabilityRecurse n > ATExecAlterConstrEnforceability renamed to ATExecAlterFKConstrEnforceability. > comments slightly adjusted, no other changes. > Looks good to me, thanks ! > v5-0002: alter check constraint enforceability > The patch also looks good, but I have a minor comment for the test -- you created the check_constraint_status view, which is not dropped, it should be dropped at the end. Also, instead of a view, I think you could use the \set psql-meta-command; for example, see the init_range_parted or show_data tests in update.sql Also, run pgindent on both patches. Regards, Amul
-
Re: alter check constraint enforceability
jian he <jian.universality@gmail.com> — 2025-12-16T04:55:52Z
On Mon, Dec 15, 2025 at 7:49 PM Amul Sul <sulamul@gmail.com> wrote: > > > v5-0002: alter check constraint enforceability > > > The patch also looks good, but I have a minor comment for the test -- > you created the check_constraint_status view, which is not dropped, it > should be dropped at the end. Also, instead of a view, I think you > could use the \set psql-meta-command; for example, see the > init_range_parted or show_data tests in update.sql > > Also, run pgindent on both patches. > i have tried using \set, but it seems to require the query within a single line. since the view check_constraint_status definition is quite longer, \set would make it less readable, so I choose to use view. previously I use + newcon = (NewConstraint *) palloc0(sizeof(NewConstraint)); now use + newcon = palloc0_object(NewConstraint); v6-0001, v6-0002 both indented properly via pgindent, also polished the commit messages. -- jian https://www.enterprisedb.com/
-
Re: alter check constraint enforceability
amul sul <sulamul@gmail.com> — 2025-12-16T06:34:40Z
On Tue, Dec 16, 2025 at 10:26 AM jian he <jian.universality@gmail.com> wrote: > > On Mon, Dec 15, 2025 at 7:49 PM Amul Sul <sulamul@gmail.com> wrote: > > > > [...] > v6-0001, v6-0002 both indented properly via pgindent, also polished the commit > messages. Thanks. I don't have any other comments. The patch is now ready for committer review. I have updated the status of the CommitFest entry accordingly. Regards, Amul