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-09 11:49:45
Message-ID: CAFjFpReu22z0YX7a885Eb=_rx_Z9dBLXOmjk5TegaP0PAjox7w@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

May be we should pass a flag to predicate_implied_by() to handle NULL
behaviour for CHECK constraints. Partitioning has shown that it needs
to use predicate_implied_by() for comparing constraints and there may
be other cases that can come up in future. Instead of handling it
outside predicate_implied_by() we may want to change it under a flag.

On Fri, Jun 9, 2017 at 11:43 AM, Amit Langote
<Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)jp> wrote:
> 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(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 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

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

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Peter Eisentraut 2017-06-09 12:31:17 Re: walsender termination error messages worse in v10
Previous Message Ashutosh Bapat 2017-06-09 11:47:02 Re: A bug in mapping attributes in ATExecAttachPartition()