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

On Thu, Jun 15, 2017 at 10:46 AM, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:
> 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.
>

I agree.

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

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

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.

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.

+            /*
+             * Adjust the constraint to match this partition.
+             *
+             * Since partConstraint contains attachRel's attnos due to the
+             * mapping we did just before attempting the proof above, we pass
+             * attachRel as the parent to map_partition_varattnos, not 'rel'
+             * which is the root parent.
+             */
May be reworded as "Adjust the partition constraints constructed for the table
being attached for the leaf partition being validated."

+CREATE TABLE part_7 (
+    a int,
+    b char,
+    CONSTRAINT check_a CHECK (a IS NOT NULL AND a = 7)
+) PARTITION BY LIST (b);
+CREATE TABLE part_7_a_null (
+    c int,
+    d int,
+    b char,
+    a int,    -- 'a' will have attnum = 4
+    CONSTRAINT check_b CHECK (b IS NULL OR b = 'a'),
+    CONSTRAINT check_a CHECK (a IS NOT NULL AND a = 7)
+);
Why not to use LIKE list_parted as you have done for part_6?

[1] https://postgr.es/m/CA+TgmoZ+54-z+VJrxeuMmSV8aDWmuEQcsE9iFfhh=ZybomcZnw@mail.gmail.com


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