Re: A bug in mapping attributes in ATExecAttachPartition()

From: Ashutosh Bapat <ashutosh(dot)bapat(at)enterprisedb(dot)com>
To: Amit Langote <Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)jp>
Cc: Robert Haas <robertmhaas(at)gmail(dot)com>, pgsql-hackers <pgsql-hackers(at)postgresql(dot)org>, Amit Langote <amitlangote09(at)gmail(dot)com>
Subject: Re: A bug in mapping attributes in ATExecAttachPartition()
Date: 2017-06-08 10:25:08
Message-ID: CAFjFpRd+6YKo-mKAXb2vbLmKCsCGZmbr_pAcPNjp1pm0krENAQ@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

On Thu, Jun 8, 2017 at 3:13 PM, Amit Langote
<Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)jp> wrote:
> On 2017/06/08 1:44, Robert Haas wrote:
>> On Wed, Jun 7, 2017 at 7:47 AM, Ashutosh Bapat
>> <ashutosh(dot)bapat(at)enterprisedb(dot)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 think this code could be removed entirely in light of commit
>> 3ec76ff1f2cf52e9b900349957b42d28128b7bc7.
>
> I am assuming you think that because now we emit IS NOT NULL constraint
> internally for any partition keys that do not allow null values (including
> all the keys of range partitions as of commit
> 3ec76ff1f2cf52e9b900349957b42d28128b7bc7). But those IS NOT NULL
> constraint expressions are inconclusive as far as the application of
> predicate_implied_by() to determine if we can skip the scan is concerned.
> So even if predicate_implied_by() returned 'true', we cannot conclude,
> just based on that result, that there are not any null values in the
> partition keys.

I am not able to understand this. Are you saying that
predicate_implied_by() returns true even when it's not implied when
NOT NULL constraints are involved? That sounds like a bug in
predicate_implied_by(), which should be fixed instead of adding code
to pepper over it?

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

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Ashutosh Bapat 2017-06-08 10:34:59 Re: Adding support for Default partition in partitioning
Previous Message Amit Langote 2017-06-08 09:43:18 Re: A bug in mapping attributes in ATExecAttachPartition()