Horton

Re: NOT ENFORCED constraint feature

amul sul <sulamul@gmail.com>

View thread
From: Amul Sul <sulamul@gmail.com>
To: jian he <jian.universality@gmail.com>
Cc: pgsql-hackers <pgsql-hackers@postgresql.org>, Joel Jacobson <joel@compiler.org>, Peter Eisentraut <peter@eisentraut.org>
Date: 2024-12-04T11:04:23Z
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 →

  1. Add support for NOT ENFORCED in foreign key constraints

  2. Expand test a bit

  3. refactor: Pass relation OID instead of Relation to createForeignKeyCheckTriggers()

  4. refactor: Split ATExecAlterConstraintInternal()

  5. refactor: Move some code that updates pg_constraint to a separate function

  6. Move RemoveInheritedConstraint() call slightly earlier

  7. refactor: Split tryAttachPartitionForeignKey()

  8. refactor: re-add ATExecAlterChildConstr()

  9. Add ATAlterConstraint struct for ALTER .. CONSTRAINT

  10. refactor: split ATExecAlterConstrRecurse()

  11. Add support for NOT ENFORCED in CHECK constraints

On Wed, Dec 4, 2024 at 1:40 PM jian he <jian.universality@gmail.com> wrote:
>
> i just only apply v5-0001 for now.
>
> create table t(a int);
> alter table t ADD CONSTRAINT cc CHECK (a > 0);
> alter table t alter CONSTRAINT cc NOT ENFORCED;
> alter table t alter CONSTRAINT cc ENFORCED;
>
> the last two queries will fail, which means
> ALTER CONSTRAINT constraint_name [ DEFERRABLE | NOT DEFERRABLE ] [
> INITIALLY DEFERRED | INITIALLY IMMEDIATE ] [ ENFORCED | NOT ENFORCED ]
> in doc/src/sgml/ref/alter_table.sgml is not correct?
> also no code change in ATExecAlterConstraint.
>

Your are correct, will move this to 0005 patch.

> errmsg("cannot validated NOT ENFORCED constraint")));
> should be
> errmsg("cannot validate NOT ENFORCED constraint")));
> ?
>

Yes, I realized that while working on Peter's last review comments.

> typedef struct ConstrCheck
> {
>     char       *ccname;
>     char       *ccbin;            /* nodeToString representation of expr */
>     bool        ccenforced;
>     bool        ccvalid;
>     bool        ccnoinherit;    /* this is a non-inheritable constraint */
> } ConstrCheck
>
> ConstraintImpliedByRelConstraint,
> get_relation_constraints
> need skip notenforced check constraint?
>

That gets skipped since ccvalid is false for NOT ENFORCED constraints.
However, for better readability, I've added an assertion with a
comment in my local changes.

>
> put domain related tests from constraints.sql to domain.sql would be better.

Ok.

> looking at it again.
>
>     if (!con->conenforced)
>         ereport(ERROR,
>                 (errcode(ERRCODE_WRONG_OBJECT_TYPE),
>                  errmsg("cannot validated NOT ENFORCED constraint")));
>
> ERRCODE_WRONG_OBJECT_TYPE is not that ok? maybe
> ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE
> or
> ERRCODE_INVALID_TABLE_DEFINITION
>

I think ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE would be much suitable.

>
> if (!con->conenforced)
>        ereport(ERROR,
>                 (errcode(ERRCODE_WRONG_OBJECT_TYPE),
>                  errmsg("cannot validated NOT ENFORCED constraint")));
> if (!con->convalidated)
> {
> ....
> if (con->contype == CONSTRAINT_FOREIGN)
>  {
>             /*
>              * Queue validation for phase 3 only if constraint is enforced;
>              * otherwise, adding it to the validation queue won't be very
>              * effective, as the verification will be skipped.
>              */
>             if (con->conenforced)
>              ......
>  }
>
> in ATExecValidateConstraint "" if (con->conenforced)"""  will always be true?

Yes, the changes from that patch have been reverted in my local code, which
I will post soon.

Thanks again for your review comments; they were very helpful.

Regards,
Amul