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 <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
Attachments:
range_null_values.patchtext/x-patch; charset=US-ASCII; name=range_null_values.patchDownload
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);
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 -0500Centralize 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
Attachments:
range_null_values-2.patchtext/plain; charset=UTF-8; name=range_null_values-2.patchDownload
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);
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 -0500Centralize 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
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;
27default_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
Attachments:
range_null_values-3.patchtext/plain; charset=UTF-8; name=range_null_values-3.patchDownload
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);
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;
27default_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
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