Re: NOT ENFORCED constraint feature

amul sul <sulamul@gmail.com>

From: Amul Sul <sulamul@gmail.com>

To: jian he <jian.universality@gmail.com>

Cc: Alvaro Herrera <alvherre@alvh.no-ip.org>, Peter Eisentraut <peter@eisentraut.org>, pgsql-hackers <pgsql-hackers@postgresql.org>, Joel Jacobson <joel@compiler.org>

Date: 2024-12-10T11:47:25Z

Lists: pgsql-hackers

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 NOT ENFORCED in foreign key constraints
- eec0040c4bcd 18.0 landed
Expand test a bit
- 5d5f415816a6 18.0 landed
refactor: Pass relation OID instead of Relation to createForeignKeyCheckTriggers()
- ef7a5af77d44 18.0 landed
refactor: Split ATExecAlterConstraintInternal()
- 639238b978fe 18.0 landed
refactor: Move some code that updates pg_constraint to a separate function
- a3280e2a494f 18.0 landed
Move RemoveInheritedConstraint() call slightly earlier
- dabccf45139a 18.0 landed
refactor: Split tryAttachPartitionForeignKey()
- 1d26c2d2c4b8 18.0 landed
refactor: re-add ATExecAlterChildConstr()
- 64224a834ce4 18.0 landed
Add ATAlterConstraint struct for ALTER .. CONSTRAINT
- 80d7f990496b 18.0 landed
refactor: split ATExecAlterConstrRecurse()
- 7a947ed25b54 18.0 landed
Add support for NOT ENFORCED in CHECK constraints
- ca87c415e2fc 18.0 landed

Attachments

v8-0001-Add-support-for-NOT-ENFORCED-in-CHECK-constraints.patch (application/octet-stream) patch v8-0001
v8-0002-refactor-split-ATExecAlterConstrRecurse.patch (application/octet-stream) patch v8-0002
v8-0003-refactor-Change-ATExecAlterConstrRecurse-argument.patch (application/octet-stream) patch v8-0003
v8-0004-Remove-hastriggers-flag-check-before-fetching-FK-.patch (application/octet-stream) patch v8-0004
v8-0005-WIP-Add-support-for-NOT-ENFORCED-in-foreign-key-c.patch (application/octet-stream) patch v8-0005

On Tue, Dec 10, 2024 at 1:21 PM jian he <jian.universality@gmail.com> wrote:
>
> hi. some minor issue about v7-0001.
>
> there are 5 appearances of "sizeof(CookedConstraint)"
> to make it safe, it would be nice to manual do
> `
> cooked->is_enforced = true;
> `
> for other kinds of constraints.
>

I am not sure if it's necessary, but it doesn't seem like a bad idea,
did the same in the attached version.

>
> static bool
>  MergeWithExistingConstraint(Relation rel, const char *ccname, Node *expr,
>   bool allow_merge, bool is_local,
> + bool is_enforced,
>   bool is_initially_valid,
>   bool is_no_inherit)
>  {
> @@ -2729,12 +2738,24 @@ MergeWithExistingConstraint(Relation rel,
> const char *ccname, Node *expr,
>   * If the child constraint is "not valid" then cannot merge with a
>   * valid parent constraint.
>   */
> - if (is_initially_valid && !con->convalidated)
> + if (is_initially_valid && con->conenforced && !con->convalidated)
>   ereport(ERROR,
>   (errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
>   errmsg("constraint \"%s\" conflicts with NOT VALID constraint on
> relation \"%s\"",
>   ccname, RelationGetRelationName(rel))));
>
> There are no tests for this change. I think this change is not necessary.
>

It is necessary; otherwise, it would raise an error for a NOT ENFORCED
constraint, which is NOT VALID by default.

>
> - a/src/test/regress/expected/alter_table.out
> +++ b/src/test/regress/expected/alter_table.out
> ...
> +ALTER TABLE attmp3 VALIDATE CONSTRAINT b_greater_than_ten_not_enforced; -- fail
> +ERROR:  cannot validated NOT ENFORCED constraint
>
> there should be
> ERROR:  cannot validate NOT ENFORCED constraint
> ?
>

Are you sure you're looking at the latest patch? The error string is
already the same as you suggested.

> Do we need to update create_foreign_table.sgml
> and alter_foreign_table.sgml?

Yes, I think we just need to add the syntax; a description isn't
necessary, imo, since constraints on foreign constraints are never
enforced.

Regards,
Amul