Re: [HACKERS] A bug in mapping attributes in ATExecAttachPartition()

Thanks for the review.

On 2017/07/03 20:13, Ashutosh Bapat wrote:
> Thanks for working on the previous comments. The code really looks good now.
> On Fri, Jun 23, 2017 at 2:29 PM, Amit Langote
> <Langote_Amit_f8@lab.ntt.co.jp> wrote:
>>
>>> Don't we need an exclusive lock to
>>> make sure that the constraints are not changed while we are validating those?
>>
>> If I understand your question correctly, you meant to ask if we don't need
>> the strongest lock on individual partitions while looking at their
>> constraints to prove that we don't need to scan them.  We do and we do
>> take the strongest lock on individual partitions even today in the second
>> call to find_all_inheritors().  We're trying to eliminate the second call
>> here.
> 
> The comment seems to imply that we need strongest lock only when we
> "scan" the table/s.
> 
> Some more comments on 0001
> -     * Prevent circularity by seeing if rel is a partition of attachRel. (In
> +     * Prevent circularity by seeing if rel is a partition of attachRel, (In
>       * particular, this disallows making a rel a partition of itself.)
> The sentence outside () doesn't have a full-stop. I think the original
> construct was better.

Yep, fixed.

> +     * We want to avoid having to construct this list again, so we request the
>
> "this list" is confusing here since the earlier sentence doesn't mention any
> list at all. Instead we may reword it as "We will need the list of children
> later to check whether any of those have a row which would not fit the
> partition constraints. So, take the strongest lock ..."

It was confusing for sure, so rewrote.

>      * XXX - Do we need to lock the partitions here if we already have the
>      * strongest lock on attachRel?  The information we need here to check
>      * for circularity cannot change without taking a lock on attachRel.
>
> I wondered about this. Do we really need an exclusive lock to check whether
> partition constraint is valid? May be we can compare this condition with ALTER
> TABLE ... ADD CONSTRAINT since the children will all get a new constraint
> effectively. So, exclusive lock it is.

Actually, the XXX comment is about whether we need to lock the children at
all when checking the circularity of inheritance, that is, not about
whether we need lock to check the partition constraint is valid.

>         Assert(linitial_oid(attachRel_children) ==
>                                             RelationGetRelid(attachRel));
>         if (attachRel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
>             attachRel_children = list_delete_first(attachRel_children);
>
> Is it necessary for this code to have OID of the relation being attached as the
> first one? You could simply call list_delete_oid() instead of
> list_delete_first(). If for any reason find_all_inheritors() changes the output
> order, this assertion and code would need a change.\

I concluded that removing attachRel's OID from attachRel_children is
pointless, considering that we have to check for attachRel in the loop
below anyway.  The step of removing the OID wasn't helping much.

>>> The name skipPartConstraintValidation() looks very specific to the case at
>>> hand. The function is really checking whether existing constraints on the table
>>> can imply the given constraints (which happen to be partition constraints). How
>>> about PartConstraintImpliedByRelConstraint()? The idea is to pick a general
>>> name so that the function can be used for purposes other than skipping
>>> validation scan in future.
>>
>> I liked this idea, so done.
> 
> + * skipPartConstraintValidation
> +PartConstraintImpliedByRelConstraint(Relation partrel, List *partConstraint)
> Different function names in prologue and the definition.

Oops, fixed.

> +    if (predicate_implied_by(partConstraint, existConstraint, true))
> +        return true;
> +
> +    /* Tough luck. */
> +    return false;
>
> why not to just return the return value of predicate_implied_by() as is?

Sigh.  Done. :)

> With this we can actually handle constr == NULL a bit differently.
> +    if (constr == NULL)
> +        return false;
>
> To be on safer side, return false when partConstraint is not NULL. If both the
> relation constraint and partConstraint are both NULL, the first does imply the
> other. Or may be leave that decision to predicate_implied_by(), which takes
> care of it right at the beginning of the function.

Rearranged code considering this comment.  Let's let
predicate_implied_by() make ultimate decisions about logical implication. :)

> 
> +         * For each leaf partition, check if it we can skip the validation
>
> An extra "it".

Fixed.

> 
> +         * Note that attachRel's OID is in this list.  Since we already
> +         * determined above that its validation scan cannot be skipped, we
> +         * need not check that again in the loop below.  If it's partitioned,
>
> I don't see code to skip checking whether scan can be skipped for relation
> being attached. The loop below this comments executes for every unpartitioned
> table in the list of OIDs returned. Thus for an unpartitioned relation being
> attached, it will try to compare the constraints again. Am I correct?

Good catch, fixed.

> +         * comparing it to similarly-processed qual clauses, and may fail
> There are no "qual clauses" here only constraints :).

Oh, yes.  Text fixed.

> The testcase looks good to me.

Attached updated patches.

Thanks,
Amit

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