Re: NOT ENFORCED constraint feature

On Thu, Mar 27, 2025 at 6:28 PM Peter Eisentraut <peter(at)eisentraut(dot)org> wrote:
>
> On 25.03.25 17:48, Peter Eisentraut wrote:
> > I have committed patches 0001 through 0003. I made some small changes:
>
> > I will work through the remaining patches. It looks good to me so far.
>
> For the time being, here are the remaining patches rebased.
>
> I think you could squash these together at this point. This is
> especially useful since 0003 overwrites part of the changes in 0002, so
> it's better to see them all together.
>
> Some details:
>
> In CloneFkReferenced() and also in DetachPartitionFinalize(), you have
> this change:
>
> - fkconstraint->initially_valid = true;
> + fkconstraint->initially_valid = constrForm->convalidated;
>
> I'm having a hard time understanding this. Is this an pre-existing
> problem? What does this change do?
>

No issue for the master head since constraints are always valid for
newly created tables. However, I wanted to ensure that the validation
status aligns with enforceability. When constraints are not enforced,
the convalidated flag must be false, so I didn't want to mark it as
true blindly, so fetching its value.

> Most of the other stuff is mechanical and fits into existing structures,
> so it seems ok.
>
> Small cosmetic suggestion: write count(*) instead of count(1). This
> fits better with existing practices.
>

Ok.

> The merge rules for inheritance and partitioning are still confusing. I
> don't understand the behavior inh_check_constraint10 in the inherit.sql
> test:
>
> +-- the invalid state of the child constraint will be ignored here.
> +alter table p1 add constraint inh_check_constraint10 check (f1 < 10)
> not enforced;
> +alter table p1_c1 add constraint inh_check_constraint10 check (f1 < 10)
> not valid enforced;
>
> Why is that correct? At least we should explain it here, or somewhere.
>

A NOT ENFORCED constraint will be considered NOT VALID, so the next
constraint, even if specified with a NOT VALID or VALID clause, will
be ignored. I'll improve the comment a bit.

> I'm also confused about the changes of the constraint names in the
> foreign_key.sql test:
>
> -ERROR: insert or update on table "fk_partitioned_fk_2" violates
> foreign key constraint "fk_partitioned_fk_a_b_fkey"
> +ERROR: insert or update on table "fk_partitioned_fk_2" violates
> foreign key constraint "fk_partitioned_fk_2_a_b_fkey"
>
> And then patch 0003 changes it again. This seems pretty random. We
> should make sure that tests don't contain unrelated changes like that.
> (Or at least it's not clear why they are related.)
>

I have fixed it in the v19 version, which I just posted a few moments ago.

> There is also in 0002
>
> +-- should be having two constraints
>
> and then in 0003:
>
> --- should be having two constraints
> +-- should only have one constraint
>
> So another reason for squashing these together, so we don't have
> confusing intermediate states.
>

Sure.

> That said, is there a simpler way? Patch 0003 appears to add a lot of
> complexity. Could we make this simpler by saying, if you have otherwise
> matching constraints with different enforceability, make this an error.
> Then users can themselves adjust the enforceability how they want to
> make it match.

We can simply discard this patch, as it still reflects the correct
behavior. It creates a new constraint without affecting the existing
constraint with differing enforceability on the child. I noticed
similar behavior with deferrability -- when it differs, the
constraints are not merged, and a new constraint is created on the
child. Let me know your thoughts so I can avoid squashing patch 0006.

Regards,
Amul