Re: [HACKERS] Bug in get_partition_for_tuple
On 2017/03/27 23:54, Robert Haas wrote: > On Wed, Mar 22, 2017 at 4:00 AM, Amit Langote >wrote: >>> You're right, fixed that in the attached updated patch. >> >> Added this to the partitioning open items list, to avoid being forgotten >> about. >> >> https://wiki.postgresql.org/wiki/PostgreSQL_10_Open_Items#Partitioning > > Thanks for adding it there. I'm making a sweep through that list now, > or at least the parts of it that obvious pertain to my commits. This > patch looks good, so committed. Thanks. Regards, Amit -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Bug in get_partition_for_tuple
On Wed, Mar 22, 2017 at 4:00 AM, Amit Langotewrote: >> You're right, fixed that in the attached updated patch. > > Added this to the partitioning open items list, to avoid being forgotten > about. > > https://wiki.postgresql.org/wiki/PostgreSQL_10_Open_Items#Partitioning Thanks for adding it there. I'm making a sweep through that list now, or at least the parts of it that obvious pertain to my commits. This patch looks good, so committed. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Bug in get_partition_for_tuple
On 2017/03/13 14:41, Amit Langote wrote: > On 2017/03/13 14:31, Jeevan Ladhe wrote: >> However, In following comment in your test: >> >> -- check routing error through a list partitioned table when they key is >> null >> >> I think you want to say: >> >> -- check routing error through a list partitioned table when the key is null > > You're right, fixed that in the attached updated patch. Added this to the partitioning open items list, to avoid being forgotten about. https://wiki.postgresql.org/wiki/PostgreSQL_10_Open_Items#Partitioning Thanks, Amit -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Bug in get_partition_for_tuple
Hi Jeevan, On 2017/03/13 14:31, Jeevan Ladhe wrote: > Hi Amit, > > I was able to reproduce the crash, and with the attached patch the crash > goes > away. Also, "make check-world" passes clean. > > Patch looks good to me. Thanks for the review. > However, In following comment in your test: > > -- check routing error through a list partitioned table when they key is > null > > I think you want to say: > > -- check routing error through a list partitioned table when the key is null You're right, fixed that in the attached updated patch. Thanks, Amit >From dbdb0f8f5205a3abd4691c00febf196b853df43f Mon Sep 17 00:00:00 2001 From: amitDate: Fri, 10 Mar 2017 11:53:41 +0900 Subject: [PATCH] Fix a bug in get_partition_for_tuple Handle the NULL partition key more carefully in the case of list partitioning. --- src/backend/catalog/partition.c | 10 +++--- src/test/regress/expected/insert.out | 7 +++ src/test/regress/sql/insert.sql | 6 ++ 3 files changed, 20 insertions(+), 3 deletions(-) diff --git a/src/backend/catalog/partition.c b/src/backend/catalog/partition.c index e01ef864f0..a5ba7448eb 100644 --- a/src/backend/catalog/partition.c +++ b/src/backend/catalog/partition.c @@ -1726,10 +1726,14 @@ get_partition_for_tuple(PartitionDispatch *pd, errmsg("range partition key of row contains null"))); } - if (partdesc->boundinfo->has_null && isnull[0]) - /* Tuple maps to the null-accepting list partition */ + /* + * A null partition key is only acceptable if null-accepting list + * partition exists. + */ + cur_index = -1; + if (isnull[0] && partdesc->boundinfo->has_null) cur_index = partdesc->boundinfo->null_index; - else + else if (!isnull[0]) { /* Else bsearch in partdesc->boundinfo */ bool equal = false; diff --git a/src/test/regress/expected/insert.out b/src/test/regress/expected/insert.out index 116854e142..7fafa98212 100644 --- a/src/test/regress/expected/insert.out +++ b/src/test/regress/expected/insert.out @@ -365,6 +365,13 @@ DETAIL: Failing row contains (1, 2). insert into mlparted1 (a, b) values (2, 3); ERROR: new row for relation "mlparted11" violates partition constraint DETAIL: Failing row contains (3, 2). +-- check routing error through a list partitioned table when the key is null +create table lparted_nonullpart (a int, b char) partition by list (b); +create table lparted_nonullpart_a partition of lparted_nonullpart for values in ('a'); +insert into lparted_nonullpart values (1); +ERROR: no partition of relation "lparted_nonullpart" found for row +DETAIL: Partition key of the failing row contains (b) = (null). +drop table lparted_nonullpart; -- check that RETURNING works correctly with tuple-routing alter table mlparted drop constraint check_b; create table mlparted12 partition of mlparted1 for values from (5) to (10); diff --git a/src/test/regress/sql/insert.sql b/src/test/regress/sql/insert.sql index c56c3c22f8..f9c00705a2 100644 --- a/src/test/regress/sql/insert.sql +++ b/src/test/regress/sql/insert.sql @@ -226,6 +226,12 @@ insert into mlparted values (1, 2); -- selected by tuple-routing insert into mlparted1 (a, b) values (2, 3); +-- check routing error through a list partitioned table when the key is null +create table lparted_nonullpart (a int, b char) partition by list (b); +create table lparted_nonullpart_a partition of lparted_nonullpart for values in ('a'); +insert into lparted_nonullpart values (1); +drop table lparted_nonullpart; + -- check that RETURNING works correctly with tuple-routing alter table mlparted drop constraint check_b; create table mlparted12 partition of mlparted1 for values from (5) to (10); -- 2.11.0 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Bug in get_partition_for_tuple
Hi Amit, I was able to reproduce the crash, and with the attached patch the crash goes away. Also, "make check-world" passes clean. Patch looks good to me. However, In following comment in your test: -- check routing error through a list partitioned table when they key is null I think you want to say: -- check routing error through a list partitioned table when the key is null Thanks, Jeevan Ladhe On Fri, Mar 10, 2017 at 8:26 AM, Amit Langotewrote: > Just observed a crash due to thinko in the logic that handles NULL > partition key. Absence of null-accepting partition in this case should > have caused an error, instead the current code proceeds with comparison > resulting in crash. > > create table p (a int, b char) partition by list (b); > create table p1 partition of p for values in ('a'); > insert into p values (1); -- crashes > > Attached patch fixes that and adds a test. > > Thanks, > Amit > > > -- > Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) > To make changes to your subscription: > http://www.postgresql.org/mailpref/pgsql-hackers > >
[HACKERS] Bug in get_partition_for_tuple
Just observed a crash due to thinko in the logic that handles NULL partition key. Absence of null-accepting partition in this case should have caused an error, instead the current code proceeds with comparison resulting in crash. create table p (a int, b char) partition by list (b); create table p1 partition of p for values in ('a'); insert into p values (1); -- crashes Attached patch fixes that and adds a test. Thanks, Amit >From 41f4d396267a817419eb338ef437e2c33d6862d9 Mon Sep 17 00:00:00 2001 From: amitDate: Fri, 10 Mar 2017 11:53:41 +0900 Subject: [PATCH] Fix a bug in get_partition_for_tuple Handle the NULL partition key more carefully in the case of list partitioning. --- src/backend/catalog/partition.c | 10 +++--- src/test/regress/expected/insert.out | 7 +++ src/test/regress/sql/insert.sql | 6 ++ 3 files changed, 20 insertions(+), 3 deletions(-) diff --git a/src/backend/catalog/partition.c b/src/backend/catalog/partition.c index e01ef864f0..a5ba7448eb 100644 --- a/src/backend/catalog/partition.c +++ b/src/backend/catalog/partition.c @@ -1726,10 +1726,14 @@ get_partition_for_tuple(PartitionDispatch *pd, errmsg("range partition key of row contains null"))); } - if (partdesc->boundinfo->has_null && isnull[0]) - /* Tuple maps to the null-accepting list partition */ + /* + * A null partition key is only acceptable if null-accepting list + * partition exists. + */ + cur_index = -1; + if (isnull[0] && partdesc->boundinfo->has_null) cur_index = partdesc->boundinfo->null_index; - else + else if (!isnull[0]) { /* Else bsearch in partdesc->boundinfo */ bool equal = false; diff --git a/src/test/regress/expected/insert.out b/src/test/regress/expected/insert.out index 116854e142..9553fc597a 100644 --- a/src/test/regress/expected/insert.out +++ b/src/test/regress/expected/insert.out @@ -365,6 +365,13 @@ DETAIL: Failing row contains (1, 2). insert into mlparted1 (a, b) values (2, 3); ERROR: new row for relation "mlparted11" violates partition constraint DETAIL: Failing row contains (3, 2). +-- check routing error through a list partitioned table when they key is null +create table lparted_nonullpart (a int, b char) partition by list (b); +create table lparted_nonullpart_a partition of lparted_nonullpart for values in ('a'); +insert into lparted_nonullpart values (1); +ERROR: no partition of relation "lparted_nonullpart" found for row +DETAIL: Partition key of the failing row contains (b) = (null). +drop table lparted_nonullpart; -- check that RETURNING works correctly with tuple-routing alter table mlparted drop constraint check_b; create table mlparted12 partition of mlparted1 for values from (5) to (10); diff --git a/src/test/regress/sql/insert.sql b/src/test/regress/sql/insert.sql index c56c3c22f8..e66783bb9b 100644 --- a/src/test/regress/sql/insert.sql +++ b/src/test/regress/sql/insert.sql @@ -226,6 +226,12 @@ insert into mlparted values (1, 2); -- selected by tuple-routing insert into mlparted1 (a, b) values (2, 3); +-- check routing error through a list partitioned table when they key is null +create table lparted_nonullpart (a int, b char) partition by list (b); +create table lparted_nonullpart_a partition of lparted_nonullpart for values in ('a'); +insert into lparted_nonullpart values (1); +drop table lparted_nonullpart; + -- check that RETURNING works correctly with tuple-routing alter table mlparted drop constraint check_b; create table mlparted12 partition of mlparted1 for values from (5) to (10); -- 2.11.0 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers