Re: A bug in mapping attributes in ATExecAttachPartition()

On 2017/06/08 18:43, Amit Langote wrote:
> On 2017/06/08 1:44, Robert Haas wrote:
>> On Wed, Jun 7, 2017 at 7:47 AM, Ashutosh Bapat
>> <ashutosh.bapat@enterprisedb.com> wrote:
>>> In ATExecAttachPartition() there's following code
>>>
>>> 13715         partnatts = get_partition_natts(key);
>>> 13716         for (i = 0; i < partnatts; i++)
>>> 13717         {
>>> 13718             AttrNumber  partattno;
>>> 13719
>>> 13720             partattno = get_partition_col_attnum(key, i);
>>> 13721
>>> 13722             /* If partition key is an expression, must not skip
>>> validation */
>>> 13723             if (!partition_accepts_null &&
>>> 13724                 (partattno == 0 ||
>>> 13725                  !bms_is_member(partattno, not_null_attrs)))
>>> 13726                 skip_validate = false;
>>> 13727         }
>>>
>>> partattno obtained from the partition key is the attribute number of
>>> the partitioned table but not_null_attrs contains the attribute
>>> numbers of attributes of the table being attached which have NOT NULL
>>> constraint on them. But the attribute numbers of partitioned table and
>>> the table being attached may not agree i.e. partition key attribute in
>>> partitioned table may have a different position in the table being
>>> attached. So, this code looks buggy. Probably we don't have a test
>>> which tests this code with different attribute order between
>>> partitioned table and the table being attached. I didn't get time to
>>> actually construct a testcase and test it.
> 
> There seem to be couple of bugs here:
> 
> 1. When partition's key attributes differ in ordering from the parent,
>    predicate_implied_by() will give up due to structural inequality of
>    Vars in the expressions.  By fixing this, we can get it to return
>    'true' when it's really so.
> 
> 2. As you said, we store partition's attribute numbers in the
>    not_null_attrs bitmap, but then check partattno (which is the parent's
>    attribute number which might differ) against the bitmap, which seems
>    like it might produce incorrect result.  If, for example,
>    predicate_implied_by() set skip_validate to true, and the above code
>    failed to set skip_validate to false where it should have, then we
>    would wrongly end up skipping the scan.  That is, rows in the partition
>    will contain null values whereas the partition constraint does not
>    allow it.  It's hard to reproduce this currently, because with
>    different ordering of attributes, predicate_refute_by() never returns
>    true (as mentioned in 1 above), so skip_validate does not need to be
>    set to false again.
> 
> Consider this example:
> 
> create table p (a int, b char) partition by list (a);
> 
> create table p1 (b char not null, a int check (a in (1)));
> insert into p1 values ('b', null);
> 
> Note that not_null_attrs for p1 will contain 1 corresponding to column b,
> which matches key attribute of the parent, that is 1, corresponding to
> column a.  Hence we end up wrongly concluding that p1's partition key
> column does not allow nulls.

[ ... ]

> I am working on a patch to fix the above mentioned issues and will post
> the same no later than Friday.

And here is the patch.

Thanks,
Amit