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.

> 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.

> 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?

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.


Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:

Reply via email to