Re: Constraint merge and not valid status

Hello,

At Fri, 22 Jul 2016 17:35:48 +0900, Amit Langote <Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)jp> wrote in <9733fae3-c32f-b150-e368-a8f87d546a7f(at)lab(dot)ntt(dot)co(dot)jp>
> On 2016/07/22 17:06, Kyotaro HORIGUCHI wrote:
> > At Fri, 22 Jul 2016 14:10:48 +0900, Amit Langote wrote:
> >> On 2016/07/22 0:38, Robert Haas wrote:
> >>> On Wed, Jul 13, 2016 at 5:22 AM, Amit Langote wrote:
> >>>> Consider a scenario where one adds a *valid* constraint on a inheritance
> >>>> parent which is then merged with a child table's *not valid* constraint
> >>>> during inheritance recursion. If merged, the constraint is not checked
> >>>> for the child data even though it may have some. Is that an oversight?
> >>>
> >>> Seems like it. I'd recommend we just error out in that case and tell
> >>> the user that they should validate the child's constraint first.
> >>
> >> Agreed.
> >>
> >> Patch attached. In addition to the recursion from parent case, this seems
> >> to be broken for the alter table child inherit parent case as well. So,
> >> fixed both MergeWithExistingConstraint (called from
> >> AddRelationNewConstraints) and MergeConstraintsIntoExisting (called from
> >> ATExecAddInherit). I had to add a new argument is_not_valid to the former
> >> to signal whether the constraint being propagated itself is declared NOT
> >> VALID, in which we can proceed with merging. Also added some tests for
> >> both cases.
> >
> > It seems to work as expected and message seems to be
> > reasonable. Test seems to be fine.
>
> Thanks for reviewing.
>
> > By the way I have one question.
> >
> > Is it an expected configuration where tables in an inheritance
> > tree has different valid state on the same (check) constraint?
>
> I would think not.

I understand that the first problem is that the problematic
state inhibits later VALIDATE CONSTRAINT on parent from working
as expected. This patch inhibits the state where a parent is
valid and any of its children is not-valid, but allows the
opposite and it is enough to fix the problem.

I thought the opposite state is ok generally but not with full
confidence.

After some reading the code, it seems to affect only on some
cache invalidation logics and constraint exclusion logic to
ignore the check constraint per component table, and
acquire_inherited_sample_rows.

The first and second wouldn't harm. The third causes needless
tuple conversion. If this is a problem, the validity state of all
relations in an inheritance tree should be exactly the same,
ether valid or not-valid. Or should make the function to ignore
the difference of validity state.

If the problem is only VALIDATE CONSTRAINT on the parent and
mixted validity states within an inheritance tree is not, making
it process whole the inheritance tree regardsless of the validity
state of the parent would also fix the problem.

After all, my concerns are the following.

- Is the mixed validity states (in any form) in an inheritnce
tree should be valid? If so, VALIDATE CONSTRAINT should be
fixed, not MergeWithExistingConstraint. If not, the opposite
state also should be inhibited.

- Is it valid to represent all descendants' validity states by
the parent's state? (Currently only VALIDATE CONSTRAINT does)
If not, VALIDATE CONSTRAINT should be fixed.

Any thoughts?

> > The check should be an equality if it's not.
>
> If you mean that the valid state should be same (equal) at all times on
> parent and all the child tables, then that is exactly what the patch tries
> to achieve. Currently, valid state of a constraint on a child table is
> left to differ from the parent in two cases as described in my messages.

regards,

--
Kyotaro Horiguchi
NTT Open Source Software Center