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

On Thu, Jun 15, 2017 at 2:12 PM, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:
> Thanks for the review.
>
> On 2017/06/15 16:08, Ashutosh Bapat wrote:
>> On Thu, Jun 15, 2017 at 10:46 AM, Amit Langote wrote:
>>> If we end up having to perform the validation scan and the table being
>>> attached is a partitioned table, we will scan its leaf partitions.  Each
>>> of those leaf partitions may have different attribute numbers for the
>>> partitioning columns, so we will need to do the mapping, which actually we
>>> do even today.  With this patch however, we apply mapping to the generated
>>> partition constraint so that it no longer bears the original parent's
>>> attribute numbers but those of the table being attached.  Down below where
>>> we map to the leaf partition's attribute numbers, we still pass the root
>>> partitioned table as the parent.  But it may so happen that the attnos
>>> appearing in the Vars can no longer be matched with any of the root
>>> table's attribute numbers, resulting in the following code in
>>> map_variable_attnos_mutator() to trigger an error:
>>>
>>> if (attno > context->map_length || context->attno_map[attno - 1] == 0)
>>>     elog(ERROR, "unexpected varattno %d in expression to be mapped",
>>>                  attno);
>>>
>>> Consider this example:
>>>
>>> root: (a, b, c) partition by list (a)
>>> intermediate: (b, c, ..dropped.., a) partition by list (b)
>>> leaf: (b, c, a) partition of intermediate
>>>
>>> When attaching intermediate to root, we will generate the partition
>>> constraint and after mapping, its Vars will have attno = 4.  When trying
>>> to map the same for leaf, we currently do map_partition_varattnos(expr, 1,
>>> leaf, root).  So, the innards of map_variable_attnos will try to look for
>>> an attribute with attno = 4 in root which there isn't, so the above error
>>> will occur.  We should really be passing intermediate as parent to the
>>> mapping routine.  With the previous patch's approach, we would pass root
>>> as the parent along with partConstraintOrig which would bear the root
>>> parent's attnos.
>>
>> Thanks for the explanation. So, your earlier patch did map Vars
>> correctly for the leaf partitions of the table being attached.
>
> Yes I think.
>
>>> Please find attached the updated patch.  In addition to the already
>>> described fixes, the patch also rearranges code so that a redundant AT
>>> work queue entry is avoided.  (Currently, we end up creating one for
>>> attachRel even if it's partitioned, although it's harmless because
>>> ATRewriteTables() knows to skip partitioned tables.)
>>
>> We are calling find_all_inheritors() on attachRel twice in this function, once
>> to avoid circularity and second time for scheduling a scan. Why can't call it
>> once and reuse the result?
>
> Hmm, avoiding calling it twice would be a good idea.
>
> Also, I noticed that there might be a deadlock hazard here due to the
> lock-strength-upgrade thing happening here across the two calls.  In the
> first call to find_all_inheritors(), we request AccessShareLock, whereas
> in the second, an AccessExclusiveLock.  That ought to be fixed anyway I'd
> think.
>
> So, the first (and the only after this change) call will request an
> AccessExclusiveLock, even though we may not scan the leaf partitions if a
> suitable constraint exists on the table being attached.  Note that
> previously, we would not have exclusive-locked the leaf partitions in such
> a case, although it was deadlock-prone/buggy anyway.
>
> The updated patch includes this fix.
>
>> On the default partitioning thread [1] Robert commented that we should try to
>> avoid queueing the subpartitions which have constraints that imply the new
>> partitioning constraint. I think that comment applies to this code, which the
>> refactoring patch has moved into a function. If you do this, instead of
>> duplicating the code to gather existing constraints, please create a function
>> for gathering constraints of a given relation and use it for the table being
>> attached as well as its partitions. Also, we should avoid matching
>> constraints for the table being attached twice, when it's not
>> partitioned.
>
> I guess you are talking about the case where the table being attached
> itself does not have a check constraint that would help avoid the scan,
> but its individual leaf partitions (if any) may.

Right.

>
>> Both of the above comments are not related to the bug that is being fixed, but
>> they apply to the same code where the bug exists. So instead of fixing it
>> twice, may be we should expand the scope of this work to cover other
>> refactoring needed in this area. That might save us some rebasing and commits.
>
> Are you saying that the patch posted on that thread should be brought over
> and discussed here?

Not the whole patch, but that one particular comment, which applies to
the existing code in ATExecAttachPartition(). If we fix the existing
code in ATExecAttachPartition(), the refactoring patch there will
inherit it when rebased.

-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company