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

Thanks for taking a look.

On 2017/06/14 20:06, Ashutosh Bapat wrote:
> On Wed, Jun 14, 2017 at 9:20 AM, Amit Langote
> <Langote_Amit_f8@lab.ntt.co.jp> wrote:
>>
>> By the way, I mentioned an existing problem in one of the earlier emails
>> on this thread about differing attribute numbers in the table being
>> attached causing predicate_implied_by() to give up due to structural
>> inequality of Vars.  To clarify: table's check constraints will bear the
>> table's attribute numbers whereas the partition constraint generated using
>> get_qual_for_partbound() (the predicate) bears the parent's attribute
>> numbers.  That results in Var arguments of the expressions passed to
>> predicate_implied_by() not matching and causing the latter to return
>> failure prematurely.  Attached find a patch to fix that that applies on
>> top of your patch (added a test too).
> 
> +    * Adjust the generated constraint to match this partition's attribute
> +    * numbers.  Save the original to be used later if we decide to proceed
> +    * with the validation scan after all.
> +    */
> +   partConstraintOrig = copyObject(partConstraint);
> +   partConstraint = map_partition_varattnos(partConstraint, 1, attachRel,
> +                                            rel);
> +
> If the partition has different column order than the parent, its heap will also
> have different column order. I am not able to understand the purpose of using
> original constraints for validation using scan. Shouldn't we just use the
> mapped constraint expressions?

Actually, I dropped the approach of using partConstraintOrig altogether
from the latest updated patch.  I will explain the problem I was trying to
solve with that approach, which is now replaced in the new patch by, I
think, a more correct solution.

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.

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

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