Re: A bug in mapping attributes in ATExecAttachPartition()

Thanks for the review again.

On 2017/06/22 19:55, Ashutosh Bapat wrote:
> On Thu, Jun 15, 2017 at 4:06 PM, Amit Langote
> <Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)jp> wrote:
>>
>> Anyway, I tried to implement the refactoring in patch 0002, which is not
>> all of the patch 0001 that Jeevan posted. Please take a look. I wondered
>> if we should emit a NOTICE when an individual leaf partition validation
>> can be skipped?
>
> Yes. We emit an INFO for the table being attached. We want to do the
> same for the child. Or NOTICE In both the places.

Actually, I meant INFO.

>> No point in adding a new test if the answer to that is
>> no, I'd think.

Updated the patch 0002 so that an INFO message is printed for each leaf
partition and a test for the same.

>> Attaching here 0001 which fixes the bug (unchanged from the previous
>> version) and 0002 which implements the refactoring (and the feature to
>> look at the individual leaf partitions' constraints to see if validation
>> can be skipped.)
>
> Comments on 0001 patch.
> + *
> + * We request an exclusive lock on all the partitions, because we may
> + * decide later in this function to scan them to validate the new
> + * partition constraint.
> Does that mean that we may not scan the partitions later, in which the stronger
> lock we took was not needed. Is that right?

Yes. I wrote that comment thinking only about the deadlock hazard which
had then occurred to me, so the text somehow ended up being reflective of
that thinking. Please see the new comment, which hopefully is more
informative.

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

With the current code, we take AccessShareLock in the first call when
checking the circularity of inheritance. Then if attachRel doesn't have
the constraint to avoid the scan, we decide to look at individual
partitions (their rows, not constraints, as of now) when we take
AccessExclusiveLock. That might cause a deadlock (was able to reproduce
one using the debugger).

> if (!skip_validate)
> May be we should turn this into "else" condition of the "if" just above.

Yes, done.

> + /*
> + * We already collected the list of partitions, including the table
> + * named in the command itself, which should appear at the head of the
> + * list.
> + */
> + Assert(list_length(attachRel_children) >= 1 &&
> + linitial_oid(attachRel_children) == RelationGetRelid(attachRel));
> Probably the Assert should be moved next to find_all_inheritors(). But I don't
> see much value in this comment and the Assert at this place.

I agree, removed both.

>
> + /* Skip the original table if it's partitioned. */
> + if (part_relid == RelationGetRelid(attachRel) &&
> + attachRel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
> + continue;
> +
>
> There isn't much point in checking this for every child in the loop. Instead,
> we should remove attachRel from the attachRel_children if there are more than
> one elements in the list (which means the attachRel is partitioned, may be
> verify that by Asserting).

Rearranged code considering these comments.

> Comments on 0002 patch.
> Thanks for the refactoring. The code looks really good now.

Thanks.

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

> * This basically returns if the partrel's existing constraints, which
> returns "true". Add "otherwise returns false".
>
> if (constr != NULL)
> {
> ...
> }
> return false;
> May be you should return false when constr == NULL (I prefer !constr, but
> others may have different preferences.). That should save us one level of
> indentation. At the end just return whatever predicate_implied_by() returns.

Good suggestion, done.

Attached updated patches.

Thanks,
Amit