Re: Constraint merge and not valid status

Hello,

At Mon, 25 Jul 2016 15:57:00 +0900, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> wrote in
<fe27dd92-61bb-eb63-da47-63b535e41c48@lab.ntt.co.jp>
> On 2016/07/25 12:44, Kyotaro HORIGUCHI wrote:
> > At Fri, 22 Jul 2016 17:35:48 +0900, Amit Langote wrote:
> >> On 2016/07/22 17:06, Kyotaro HORIGUCHI wrote:
> >>
> >>> 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.
> 
> I obviously missed the opposite case.  However, it's OK for a child's
> constraint to be valid while the parent's is not.  There seems to be in
> place only the one-way rule which is that we don't mark parent's
> constraint valid until and unless it is marked valid on all the child
> tables.  From the following comment in ATExecValidateConstraint():
> 
> /*
>  * For CHECK constraints, we must ensure that we only mark the
>  * constraint as validated on the parent if it's already validated
>  * on the children.
>  *
> 
> And it seems to be in place only for VALIDATE CONSTRAINT's purpose.

Agreed. It guarantees nothing outside the function but it shows
that at least one side of mixed validity status is/should be
considered.

> > 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.
> 
> Hmm, perhaps check constraint valid status affecting whether
> child-to-parent-rowtype conversion should occur is unnecessary.  Maybe a
> subset of those checks for purposes of acquire_inherited_sample_rows would
> suffice.  Or simply make equalTupleDescs accept a boolean parameter that
> indicates whether or not to check TupleConstr equality.

equalTupleDescs are used in few places. The equalTupleDescs's
"Logical" equality seems to stem from the compatibility while
given to heap_form/modify_tuple. I don't think the validity
status has something to do with the result of such functions.

> > 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?
> 
> Wouldn't it be better to leave VALIDATE CONSTRAINT alone, because the way
> it works now it short-circuits some extra processing if the parent's
> constraint is marked valid?  All we need to do is to prevent the rule from
> being broken by fixing the current code like the patch proposes.  If we
> try to fix the VALIDATE CONSTRAINT instead, we'll always have to pay the
> cost of find_all_inheritors().  Thoughts?

VALIDATE CONSTRAINT is expected to take a long time like analyze
and used for very limited cases so the cost of
find_all_inheritors() don't seem to be siginificant. However,
such modification would give a pain more than required. As the
result, I agree with you.

> As for the other cases (determining whether we can live with the state
> where a child's constraint is valid whereas the same on the parent and
> siblings is not):
> 
> 1. Both cache invalidation and acquire_inherited_sample_rows depend on
> equalTupleDescs, where the former is unrelated to inheritance behavior as
> such (and hence unaffected by current discussion); for the latter, we
> might want to simply ignore comparing the check constraint valid status as
> mentioned above
> 
> 2. Constraint exclusion, where it seems OK for a child's check constraint
> to cause it to be excluded while the same check constraint of its parent
> and siblings is ignored causing them to be needlessly scanned.

Agreed to the both. So the conclusion here is,
- Inhibit only the case where parent is to be validated while  child is not-validated while merge. The oppsite is
deliberatelyallowed and properly handled by VALIDATE  CONSTRAINT.
 
  That is, the last patch doesn't need modification.
- Remove ccvalid condition from equalTupleDescs() to reduce  unnecessary cache invalidation or tuple rebuilding.
  The attached patch does this.


These two thing seems to be different problem.

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center