Re: With commit 4e5fe9ad19, range partition missing handling for the NULL partition key
On 2017/11/29 4:16, Robert Haas wrote: > On Thu, Nov 23, 2017 at 5:41 AM, Rushabh Lathia > 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
Re: With commit 4e5fe9ad19, range partition missing handling for the NULL partition key
On Thu, Nov 23, 2017 at 5:41 AM, Rushabh Lathia 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 the NULL partition key
On Wed, Nov 22, 2017 at 2:36 PM, Amit Langote 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 the NULL partition key
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 diff --git a/src/backend/catalog/partition.c b/src/backend/catalog/partition.c index 9a44cceb22..3b5df5164a 100644 --- a/src/backend/catalog/partition.c +++ b/src/backend/catalog/partition.c @@ -2531,16 +2531,14 @@ get_partition_for_tuple(Relation relation, Datum *values, bool *isnull) /* * No range includes NULL, so this will be accepted by the -* default partition if there is one, and otherwise -* rejected. +* default partition if there is one, otherwise rejected. */ for (i = 0; i < key->partnatts; i++) { - if (isnull[i] && - partition_bound_has_default(partdesc->boundinfo)) + if (isnull[i]) { range_partkey_has_null = true; - part_index = partdesc->boundinfo->default_index; + break; } } diff --git a/src/test/regress/expected/insert.out b/src/test/regress/expected/insert.out index 9d84ba4658..a0e3746e70 100644 --- a/src/test/regress/expected/insert.out +++ b/src/test/regress/expected/insert.out @@ -642,6 +642,10 @@ create table mcrparted2 partition of mcrparted for values from (10, 6, minvalue) create table mcrparted3 partition of mcrparted for values from (11, 1, 1) to (20, 10, 10); create table mcrparted4 partition of mcrparted for values from (21, minvalue, minvalue) to (30, 20, maxvalue); create table mcrparted5 partition of mcrparted for values from (30, 21, 20) to (maxvalue, maxvalue, maxvalue); +-- null not allowed in range partition +insert into mcrparted values (null, null, null); +ERROR: no partition of relation "mcrparted" found for row +DETAIL: Partition key of the failing row contains (a, abs(b), c) = (null, null, null). -- routed to mcrparted0 insert into mcrparted values (0, 1, 1); insert into mcrparted0 values (0, 1, 1); diff --git a/src/test/regress/sql/insert.sql b/src/test/regress/sql/insert.sql index 791817ba50..1c4491a6a2 100644 --- a/src/test/regress/sql/insert.sql +++ b/src/test/regress/sql/insert.sql @@ -417,6 +417,9 @@ create table mcrparted3 partition of mcrparted for values from (11, 1, 1) to (20 create table mcrparted4 partition of mcrparted for values from (21, minvalue, minvalue) to (30, 20, maxvalue); create table mcrparted5 partition of mcrparted for values from (30, 21, 20) to (maxvalue, maxvalue, maxvalue); +-- null not allowed in range partition +insert into mcrparted values (null, null, null); + -- routed to mcrparted0 insert into mcrparted values (0, 1, 1); insert into mcrparted0 values (0, 1, 1);
Re: With commit 4e5fe9ad19, range partition missing handling for the NULL partition key
On Wed, Nov 22, 2017 at 11:38 AM, Amit Langote 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 >> 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
Re: With commit 4e5fe9ad19, range partition missing handling for the NULL partition 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 > 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 diff --git a/src/backend/catalog/partition.c b/src/backend/catalog/partition.c index 9a44cceb22..a87dacc057 100644 --- a/src/backend/catalog/partition.c +++ b/src/backend/catalog/partition.c @@ -2536,11 +2536,10 @@ get_partition_for_tuple(Relation relation, Datum *values, bool *isnull) */ for (i = 0; i < key->partnatts; i++) { - if (isnull[i] && - partition_bound_has_default(partdesc->boundinfo)) + if (isnull[i]) { range_partkey_has_null = true; - part_index = partdesc->boundinfo->default_index; + break; } } @@ -2560,6 +2559,8 @@ get_partition_for_tuple(Relation relation, Datum *values, bool *isnull) */ part_index = partdesc->boundinfo->indexes[bound_offset + 1]; } + else + part_index = partdesc->boundinfo->default_index; } break; diff --git a/src/test/regress/expected/insert.out b/src/test/regress/expected/insert.out index 9d84ba4658..a0e3746e70 100644 --- a/src/test/regress/expected/insert.out +++ b/src/test/regress/expected/insert.out @@ -642,6 +642,10 @@ create table mcrparted2 partition of mcrparted for values from (10, 6, minvalue) create table mcrparted3 partition of mcrparted for values from (11, 1, 1) to (20, 10, 10); create table mcrparted4 partition of mcrparted for values from (21, minvalue, minvalue) to (30, 20, maxvalue); create table mcrparted5 partition of mcrparted for values from (30, 21, 20) to (maxvalue, maxvalue, maxvalue); +-- null not allowed in range partition +insert into mcrparted values (null, null, null); +ERROR: no partition of relation "mcrparted" found for row +DETAIL: Partition key of the failing row contains (a, abs(b), c) = (null, null, null). -- routed to mcrparted0 insert into mcrparted values (0, 1, 1); insert into mcrparted0 values (0, 1, 1); diff --git a/src/test/regress/sql/insert.sql b/src/test/regress/sql/insert.sql index 791817ba50..1c4491a6a2 100644 --- a/src/test/regress/sql/insert.sql +++ b/src/test/regress/sql/insert.sql @@ -417,6 +417,9 @@ create table mcrparted3 partition of mcrparted for values from (11, 1, 1) to (20 create table mcrparted4 partition of mcrparted for values from (21, minvalue, minvalue) to (30, 20, maxvalue); create table mcrparted5 partition of mcrparted for values from (30, 21, 20) to (maxvalue, maxvalue, maxvalue); +-- null not allowed in range partition +insert into mcrparted values (null, null, null); + -- routed to mcrparted0 insert into mcrparted values (0, 1, 1); insert into mcrparted0 values (0, 1, 1);
With commit 4e5fe9ad19, range partition missing handling for the NULL partition key
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 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 diff --git a/src/backend/catalog/partition.c b/src/backend/catalog/partition.c index 67d4c2a..b62e8f5 100644 --- a/src/backend/catalog/partition.c +++ b/src/backend/catalog/partition.c @@ -2541,6 +2541,11 @@ get_partition_for_tuple(Relation relation, Datum *values, bool *isnull) range_partkey_has_null = true; part_index = partdesc->boundinfo->default_index; } + else if(isnull[i]) + { + range_partkey_has_null = true; + part_index = -1; + } } if (!range_partkey_has_null) diff --git a/src/test/regress/expected/insert.out b/src/test/regress/expected/insert.out index 9d84ba4..a0e3746 100644 --- a/src/test/regress/expected/insert.out +++ b/src/test/regress/expected/insert.out @@ -642,6 +642,10 @@ create table mcrparted2 partition of mcrparted for values from (10, 6, minvalue) create table mcrparted3 partition of mcrparted for values from (11, 1, 1) to (20, 10, 10); create table mcrparted4 partition of mcrparted for values from (21, minvalue, minvalue) to (30, 20, maxvalue); create table mcrparted5 partition of mcrparted for values from (30, 21, 20) to (maxvalue, maxvalue, maxvalue); +-- null not allowed in range partition +insert into mcrparted values (null, null, null); +ERROR: no partition of relation "mcrparted" found for row +DETAIL: Partition key of the failing row contains (a, abs(b), c) = (null, null, null). -- routed to mcrparted0 insert into mcrparted values (0, 1, 1); insert into mcrparted0 values (0, 1, 1); diff --git a/src/test/regress/sql/insert.sql b/src/test/regress/sql/insert.sql index 791817b..1c4491a 100644 --- a/src/test/regress/sql/insert.sql +++ b/src/test/regress/sql/insert.sql @@ -417,6 +417,9 @@ create table mcrparted3 partition of mcrparted for values from (11, 1, 1) to (20 create table mcrparted4 partition of mcrparted for values from (21, minvalue, minvalue) to (30, 20, maxvalue); create table mcrparted5 partition of mcrparted for values from (30, 21, 20) to (maxvalue, maxvalue, maxvalue); +-- null not allowed in range partition +insert into mcrparted values (null, null, null); + -- routed to mcrparted0 insert into mcrparted values (0, 1, 1); insert into mcrparted0 values (0, 1, 1);