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
