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
