Horton

Thread

Commits

  1. If a range-partitioned table has no default partition, reject null keys.

  2. Centralize executor-related partitioning code.

  1. With commit 4e5fe9ad19, range partition missing handling for the NULL partition key

    Rushabh Lathia <rushabh.lathia@gmail.com> — 2017-11-22T04:45:35Z

    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

  2. Re: With commit 4e5fe9ad19, range partition missing handling for the NULL partition key

    Amit Langote <langote_amit_f8@lab.ntt.co.jp> — 2017-11-22T06:08:20Z

    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

  3. Re: With commit 4e5fe9ad19, range partition missing handling for the NULL partition key

    Amul Sul <sulamul@gmail.com> — 2017-11-22T08:42:19Z

    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 +               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;


Regards,
Amul

  4. Re: With commit 4e5fe9ad19, range partition missing handling for the NULL partition key

    Amit Langote <langote_amit_f8@lab.ntt.co.jp> — 2017-11-22T09:06:28Z

    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

  5. Re: With commit 4e5fe9ad19, range partition missing handling for the NULL partition key

    Rushabh Lathia <rushabh.lathia@gmail.com> — 2017-11-23T10:41:59Z

    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

  6. Re: With commit 4e5fe9ad19, range partition missing handling for the NULL partition key

    Robert Haas <robertmhaas@gmail.com> — 2017-11-28T19:16:43Z

    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

  7. Re: With commit 4e5fe9ad19, range partition missing handling for the NULL partition key

    Amit Langote <langote_amit_f8@lab.ntt.co.jp> — 2017-11-29T00:40:12Z

    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