Thread: With commit 4e5fe9ad19, range partition missing handling for the NULLpartition key

With commit 4e5fe9ad19, range partition missing handling for the NULLpartition key

From

Rushabh Lathia

Date:

22 November 2017, 10:45:35

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.

postgres@112171=#INSERT INTO range_tab VALUES(NULL, 10);
ERROR: no partition of relation "range_tab" found for row
DETAIL: Partition key of the failing row contains (a) = (null).

Looking at the code partition_bound_cmp(), before 4e5fe9ad19 commit there
was a condition for the null values:

                     /*
                    * No range includes NULL, so this will be accepted by the
                    * default partition if there is one, and otherwise
                    * rejected.
                    */
                    for (i = 0; i < key->partnatts; i++)
                    {
                        if (isnull[i] &&
                            partition_bound_has_default(partdesc->boundinfo))
                        {
                            range_partkey_has_null = true;
                            break;
                        }

                        else if (isnull[i])
                        {
                            *failed_at = parent;
                            *failed_slot = slot;
                            result = -1;
                            goto error_exit;
                        }
                    }

But after commit, condition for isnull is missing. It doesn't look intentional,
is it?

Attaching patch to fix as well as regression test.

Thanks,
Rushabh Lathia

www.EnterpriseDB.com

Attachment

range_null_values.patch

Re: With commit 4e5fe9ad19, range partition missing handling for theNULL partition key

From

Amit Langote

Date:

22 November 2017, 12:08:20

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

Attachment

range_null_values-2.patch

Re: With commit 4e5fe9ad19, range partition missing handling for theNULL partition key

From

amul sul

Date:

22 November 2017, 14:42:19

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

Re: With commit 4e5fe9ad19, range partition missing handling for theNULL partition key

From

Amit Langote

Date:

22 November 2017, 15:06:28

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

Attachment

range_null_values-3.patch

Re: With commit 4e5fe9ad19, range partition missing handling for theNULL partition key

From

Rushabh Lathia

Date:

23 November 2017, 16:41:59

On Wed, Nov 22, 2017 at 2:36 PM, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> wrote:

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.

Patch looks good to me.

Rushabh Lathia

www.EnterpriseDB.com

Re: With commit 4e5fe9ad19, range partition missing handling for theNULL partition key

From

Robert Haas

Date:

29 November 2017, 01:16:43

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

Re: With commit 4e5fe9ad19, range partition missing handling for theNULL partition key

From

Amit Langote

Date:

29 November 2017, 06:40:12

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