Обсуждение: With commit 4e5fe9ad19, range partition missing handling for the NULLpartition key

Hi Rushabh,

On 2017/11/22 13:45, Rushabh Lathia wrote:
> Consider the below test:
> 
> CREATE TABLE range_tab(a int, b int) PARTITION BY RANGE(a);
> CREATE TABLE range_tab_p1 PARTITION OF range_tab FOR VALUES FROM (minvalue)
> TO (10);
> CREATE TABLE range_tab_p2 PARTITION OF range_tab FOR VALUES FROM (10) TO
> (20);
> CREATE TABLE range_tab_p3 PARTITION OF range_tab FOR VALUES FROM (20) TO
> (maxvalue);
> 
> INSERT INTO range_tab VALUES(NULL, 10);
> 
> Above insert should fail with an error "no partition of relation found for
> row".
> 
> Looking further I found that, this behaviour is changed after below commit:
> 
> commit 4e5fe9ad19e14af360de7970caa8b150436c9dec
> Author: Robert Haas <rhaas@postgresql.org>
> Date:   Wed Nov 15 10:23:28 2017 -0500
> 
>     Centralize executor-related partitioning code.
> 
>     Some code is moved from partition.c, which has grown very quickly
> lately;
>     splitting the executor parts out might help to keep it from getting
>     totally out of control.  Other code is moved from execMain.c.  All is
>     moved to a new file execPartition.c.  get_partition_for_tuple now has
>     a new interface that more clearly separates executor concerns from
>     generic concerns.
> 
>     Amit Langote.  A slight comment tweak by me.
> 
> Before above commit insert with NULL partition key in the range partition
> was throwing a proper error.

Oops, good catch.

> Attaching patch to fix as well as regression test.

Thanks for the patch.  About the code, how about do it like the attached
instead?

Thanks,
Amit

On Wed, Nov 22, 2017 at 11:38 AM, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:
> Hi Rushabh,
>
> On 2017/11/22 13:45, Rushabh Lathia wrote:
>> Consider the below test:
>>
>> CREATE TABLE range_tab(a int, b int) PARTITION BY RANGE(a);
>> CREATE TABLE range_tab_p1 PARTITION OF range_tab FOR VALUES FROM (minvalue)
>> TO (10);
>> CREATE TABLE range_tab_p2 PARTITION OF range_tab FOR VALUES FROM (10) TO
>> (20);
>> CREATE TABLE range_tab_p3 PARTITION OF range_tab FOR VALUES FROM (20) TO
>> (maxvalue);
>>
>> INSERT INTO range_tab VALUES(NULL, 10);
>>
>> Above insert should fail with an error "no partition of relation found for
>> row".
>>
>> Looking further I found that, this behaviour is changed after below commit:
>>
>> commit 4e5fe9ad19e14af360de7970caa8b150436c9dec
>> Author: Robert Haas <rhaas@postgresql.org>
>> Date:   Wed Nov 15 10:23:28 2017 -0500
>>
>>     Centralize executor-related partitioning code.
>>
>>     Some code is moved from partition.c, which has grown very quickly
>> lately;
>>     splitting the executor parts out might help to keep it from getting
>>     totally out of control.  Other code is moved from execMain.c.  All is
>>     moved to a new file execPartition.c.  get_partition_for_tuple now has
>>     a new interface that more clearly separates executor concerns from
>>     generic concerns.
>>
>>     Amit Langote.  A slight comment tweak by me.
>>
>> Before above commit insert with NULL partition key in the range partition
>> was throwing a proper error.
>
> Oops, good catch.
>
>> Attaching patch to fix as well as regression test.
>
> Thanks for the patch.  About the code, how about do it like the attached
> instead?
>

Looks good to me, even we can skip the following change in v2 patch:

19 @@ -2560,6 +2559,8 @@ get_partition_for_tuple(Relation relation,
Datum *values, bool *isnull)20                      */21                     part_index =
partdesc->boundinfo->indexes[bound_offset + 1];22                 }23 +               else24 +
part_index= partdesc->boundinfo->default_index;25             }26             break;27
 

default_index will get assign by following code in get_partition_for_tuple() :
  /*    * part_index < 0 means we failed to find a partition of this parent.    * Use the default partition, if there
isone.    */   if (part_index < 0)       part_index = partdesc->boundinfo->default_index;
 


Regards,
Amul

On 2017/11/22 17:42, amul sul wrote:
> On Wed, Nov 22, 2017 at 11:38 AM, Amit Langote wrote:
>> On 2017/11/22 13:45, Rushabh Lathia wrote:
>>> Attaching patch to fix as well as regression test.
>>
>> Thanks for the patch.  About the code, how about do it like the attached
>> instead?
>>
> 
> Looks good to me, even we can skip the following change in v2 patch:
> 
> 19 @@ -2560,6 +2559,8 @@ get_partition_for_tuple(Relation relation,
> Datum *values, bool *isnull)
>  20                      */
>  21                     part_index =
> partdesc->boundinfo->indexes[bound_offset + 1];
>  22                 }
>  23 +               else
>  24 +                   part_index = partdesc->boundinfo->default_index;
>  25             }
>  26             break;
>  27
> 
> default_index will get assign by following code in get_partition_for_tuple() :
> 
>    /*
>      * part_index < 0 means we failed to find a partition of this parent.
>      * Use the default partition, if there is one.
>      */
>     if (part_index < 0)
>         part_index = partdesc->boundinfo->default_index;

Good point.  Updated patch attached.

Thanks,
Amit

On Thu, Nov 23, 2017 at 5:41 AM, Rushabh Lathia
<rushabh.lathia@gmail.com> wrote:
>> Good point.  Updated patch attached.
>
> Thanks Amit.
>
> Patch looks good to me.

Committed, except I left out the comment tweak, which seemed irrelevant.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 2017/11/29 4:16, Robert Haas wrote:
> On Thu, Nov 23, 2017 at 5:41 AM, Rushabh Lathia
> <rushabh.lathia@gmail.com> wrote:
>>> Good point.  Updated patch attached.
>>
>> Thanks Amit.
>>
>> Patch looks good to me.
> 
> Committed, except I left out the comment tweak, which seemed irrelevant.

Thank you.

Regards,
Amit