BUG #15212: Default values in partition tables don't work as expected and allow NOT NULL violation
The following bug has been logged on the website:
Bug reference: 15212
Logged by: Jürgen Strobel
Email address: juergen+postgresql@strobel.info
PostgreSQL version: 10.4
Operating system: Debian
Description:
I found unexpected behavior when playing around with declarative
partitioning.
First, any way to define defaults on (child) partition tables is silently
ignored when inserting into the master table, but not when inserting into
the child table. The easiest way to reproduce this is:
jue=> create table ptest (a int, b int) partition by list (a);
CREATE TABLE
jue=> create table ptest1 partition of ptest (b default 7) for values in
(1);
CREATE TABLE
jue=> insert into ptest (a) values (1);
INSERT 0 1
jue=> table ptest;
a | b
---+---
1 |
(1 row)
jue=> insert into ptest1 (a) values (1);
INSERT 0 1
jue=> table ptest;
a | b
---+---
1 |
1 | 7
(2 rows)
Second, this is a way to violate a NOT NULL constraint, presumably because a
default value should be applied later but isn't:
jue=> create table ptest (a int, b int not null) partition by list (a);
CREATE TABLE
jue=> create table ptest1 partition of ptest (b default 7) for values in
(1);
CREATE TABLE
jue=> insert into ptest (a) values (1);
INSERT 0 1
jue=> select * from ptest where b is null;
a | b
---+---
1 |
(1 row)
The same happens for defaults using nextval(sequence), either if specified
directly or as SERIAL columns with ALTER TABLE ... ATTACH PARTITION. My
current workaround is to use a before-row trigger to apply the default
value.
Hello.
On 2018/05/28 9:30, PG Bug reporting form wrote:
The following bug has been logged on the website:
Bug reference: 15212
Logged by: Jürgen Strobel
Email address: juergen+postgresql@strobel.info
PostgreSQL version: 10.4
Operating system: Debian
Description:I found unexpected behavior when playing around with declarative
partitioning.
Thank you for reporting this and sorry it took a while to reply here.
First, any way to define defaults on (child) partition tables is silently
ignored when inserting into the master table, but not when inserting into
the child table. The easiest way to reproduce this is:jue=> create table ptest (a int, b int) partition by list (a);
CREATE TABLE
jue=> create table ptest1 partition of ptest (b default 7) for values in
(1);
CREATE TABLE
jue=> insert into ptest (a) values (1);
INSERT 0 1
jue=> table ptest;
a | b
---+---
1 |
(1 row)jue=> insert into ptest1 (a) values (1);
INSERT 0 1
jue=> table ptest;
a | b
---+---
1 |
1 | 7
(2 rows)The same happens for defaults using nextval(sequence), either if specified
directly or as SERIAL columns with ALTER TABLE ... ATTACH PARTITION.
Hmm, so we provide the ability to specify default values per partition,
but it is not applied when inserting through the parent. I'd like to hear
from others on whether we should fix things so that we fill the
partition's default value for a given column if it's null in the input
tuple, after that tuple is routed to that partition. It does seem like a
inconvenience to have to do it through workarounds like a BR trigger.
Actually, default value substitution happens much earlier in the query
rewrite phase, whereas the partition to actually insert the tuple into
(that is, tuple routing) is determined much later during the execution of
the query. So fixing this will require some work.
Second, this is a way to violate a NOT NULL constraint, presumably because a
default value should be applied later but isn't:jue=> create table ptest (a int, b int not null) partition by list (a);
CREATE TABLE
jue=> create table ptest1 partition of ptest (b default 7) for values in
(1);
CREATE TABLE
jue=> insert into ptest (a) values (1);
INSERT 0 1
jue=> select * from ptest where b is null;
a | b
---+---
1 |
(1 row)
This is clearly a bug of CREATE TABLE .. PARTITION OF. It seems that the
parent's NOT NULL constraint is not copied to the partition when a clause
to set other column options, such as default 7 above, is used in the
command to create a partition. It *is* successfully copied when such a
clause is not specified. For example, same example but without the
default value clause will lead to correct behavior wrt NOT NULL constraint.
create table p (a int, b int not null) partition by list (a);
-- note there is no (b default 7) clause being used here
create table p1 partition of p for values in (1);
-- NOT NULL constraint is correctly enforced
insert into p values (1);
ERROR: null value in column "b" violates not-null constraint
DETAIL: Failing row contains (1, null).
Attached patches fix that for PG 10 (patch filename starting with PG10-)
and HEAD branches, respectively.
Thanks,
Amit
Attachments:
0001-Fix-bug-that-partition-won-t-inherit-NOT-NULL-if-def.patchtext/plain; charset=UTF-8; name=0001-Fix-bug-that-partition-won-t-inherit-NOT-NULL-if-def.patchDownload
From fa7945ba199108ebc78a699e99e81d9074f41c91 Mon Sep 17 00:00:00 2001
From: amit <amitlangote09@gmail.com>
Date: Wed, 6 Jun 2018 16:46:46 +0900
Subject: [PATCH] Fix bug that partition won't inherit NOT NULL if default
specified
---
src/backend/commands/tablecmds.c | 4 +++-
src/test/regress/expected/create_table.out | 12 ++++++++++++
src/test/regress/sql/create_table.sql | 6 ++++++
3 files changed, 21 insertions(+), 1 deletion(-)
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 0e95037dcf..1773625a1a 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -2413,7 +2413,9 @@ MergeAttributes(List *schema, List *supers, char relpersistence,
*/
if (coldef->is_from_parent)
{
- coldef->is_not_null = restdef->is_not_null;
+ /* Do not override parent's NOT NULL constraint. */
+ if (restdef->is_not_null)
+ coldef->is_not_null = restdef->is_not_null;
coldef->raw_default = restdef->raw_default;
coldef->cooked_default = restdef->cooked_default;
coldef->constraints = restdef->constraints;
diff --git a/src/test/regress/expected/create_table.out b/src/test/regress/expected/create_table.out
index 470fca0cab..bca8f9912e 100644
--- a/src/test/regress/expected/create_table.out
+++ b/src/test/regress/expected/create_table.out
@@ -727,6 +727,18 @@ ERROR: column "c" named in partition key does not exist
CREATE TABLE part_c PARTITION OF parted (b WITH OPTIONS NOT NULL DEFAULT 0) FOR VALUES IN ('c') PARTITION BY RANGE ((b));
-- create a level-2 partition
CREATE TABLE part_c_1_10 PARTITION OF part_c FOR VALUES FROM (1) TO (10);
+-- check inheriting NOT NULL constraint works correctly
+create table parted_notnull_inh_test (a int, b int not null default 0) partition by list (a);
+create table parted_notnull_inh_test1 partition of parted_notnull_inh_test (b default 1) for values in (1);
+\d parted_notnull_inh_test1
+ Table "public.parted_notnull_inh_test1"
+ Column | Type | Collation | Nullable | Default
+--------+---------+-----------+----------+---------
+ a | integer | | |
+ b | integer | | not null | 1
+Partition of: parted_notnull_inh_test FOR VALUES IN (1)
+
+drop table parted_notnull_inh_test;
-- Partition bound in describe output
\d+ part_b
Table "public.part_b"
diff --git a/src/test/regress/sql/create_table.sql b/src/test/regress/sql/create_table.sql
index 140bf41f76..b9ca2dfccf 100644
--- a/src/test/regress/sql/create_table.sql
+++ b/src/test/regress/sql/create_table.sql
@@ -659,6 +659,12 @@ CREATE TABLE part_c PARTITION OF parted (b WITH OPTIONS NOT NULL DEFAULT 0) FOR
-- create a level-2 partition
CREATE TABLE part_c_1_10 PARTITION OF part_c FOR VALUES FROM (1) TO (10);
+-- check inheriting NOT NULL constraint works correctly
+create table parted_notnull_inh_test (a int, b int not null default 0) partition by list (a);
+create table parted_notnull_inh_test1 partition of parted_notnull_inh_test (b default 1) for values in (1);
+\d parted_notnull_inh_test1
+drop table parted_notnull_inh_test;
+
-- Partition bound in describe output
\d+ part_b
--
2.11.0
PG10-0001-Fix-bug-that-partition-won-t-inherit-NOT-NULL-if-def.patchtext/plain; charset=UTF-8; name=PG10-0001-Fix-bug-that-partition-won-t-inherit-NOT-NULL-if-def.patchDownload
From 8c7cd78a7ede9cd16ef4393aca828704e11d2f1e Mon Sep 17 00:00:00 2001
From: amit <amitlangote09@gmail.com>
Date: Wed, 6 Jun 2018 16:46:46 +0900
Subject: [PATCH] Fix bug that partition won't inherit NOT NULL if default
specified
---
src/backend/commands/tablecmds.c | 4 +++-
src/test/regress/expected/create_table.out | 12 ++++++++++++
src/test/regress/sql/create_table.sql | 6 ++++++
3 files changed, 21 insertions(+), 1 deletion(-)
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index cb7d866710..eeddca7190 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -2200,7 +2200,9 @@ MergeAttributes(List *schema, List *supers, char relpersistence,
*/
if (coldef->is_from_parent)
{
- coldef->is_not_null = restdef->is_not_null;
+ /* Do not override parent's NOT NULL constraint. */
+ if (restdef->is_not_null)
+ coldef->is_not_null = restdef->is_not_null;
coldef->raw_default = restdef->raw_default;
coldef->cooked_default = restdef->cooked_default;
coldef->constraints = restdef->constraints;
diff --git a/src/test/regress/expected/create_table.out b/src/test/regress/expected/create_table.out
index 68dda4c0db..f801ea1e98 100644
--- a/src/test/regress/expected/create_table.out
+++ b/src/test/regress/expected/create_table.out
@@ -662,6 +662,18 @@ ERROR: column "c" named in partition key does not exist
CREATE TABLE part_c PARTITION OF parted (b WITH OPTIONS NOT NULL DEFAULT 0) FOR VALUES IN ('c') PARTITION BY RANGE ((b));
-- create a level-2 partition
CREATE TABLE part_c_1_10 PARTITION OF part_c FOR VALUES FROM (1) TO (10);
+-- check inheriting NOT NULL constraint works correctly
+create table parted_notnull_inh_test (a int, b int not null default 0) partition by list (a);
+create table parted_notnull_inh_test1 partition of parted_notnull_inh_test (b default 1) for values in (1);
+\d parted_notnull_inh_test1
+ Table "public.parted_notnull_inh_test1"
+ Column | Type | Collation | Nullable | Default
+--------+---------+-----------+----------+---------
+ a | integer | | |
+ b | integer | | not null | 1
+Partition of: parted_notnull_inh_test FOR VALUES IN (1)
+
+drop table parted_notnull_inh_test;
-- Partition bound in describe output
\d+ part_b
Table "public.part_b"
diff --git a/src/test/regress/sql/create_table.sql b/src/test/regress/sql/create_table.sql
index 89e1059a71..da9edc26df 100644
--- a/src/test/regress/sql/create_table.sql
+++ b/src/test/regress/sql/create_table.sql
@@ -609,6 +609,12 @@ CREATE TABLE part_c PARTITION OF parted (b WITH OPTIONS NOT NULL DEFAULT 0) FOR
-- create a level-2 partition
CREATE TABLE part_c_1_10 PARTITION OF part_c FOR VALUES FROM (1) TO (10);
+-- check inheriting NOT NULL constraint works correctly
+create table parted_notnull_inh_test (a int, b int not null default 0) partition by list (a);
+create table parted_notnull_inh_test1 partition of parted_notnull_inh_test (b default 1) for values in (1);
+\d parted_notnull_inh_test1
+drop table parted_notnull_inh_test;
+
-- Partition bound in describe output
\d+ part_b
--
2.11.0
On 6 June 2018 at 10:00, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> wrote:
Hello.On 2018/05/28 9:30, PG Bug reporting form wrote:
The following bug has been logged on the website:
Bug reference: 15212
Logged by: Jürgen Strobel
Email address: juergen+postgresql@strobel.info
PostgreSQL version: 10.4
Operating system: Debian
Description:I found unexpected behavior when playing around with declarative
partitioning.
First, any way to define defaults on (child) partition tables is silently
ignored when inserting into the master table, but not when inserting into
the child table.Hmm, so we provide the ability to specify default values per partition,
but it is not applied when inserting through the parent. I'd like to hear
from others on whether we should fix things so that we fill the
partition's default value for a given column if it's null in the input
tuple, after that tuple is routed to that partition. It does seem like a
inconvenience to have to do it through workarounds like a BR trigger.Actually, default value substitution happens much earlier in the query
rewrite phase, whereas the partition to actually insert the tuple into
(that is, tuple routing) is determined much later during the execution of
the query. So fixing this will require some work.
Well, since documentation says that partitioning build on top of inheritance,
and for inheritance:
If the new table explicitly specifies a default value for the column, this
default overrides any defaults from inherited declarations of the column.
So one may think it should be the same for partitioning as well.
On 7 June 2018 at 15:53, Dmitry Dolgov <9erthalion6@gmail.com> wrote:
On 6 June 2018 at 10:00, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> wrote:
Hello.On 2018/05/28 9:30, PG Bug reporting form wrote:
The following bug has been logged on the website:
Bug reference: 15212
Logged by: Jürgen Strobel
Email address: juergen+postgresql@strobel.info
PostgreSQL version: 10.4
Operating system: Debian
Description:I found unexpected behavior when playing around with declarative
partitioning.
First, any way to define defaults on (child) partition tables is silently
ignored when inserting into the master table, but not when inserting into
the child table.Hmm, so we provide the ability to specify default values per partition,
but it is not applied when inserting through the parent. I'd like to hear
from others on whether we should fix things so that we fill the
partition's default value for a given column if it's null in the input
tuple, after that tuple is routed to that partition. It does seem like a
inconvenience to have to do it through workarounds like a BR trigger.Actually, default value substitution happens much earlier in the query
rewrite phase, whereas the partition to actually insert the tuple into
(that is, tuple routing) is determined much later during the execution of
the query. So fixing this will require some work.Well, since documentation says that partitioning build on top of inheritance,
and for inheritance:If the new table explicitly specifies a default value for the column, this
default overrides any defaults from inherited declarations of the column.So one may think it should be the same for partitioning as well.
"The same for partitioning" - I mean the same approach when in all the
situations (whether it's an insert into a parent table or a partition) a
partition default value will take precedence.
Hi.
On 2018/06/07 23:08, Dmitry Dolgov wrote:
On 7 June 2018 at 15:53, Dmitry Dolgov <9erthalion6@gmail.com> wrote:
On 6 June 2018 at 10:00, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> wrote:
On 2018/05/28 9:30, PG Bug reporting form wrote:The following bug has been logged on the website:
Bug reference: 15212
Logged by: Jürgen Strobel
Email address: juergen+postgresql@strobel.info
PostgreSQL version: 10.4
Operating system: Debian
Description:I found unexpected behavior when playing around with declarative
partitioning.
First, any way to define defaults on (child) partition tables is silently
ignored when inserting into the master table, but not when inserting into
the child table.Hmm, so we provide the ability to specify default values per partition,
but it is not applied when inserting through the parent. I'd like to hear
from others on whether we should fix things so that we fill the
partition's default value for a given column if it's null in the input
tuple, after that tuple is routed to that partition. It does seem like a
inconvenience to have to do it through workarounds like a BR trigger.Actually, default value substitution happens much earlier in the query
rewrite phase, whereas the partition to actually insert the tuple into
(that is, tuple routing) is determined much later during the execution of
the query. So fixing this will require some work.Well, since documentation says that partitioning build on top of inheritance,
and for inheritance:If the new table explicitly specifies a default value for the column, this
default overrides any defaults from inherited declarations of the column.So one may think it should be the same for partitioning as well.
"The same for partitioning" - I mean the same approach when in all the
situations (whether it's an insert into a parent table or a partition) a
partition default value will take precedence.
I think you have a point. Before partitioning, one would either insert
directly into the child table or use a trigger to redirect an insert on
parent into one of the child tables. In both cases, child table's default
value would be used, because the insert query would mention the child
table name.
With partitioning, inserts into parent are internally handled in a way
that bypasses the processing which would otherwise fill a partition's own
default values for columns whose value is missing in the input row.
That said, I'd like to make sure before writing a patch if the feature of
being able to set defaults on partition level is something that users will
want in the long run.
Thanks,
Amit
On 06/13/2018 11:42 AM, Amit Langote wrote:
Hi.
On 2018/06/07 23:08, Dmitry Dolgov wrote:
On 7 June 2018 at 15:53, Dmitry Dolgov <9erthalion6@gmail.com> wrote:
On 6 June 2018 at 10:00, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> wrote:
On 2018/05/28 9:30, PG Bug reporting form wrote:The following bug has been logged on the website:
Bug reference: 15212
Logged by: Jürgen Strobel
Email address: juergen+postgresql@strobel.info
PostgreSQL version: 10.4
Operating system: Debian
Description:I found unexpected behavior when playing around with declarative
partitioning.
First, any way to define defaults on (child) partition tables is silently
ignored when inserting into the master table, but not when inserting into
the child table.
...
Well, since documentation says that partitioning build on top of inheritance,
and for inheritance:If the new table explicitly specifies a default value for the column, this
default overrides any defaults from inherited declarations of the column.So one may think it should be the same for partitioning as well.
"The same for partitioning" - I mean the same approach when in all the
situations (whether it's an insert into a parent table or a partition) a
partition default value will take precedence.I think you have a point. Before partitioning, one would either insert
directly into the child table or use a trigger to redirect an insert on
parent into one of the child tables. In both cases, child table's default
value would be used, because the insert query would mention the child
table name.With partitioning, inserts into parent are internally handled in a way
that bypasses the processing which would otherwise fill a partition's own
default values for columns whose value is missing in the input row.That said, I'd like to make sure before writing a patch if the feature of
being able to set defaults on partition level is something that users will
want in the long run.Thanks,
Amit
I agree, and I imagine especially being able to use per-partition
sequences would be a common use case. That was my motivation when I
discovered it, and it was very counter intuitive to me.
Thanks for the NULL violation patch btw.
Best regards,
Jürgen
On Wed, Jun 13, 2018 at 03:39:04PM +0200, Jürgen Strobel wrote:
I agree, and I imagine especially being able to use per-partition
sequences would be a common use case. That was my motivation when I
discovered it, and it was very counter intuitive to me.Thanks for the NULL violation patch btw.
Please note that I have added this thread to the Open item page, under
the older bug queue, and that a CF entry has been added so as we don't
lost track of it:
https://commitfest.postgresql.org/18/1671/
--
Michael
On 2018/06/14 10:57, Michael Paquier wrote:
On Wed, Jun 13, 2018 at 03:39:04PM +0200, J�rgen Strobel wrote:
I agree, and I imagine especially being able to use per-partition
sequences would be a common use case. That was my motivation when I
discovered it, and it was very counter intuitive to me.Thanks for the NULL violation patch btw.
Please note that I have added this thread to the Open item page, under
the older bug queue, and that a CF entry has been added so as we don't
lost track of it:
Thank you, Michael.
Just to be sure, you meant to add the open item for the NOT NULL violation
bug fix patch mainly or all of what appears to be wrong here? About
partition-specific defaults being ignored when inserting through the
parent, do you think we should consider it a bug? I could imagine fixing
the documentation to say that partition-specific defaults are only honored
if directly inserted into and ignored otherwise.
Thanks,
Amit
I didn't see any hackers thread linked to this CF entry. Hence sending this mail through CF app.
The patch looks good to me. It applies cleanly, compiles cleanly and make check passes.
I think you could avoid condition
+ /* Do not override parent's NOT NULL constraint. */
+ if (restdef->is_not_null)
+ coldef->is_not_null = restdef->is_not_null;by rewriting this line as
coldef->is_not_null = coldef->is_not_null || restdef->is_not_null;
The comment looks a bit odd either way since we are changing coldef->is_not_null based on restdef->is_not_null. That's because of the nature of boolean variables. May be a bit of explanation is needed.
On a side note, I see
coldef->constraints = restdef->constraints;
Shouldn't we merge the constraints instead of just overwriting those?
--
Best Wishesh
Ashutosh
Thanks Ashutosh.
On 2018/07/10 22:50, Ashutosh Bapat wrote:
I didn't see any hackers thread linked to this CF entry. Hence sending this mail through CF app.
Hmm, yes. I hadn't posted the patch to -hackers.
The patch looks good to me. It applies cleanly, compiles cleanly and make check passes.
I think you could avoid condition + /* Do not override parent's NOT NULL constraint. */ + if (restdef->is_not_null) + coldef->is_not_null = restdef->is_not_null;by rewriting this line as
coldef->is_not_null = coldef->is_not_null || restdef->is_not_null;
Agreed, done like that.
The comment looks a bit odd either way since we are changing coldef->is_not_null based on restdef->is_not_null. That's because of the nature of boolean variables. May be a bit of explanation is needed.
I have modified the comments around this code in the updated patch.
On a side note, I see
coldef->constraints = restdef->constraints;
Shouldn't we merge the constraints instead of just overwriting those?
Actually, I checked that coldef->constraints is always NIL in this case.
coldef (ColumnDef) is constructed from a member in the parent's TupleDesc
earlier in that function and any constraints that may be present in the
parent's definition of the column are saved in a separate variable that's
returned to the caller as one list containing "old"/inherited constraints.
So, no constraints are getting lost here.
Attached is an updated patch. I've updated some nearby comments as the
code around here seems pretty confusing, which I thought after having
returned to it after a while.
I have attached both a patch for PG10 and PG11/HEAD branches, which are
actually not all that different from each other.
Thanks,
Amit
Attachments:
PG10-v2-0001-Fix-bug-regarding-partition-column-option-inherit.patchtext/plain; charset=UTF-8; name=PG10-v2-0001-Fix-bug-regarding-partition-column-option-inherit.patchDownload
From 1158793b899bbc1f52e37b2255bd74121a293495 Mon Sep 17 00:00:00 2001
From: amit <amitlangote09@gmail.com>
Date: Wed, 6 Jun 2018 16:46:46 +0900
Subject: [PATCH v2] Fix bug regarding partition column option inheritance
It appears that the coding pattern in MergeAttributes causes the
NOT NULL constraint and default value of a column from not being
properly inherited from the parent's definition.
---
src/backend/commands/tablecmds.c | 49 +++++++++++++++++++++++++++---
src/test/regress/expected/create_table.out | 13 ++++++++
src/test/regress/sql/create_table.sql | 7 +++++
3 files changed, 64 insertions(+), 5 deletions(-)
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 81c3845095..b7a6c1792e 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -2181,6 +2181,15 @@ MergeAttributes(List *schema, List *supers, char relpersistence,
*/
if (is_partition && list_length(saved_schema) > 0)
{
+ /*
+ * Each member in 'saved_schema' contains a ColumnDef containing
+ * partition-specific options for the column. Below, we merge the
+ * information from each into the ColumnDef of the same name found in
+ * the original 'schema' list before deleting it from the list. So,
+ * once we've finished processing all the columns from the original
+ * 'schema' list, there shouldn't be any ColumnDefs left that came
+ * from the 'saved_schema' list.
+ */
schema = list_concat(schema, saved_schema);
foreach(entry, schema)
@@ -2208,14 +2217,44 @@ MergeAttributes(List *schema, List *supers, char relpersistence,
if (strcmp(coldef->colname, restdef->colname) == 0)
{
/*
- * merge the column options into the column from the
- * parent
+ * Merge the column options specified for the partition
+ * into the column definition inherited from the parent.
*/
if (coldef->is_from_parent)
{
- coldef->is_not_null = restdef->is_not_null;
- coldef->raw_default = restdef->raw_default;
- coldef->cooked_default = restdef->cooked_default;
+ /*
+ * Combine using OR so that the NOT NULL constraint
+ * in the parent's definition doesn't get lost.
+ */
+ coldef->is_not_null = (coldef->is_not_null ||
+ restdef->is_not_null);
+
+ /*
+ * If the partition's definition specifies a default,
+ * it's in restdef->raw_default, which if non-NULL,
+ * overrides the parent's default that's in
+ * coldef->cooked_default.
+ */
+ if (restdef->raw_default)
+ {
+ coldef->raw_default = restdef->raw_default;
+ coldef->cooked_default = NULL;
+ }
+ /*
+ * Don't accidentally lose the parent's default by
+ * overwriting with NULL.
+ */
+ else if (restdef->cooked_default)
+ {
+ coldef->raw_default = NULL;
+ coldef->cooked_default = restdef->cooked_default;
+ }
+
+ /*
+ * Parent's constraints, if any, have been saved in
+ * 'constraints', which are passed back to the caller,
+ * so it's okay to overwrite the variable like this.
+ */
coldef->constraints = restdef->constraints;
coldef->is_from_parent = false;
list_delete_cell(schema, rest, prev);
diff --git a/src/test/regress/expected/create_table.out b/src/test/regress/expected/create_table.out
index 0026aa11dd..4846a6fc98 100644
--- a/src/test/regress/expected/create_table.out
+++ b/src/test/regress/expected/create_table.out
@@ -662,6 +662,19 @@ ERROR: column "c" named in partition key does not exist
CREATE TABLE part_c PARTITION OF parted (b WITH OPTIONS NOT NULL DEFAULT 0) FOR VALUES IN ('c') PARTITION BY RANGE ((b));
-- create a level-2 partition
CREATE TABLE part_c_1_10 PARTITION OF part_c FOR VALUES FROM (1) TO (10);
+-- check that NOT NULL and default value are inherited correctly
+create table parted_notnull_inh_test (a int default 1, b int not null default 0) partition by list (a);
+create table parted_notnull_inh_test1 partition of parted_notnull_inh_test (b default 1) for values in (1);
+-- note that while b's default is overriden, a's default is preserved
+\d parted_notnull_inh_test1
+ Table "public.parted_notnull_inh_test1"
+ Column | Type | Collation | Nullable | Default
+--------+---------+-----------+----------+---------
+ a | integer | | | 1
+ b | integer | | not null | 1
+Partition of: parted_notnull_inh_test FOR VALUES IN (1)
+
+drop table parted_notnull_inh_test;
-- Partition bound in describe output
\d+ part_b
Table "public.part_b"
diff --git a/src/test/regress/sql/create_table.sql b/src/test/regress/sql/create_table.sql
index ad83614137..7442c4f7d3 100644
--- a/src/test/regress/sql/create_table.sql
+++ b/src/test/regress/sql/create_table.sql
@@ -609,6 +609,13 @@ CREATE TABLE part_c PARTITION OF parted (b WITH OPTIONS NOT NULL DEFAULT 0) FOR
-- create a level-2 partition
CREATE TABLE part_c_1_10 PARTITION OF part_c FOR VALUES FROM (1) TO (10);
+-- check that NOT NULL and default value are inherited correctly
+create table parted_notnull_inh_test (a int default 1, b int not null default 0) partition by list (a);
+create table parted_notnull_inh_test1 partition of parted_notnull_inh_test (b default 1) for values in (1);
+-- note that while b's default is overriden, a's default is preserved
+\d parted_notnull_inh_test1
+drop table parted_notnull_inh_test;
+
-- Partition bound in describe output
\d+ part_b
--
2.11.0
PG11-HEAD-v2-0001-Fix-bug-regarding-partition-column-option-inherit.patchtext/plain; charset=UTF-8; name=PG11-HEAD-v2-0001-Fix-bug-regarding-partition-column-option-inherit.patchDownload
From 6ab930fa7e8f6f5e548b783451f2ab7d4515909b Mon Sep 17 00:00:00 2001
From: amit <amitlangote09@gmail.com>
Date: Wed, 6 Jun 2018 16:46:46 +0900
Subject: [PATCH v2] Fix bug regarding partition column option inheritance
It appears that the coding pattern in MergeAttributes causes the
NOT NULL constraint and default value of a column from not being
properly inherited from the parent's definition.
---
src/backend/commands/tablecmds.c | 49 +++++++++++++++++++++++++++---
src/test/regress/expected/create_table.out | 13 ++++++++
src/test/regress/sql/create_table.sql | 7 +++++
3 files changed, 64 insertions(+), 5 deletions(-)
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 7c0cf0d7ee..9955e81d14 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -2394,6 +2394,15 @@ MergeAttributes(List *schema, List *supers, char relpersistence,
*/
if (is_partition && list_length(saved_schema) > 0)
{
+ /*
+ * Each member in 'saved_schema' contains a ColumnDef containing
+ * partition-specific options for the column. Below, we merge the
+ * information from each into the ColumnDef of the same name found in
+ * the original 'schema' list before deleting it from the list. So,
+ * once we've finished processing all the columns from the original
+ * 'schema' list, there shouldn't be any ColumnDefs left that came
+ * from the 'saved_schema' list.
+ */
schema = list_concat(schema, saved_schema);
foreach(entry, schema)
@@ -2421,14 +2430,44 @@ MergeAttributes(List *schema, List *supers, char relpersistence,
if (strcmp(coldef->colname, restdef->colname) == 0)
{
/*
- * merge the column options into the column from the
- * parent
+ * Merge the column options specified for the partition
+ * into the column definition inherited from the parent.
*/
if (coldef->is_from_parent)
{
- coldef->is_not_null = restdef->is_not_null;
- coldef->raw_default = restdef->raw_default;
- coldef->cooked_default = restdef->cooked_default;
+ /*
+ * Combine using OR so that the NOT NULL constraint
+ * in the parent's definition doesn't get lost.
+ */
+ coldef->is_not_null = (coldef->is_not_null ||
+ restdef->is_not_null);
+
+ /*
+ * If the partition's definition specifies a default,
+ * it's in restdef->raw_default, which if non-NULL,
+ * overrides the parent's default that's in
+ * coldef->cooked_default.
+ */
+ if (restdef->raw_default)
+ {
+ coldef->raw_default = restdef->raw_default;
+ coldef->cooked_default = NULL;
+ }
+ /*
+ * Don't accidentally lose the parent's default by
+ * overwriting with NULL.
+ */
+ else if (restdef->cooked_default)
+ {
+ coldef->raw_default = NULL;
+ coldef->cooked_default = restdef->cooked_default;
+ }
+
+ /*
+ * Parent's constraints, if any, have been saved in
+ * 'constraints', which are passed back to the caller,
+ * so it's okay to overwrite the variable like this.
+ */
coldef->constraints = restdef->constraints;
coldef->is_from_parent = false;
list_delete_cell(schema, rest, prev);
diff --git a/src/test/regress/expected/create_table.out b/src/test/regress/expected/create_table.out
index 8fdbca1345..5750344e73 100644
--- a/src/test/regress/expected/create_table.out
+++ b/src/test/regress/expected/create_table.out
@@ -727,6 +727,19 @@ ERROR: column "c" named in partition key does not exist
CREATE TABLE part_c PARTITION OF parted (b WITH OPTIONS NOT NULL DEFAULT 0) FOR VALUES IN ('c') PARTITION BY RANGE ((b));
-- create a level-2 partition
CREATE TABLE part_c_1_10 PARTITION OF part_c FOR VALUES FROM (1) TO (10);
+-- check that NOT NULL and default value are inherited correctly
+create table parted_notnull_inh_test (a int default 1, b int not null default 0) partition by list (a);
+create table parted_notnull_inh_test1 partition of parted_notnull_inh_test (b default 1) for values in (1);
+-- note that while b's default is overriden, a's default is preserved
+\d parted_notnull_inh_test1
+ Table "public.parted_notnull_inh_test1"
+ Column | Type | Collation | Nullable | Default
+--------+---------+-----------+----------+---------
+ a | integer | | | 1
+ b | integer | | not null | 1
+Partition of: parted_notnull_inh_test FOR VALUES IN (1)
+
+drop table parted_notnull_inh_test;
-- Partition bound in describe output
\d+ part_b
Table "public.part_b"
diff --git a/src/test/regress/sql/create_table.sql b/src/test/regress/sql/create_table.sql
index 78944950fe..5a64ecc480 100644
--- a/src/test/regress/sql/create_table.sql
+++ b/src/test/regress/sql/create_table.sql
@@ -659,6 +659,13 @@ CREATE TABLE part_c PARTITION OF parted (b WITH OPTIONS NOT NULL DEFAULT 0) FOR
-- create a level-2 partition
CREATE TABLE part_c_1_10 PARTITION OF part_c FOR VALUES FROM (1) TO (10);
+-- check that NOT NULL and default value are inherited correctly
+create table parted_notnull_inh_test (a int default 1, b int not null default 0) partition by list (a);
+create table parted_notnull_inh_test1 partition of parted_notnull_inh_test (b default 1) for values in (1);
+-- note that while b's default is overriden, a's default is preserved
+\d parted_notnull_inh_test1
+drop table parted_notnull_inh_test;
+
-- Partition bound in describe output
\d+ part_b
--
2.11.0
On Thu, Jul 12, 2018 at 2:29 PM, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:
Thanks Ashutosh.
On 2018/07/10 22:50, Ashutosh Bapat wrote:
I didn't see any hackers thread linked to this CF entry. Hence sending this mail through CF app.
Hmm, yes. I hadn't posted the patch to -hackers.
The patch looks good to me. It applies cleanly, compiles cleanly and make check passes.
I think you could avoid condition + /* Do not override parent's NOT NULL constraint. */ + if (restdef->is_not_null) + coldef->is_not_null = restdef->is_not_null;by rewriting this line as
coldef->is_not_null = coldef->is_not_null || restdef->is_not_null;Agreed, done like that.
The comment looks a bit odd either way since we are changing coldef->is_not_null based on restdef->is_not_null. That's because of the nature of boolean variables. May be a bit of explanation is needed.
I have modified the comments around this code in the updated patch.
+ /*
+ * Each member in 'saved_schema' contains a ColumnDef containing
+ * partition-specific options for the column. Below, we merge the
+ * information from each into the ColumnDef of the same name found in
+ * the original 'schema' list before deleting it from the list. So,
+ * once we've finished processing all the columns from the original
+ * 'schema' list, there shouldn't be any ColumnDefs left that came
+ * from the 'saved_schema' list.
+ */This is conveyed by the prologue of the function.
+ /*
+ * Combine using OR so that the NOT NULL constraint
+ * in the parent's definition doesn't get lost.
+ */
This too is specified in prologue as
* Constraints (including NOT NULL constraints) for the child table
* are the union of all relevant constraints, from both the child schema
* and parent tables.So, I don't think we need any special mention of OR, "union" conveys
the intention.
On a side note, I see
coldef->constraints = restdef->constraints;
Shouldn't we merge the constraints instead of just overwriting those?Actually, I checked that coldef->constraints is always NIL in this case.
coldef (ColumnDef) is constructed from a member in the parent's TupleDesc
earlier in that function and any constraints that may be present in the
parent's definition of the column are saved in a separate variable that's
returned to the caller as one list containing "old"/inherited constraints.
So, no constraints are getting lost here.
In case of multiple inheritance coldef->constraints is "union" of
constraints from all the parents as described in the prologue. But in
case of partitioning there is only one parent and hence
coldef->constraints is NULL and hence just overwriting it works. I
think we need comments mentioning this special case.
Also, I am not sure whether we really need all conditions related to
raw_default and cooked_default. Do you have any testcase showing the
need for those changes?
--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company
Thanks Ashutosh, and sorry that I somehow missed replying to this.
On 2018/07/13 22:50, Ashutosh Bapat wrote:
On Thu, Jul 12, 2018 at 2:29 PM, Amit Langote wrote:
I have modified the comments around this code in the updated patch.
+ /* + * Each member in 'saved_schema' contains a ColumnDef containing + * partition-specific options for the column. Below, we merge the + * information from each into the ColumnDef of the same name found in + * the original 'schema' list before deleting it from the list. So, + * once we've finished processing all the columns from the original + * 'schema' list, there shouldn't be any ColumnDefs left that came + * from the 'saved_schema' list. + */This is conveyed by the prologue of the function.
+ /* + * Combine using OR so that the NOT NULL constraint + * in the parent's definition doesn't get lost. + */ This too is specified in prologue as * Constraints (including NOT NULL constraints) for the child table * are the union of all relevant constraints, from both the child schema * and parent tables.So, I don't think we need any special mention of OR, "union" conveys
the intention.
OK, I have removed both comments.
On a side note, I see
coldef->constraints = restdef->constraints;
Shouldn't we merge the constraints instead of just overwriting those?Actually, I checked that coldef->constraints is always NIL in this case.
coldef (ColumnDef) is constructed from a member in the parent's TupleDesc
earlier in that function and any constraints that may be present in the
parent's definition of the column are saved in a separate variable that's
returned to the caller as one list containing "old"/inherited constraints.
So, no constraints are getting lost here.In case of multiple inheritance coldef->constraints is "union" of
constraints from all the parents as described in the prologue.
AFAICS, the prologue doesn't mention *just* coldef->constraints. It talks
about both the constraints that are to be specified using various
ColumnDef fields (is_not_null, raw_default, cooked_default, etc.) and
constraints that are to be returned in the *supconstr output list. Both
types of constraints are obtained by union'ing corresponding values from
all parents and child's own definition.
But in
case of partitioning there is only one parent and hence
coldef->constraints is NULL and hence just overwriting it works. I
think we need comments mentioning this special case.
That's what I wrote in this comment:
/*
* Parent's constraints, if any, have been saved in
* 'constraints', which are passed back to the caller,
* so it's okay to overwrite the variable like this.
*/
Also, I am not sure whether we really need all conditions related to
raw_default and cooked_default. Do you have any testcase showing the
need for those changes?
Without the patch (example below is tested on PG 10, but same is true with
PG 11 and HEAD too):
create table parent (a int, b int default -1) partition by list (a);
create table child partition of parent (b not null) for values in (0);
\d parent
Table "public.parent"
Column │ Type │ Collation │ Nullable │ Default
────────┼─────────┼───────────┼──────────┼───────────────
a │ integer │ │ │
b │ integer │ │ │ '-1'::integer
Partition key: LIST (a)
Number of partitions: 1 (Use \d+ to list them.)
\d child
Table "public.child"
Column │ Type │ Collation │ Nullable │ Default
────────┼─────────┼───────────┼──────────┼─────────
a │ integer │ │ │
b │ integer │ │ not null │
Partition of: parent FOR VALUES IN (0)
Note that child didn't inherit -1 as default for b. That happens, because
the partition-specific ColumnDef for b, created to contain the NOT NULL
constraint, has its raw_default and cooked_default fields set to NULL.
The current code blindly assigns that ColumnDef's raw_default and
cooked_default to the inherited ColumnDef's corresponding fields.
Overriding raw_default like that is fine, because partition-specified
default get priority, but not cooked_default, which may contain the
inherited default.
That's not an issue if child's definition doesn't specify any of its own
constraints:
create table parent (a int, b int default -1) partition by list (a);
create table child partition of parent for values in (0);
\d child
Table "public.child"
Column │ Type │ Collation │ Nullable │ Default
────────┼─────────┼───────────┼──────────┼───────────────
a │ integer │ │ │
b │ integer │ │ │ '-1'::integer
Partition of: parent FOR VALUES IN (0)
That's because there is no partition-specific ColumnDef to override
anything in this case.
Anyway, I updated that code to look like this:
+ /*
+ * If the partition's definition specifies a default,
+ * it's in restdef->raw_default, which if non-NULL,
+ * overrides the parent's default that's in
+ * coldef->cooked_default.
+ */
+ if (restdef->raw_default)
+ {
+ coldef->raw_default = restdef->raw_default;
+ coldef->cooked_default = NULL;
+ }
+
+ /*
+ * coldef->cooked_default would contain the inherited
+ * default, unless overridden above. Don't try to
+ * override it with NULL.
+ */
+ Assert(restdef->cooked_default == NULL);Also, the patch already adds a test case that demonstrates what this code
does, but I modified it a bit in the updated version to also show the fix
for $subject. Now its expected output looks like this:
+-- check that NOT NULL and default value are inherited correctly
+create table parted_notnull_inh_test (a int default 1, b int not null
default 0) partition by list (a);
+create table parted_notnull_inh_test1 partition of
parted_notnull_inh_test (a not null, b default 1) for values in (1);
+-- note that while b's default is overriden, a's default is preserved
+\d parted_notnull_inh_test1
+ Table "public.parted_notnull_inh_test1"
+ Column | Type | Collation | Nullable | Default
+--------+---------+-----------+----------+---------
+ a | integer | | not null | 1
+ b | integer | | not null | 1
+Partition of: parted_notnull_inh_test FOR VALUES IN (1)
+
+drop table parted_notnull_inh_test;Just to clarify what's different in that \d output, here is the output
with unmodified PG 10:
\d parted_notnull_inh_test1
Table "public.parted_notnull_inh_test1"
Column │ Type │ Collation │ Nullable │ Default
────────┼─────────┼───────────┼──────────┼─────────
a │ integer │ │ not null │
b │ integer │ │ │ 1
Partition of: parted_notnull_inh_test FOR VALUES IN (1)
As can be seen, PG 10 loses inherited values of a's default and b's NOT NULL.
Please find attached updated patches for PG 10, PG 11 / HEAD, with changes
I mentioned above.
Thanks,
Amit
Attachments:
PG10-v3-0001-Fix-bug-regarding-partition-column-option-inherit.patchtext/plain; charset=UTF-8; name=PG10-v3-0001-Fix-bug-regarding-partition-column-option-inherit.patchDownload
From f3b3ef17335822f50c3ab209d996ff626f61eec4 Mon Sep 17 00:00:00 2001
From: amit <amitlangote09@gmail.com>
Date: Wed, 6 Jun 2018 16:46:46 +0900
Subject: [PATCH v3] Fix bug regarding partition column option inheritance
It appears that the coding pattern in MergeAttributes causes the
NOT NULL constraint and default value of a column from not being
properly inherited from the parent's definition.
---
src/backend/commands/tablecmds.c | 34 +++++++++++++++++++++++++-----
src/test/regress/expected/create_table.out | 13 ++++++++++++
src/test/regress/sql/create_table.sql | 7 ++++++
3 files changed, 49 insertions(+), 5 deletions(-)
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index f4745f3a9a..ae0ba9deb1 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -2208,14 +2208,38 @@ MergeAttributes(List *schema, List *supers, char relpersistence,
if (strcmp(coldef->colname, restdef->colname) == 0)
{
/*
- * merge the column options into the column from the
- * parent
+ * Merge the column options specified for the partition
+ * into the column definition inherited from the parent.
*/
if (coldef->is_from_parent)
{
- coldef->is_not_null = restdef->is_not_null;
- coldef->raw_default = restdef->raw_default;
- coldef->cooked_default = restdef->cooked_default;
+ coldef->is_not_null = (coldef->is_not_null ||
+ restdef->is_not_null);
+
+ /*
+ * If the partition's definition specifies a default,
+ * it's in restdef->raw_default, which if non-NULL,
+ * overrides the parent's default that's in
+ * coldef->cooked_default.
+ */
+ if (restdef->raw_default)
+ {
+ coldef->raw_default = restdef->raw_default;
+ coldef->cooked_default = NULL;
+ }
+
+ /*
+ * coldef->cooked_default would contain the inherited
+ * default, unless overridden above. Don't try to
+ * override it with NULL.
+ */
+ Assert(restdef->cooked_default == NULL);
+
+ /*
+ * Parent's constraints, if any, have been saved in
+ * 'constraints', which are passed back to the caller,
+ * so it's okay to overwrite the variable like this.
+ */
coldef->constraints = restdef->constraints;
coldef->is_from_parent = false;
list_delete_cell(schema, rest, prev);
diff --git a/src/test/regress/expected/create_table.out b/src/test/regress/expected/create_table.out
index 660398bdbe..9e771e58a2 100644
--- a/src/test/regress/expected/create_table.out
+++ b/src/test/regress/expected/create_table.out
@@ -657,6 +657,19 @@ ERROR: column "c" named in partition key does not exist
CREATE TABLE part_c PARTITION OF parted (b WITH OPTIONS NOT NULL DEFAULT 0) FOR VALUES IN ('c') PARTITION BY RANGE ((b));
-- create a level-2 partition
CREATE TABLE part_c_1_10 PARTITION OF part_c FOR VALUES FROM (1) TO (10);
+-- check that NOT NULL and default value are inherited correctly
+create table parted_notnull_inh_test (a int default 1, b int not null default 0) partition by list (a);
+create table parted_notnull_inh_test1 partition of parted_notnull_inh_test (a not null, b default 1) for values in (1);
+-- note that while b's default is overriden, a's default is preserved
+\d parted_notnull_inh_test1
+ Table "public.parted_notnull_inh_test1"
+ Column | Type | Collation | Nullable | Default
+--------+---------+-----------+----------+---------
+ a | integer | | not null | 1
+ b | integer | | not null | 1
+Partition of: parted_notnull_inh_test FOR VALUES IN (1)
+
+drop table parted_notnull_inh_test;
-- Partition bound in describe output
\d+ part_b
Table "public.part_b"
diff --git a/src/test/regress/sql/create_table.sql b/src/test/regress/sql/create_table.sql
index 16f2edb897..bde03f194f 100644
--- a/src/test/regress/sql/create_table.sql
+++ b/src/test/regress/sql/create_table.sql
@@ -604,6 +604,13 @@ CREATE TABLE part_c PARTITION OF parted (b WITH OPTIONS NOT NULL DEFAULT 0) FOR
-- create a level-2 partition
CREATE TABLE part_c_1_10 PARTITION OF part_c FOR VALUES FROM (1) TO (10);
+-- check that NOT NULL and default value are inherited correctly
+create table parted_notnull_inh_test (a int default 1, b int not null default 0) partition by list (a);
+create table parted_notnull_inh_test1 partition of parted_notnull_inh_test (a not null, b default 1) for values in (1);
+-- note that while b's default is overriden, a's default is preserved
+\d parted_notnull_inh_test1
+drop table parted_notnull_inh_test;
+
-- Partition bound in describe output
\d+ part_b
--
2.11.0
PG11-HEAD-v3-0001-Fix-bug-regarding-partition-column-option-inherit.patchtext/plain; charset=UTF-8; name=PG11-HEAD-v3-0001-Fix-bug-regarding-partition-column-option-inherit.patchDownload
From 220f8519f71234af3e2d47998d0d651945a47f32 Mon Sep 17 00:00:00 2001
From: amit <amitlangote09@gmail.com>
Date: Wed, 6 Jun 2018 16:46:46 +0900
Subject: [PATCH v3] Fix bug regarding partition column option inheritance
It appears that the coding pattern in MergeAttributes causes the
NOT NULL constraint and default value of a column from not being
properly inherited from the parent's definition.
---
src/backend/commands/tablecmds.c | 34 +++++++++++++++++++++++++-----
src/test/regress/expected/create_table.out | 13 ++++++++++++
src/test/regress/sql/create_table.sql | 7 ++++++
3 files changed, 49 insertions(+), 5 deletions(-)
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index eb2d33dd86..8916a6b621 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -2421,14 +2421,38 @@ MergeAttributes(List *schema, List *supers, char relpersistence,
if (strcmp(coldef->colname, restdef->colname) == 0)
{
/*
- * merge the column options into the column from the
- * parent
+ * Merge the column options specified for the partition
+ * into the column definition inherited from the parent.
*/
if (coldef->is_from_parent)
{
- coldef->is_not_null = restdef->is_not_null;
- coldef->raw_default = restdef->raw_default;
- coldef->cooked_default = restdef->cooked_default;
+ coldef->is_not_null = (coldef->is_not_null ||
+ restdef->is_not_null);
+
+ /*
+ * If the partition's definition specifies a default,
+ * it's in restdef->raw_default, which if non-NULL,
+ * overrides the parent's default that's in
+ * coldef->cooked_default.
+ */
+ if (restdef->raw_default)
+ {
+ coldef->raw_default = restdef->raw_default;
+ coldef->cooked_default = NULL;
+ }
+
+ /*
+ * coldef->cooked_default would contain the inherited
+ * default, unless overridden above. Don't try to
+ * override it with NULL.
+ */
+ Assert(restdef->cooked_default == NULL);
+
+ /*
+ * Parent's constraints, if any, have been saved in
+ * 'constraints', which are passed back to the caller,
+ * so it's okay to overwrite the variable like this.
+ */
coldef->constraints = restdef->constraints;
coldef->is_from_parent = false;
list_delete_cell(schema, rest, prev);
diff --git a/src/test/regress/expected/create_table.out b/src/test/regress/expected/create_table.out
index 8927b21ba2..d89986e908 100644
--- a/src/test/regress/expected/create_table.out
+++ b/src/test/regress/expected/create_table.out
@@ -722,6 +722,19 @@ ERROR: column "c" named in partition key does not exist
CREATE TABLE part_c PARTITION OF parted (b WITH OPTIONS NOT NULL DEFAULT 0) FOR VALUES IN ('c') PARTITION BY RANGE ((b));
-- create a level-2 partition
CREATE TABLE part_c_1_10 PARTITION OF part_c FOR VALUES FROM (1) TO (10);
+-- check that NOT NULL and default value are inherited correctly
+create table parted_notnull_inh_test (a int default 1, b int not null default 0) partition by list (a);
+create table parted_notnull_inh_test1 partition of parted_notnull_inh_test (a not null, b default 1) for values in (1);
+-- note that while b's default is overriden, a's default is preserved
+\d parted_notnull_inh_test1
+ Table "public.parted_notnull_inh_test1"
+ Column | Type | Collation | Nullable | Default
+--------+---------+-----------+----------+---------
+ a | integer | | not null | 1
+ b | integer | | not null | 1
+Partition of: parted_notnull_inh_test FOR VALUES IN (1)
+
+drop table parted_notnull_inh_test;
-- Partition bound in describe output
\d+ part_b
Table "public.part_b"
diff --git a/src/test/regress/sql/create_table.sql b/src/test/regress/sql/create_table.sql
index 81fa7658b0..1a64fbf003 100644
--- a/src/test/regress/sql/create_table.sql
+++ b/src/test/regress/sql/create_table.sql
@@ -654,6 +654,13 @@ CREATE TABLE part_c PARTITION OF parted (b WITH OPTIONS NOT NULL DEFAULT 0) FOR
-- create a level-2 partition
CREATE TABLE part_c_1_10 PARTITION OF part_c FOR VALUES FROM (1) TO (10);
+-- check that NOT NULL and default value are inherited correctly
+create table parted_notnull_inh_test (a int default 1, b int not null default 0) partition by list (a);
+create table parted_notnull_inh_test1 partition of parted_notnull_inh_test (a not null, b default 1) for values in (1);
+-- note that while b's default is overriden, a's default is preserved
+\d parted_notnull_inh_test1
+drop table parted_notnull_inh_test;
+
-- Partition bound in describe output
\d+ part_b
--
2.11.0
On 2018-Aug-07, Amit Langote wrote:
But in
case of partitioning there is only one parent and hence
coldef->constraints is NULL and hence just overwriting it works. I
think we need comments mentioning this special case.That's what I wrote in this comment:
/*
* Parent's constraints, if any, have been saved in
* 'constraints', which are passed back to the caller,
* so it's okay to overwrite the variable like this.
*/
What is this for? I tried commenting out that line to see what
test breaks, and found none.
I tried to figure it out, so while thinking what exactly is "restdef" in
that block, it struck me that we seem to be doing things in quite a
strange way there by concatenating both schema lists. I changed it so
that that block scans only the "saved_schema" list (ie. the
partition-local column list definition), searching the other list for
each matching item. This seems a much more natural implementation -- at
least, it results in less list mangling and shorter code, so.
One thing that broke was that duplicate columns in the partition-local
definition would not be caught. However, we already have that check a
few hundred lines above in the same function ... which was skipped for
partitions because of list-mangling that was done before that. I moved
the NIL-setting of schema one block below, and then all tests pass.
One thing that results is that is_from_parent becomes totally useless
and can be removed. It could in theory be removed from ColumnDef, if it
wasn't for the ABI incompatibility that would cause.
BTW this line:
coldef->is_not_null == (coldef->is_not_null || restdef->is_not_null)
can be written more easily like:
coldef->is_not_null |= restdef->is_not_null;
--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Attachments:
PG10-v4-0001-Fix-bug-regarding-partition-column-option-inherit.patchtext/x-diff; charset=us-asciiDownload
diff --git a/src/backend/commands/sequence.c b/src/backend/commands/sequence.c
index 2f3ee46236..74cb2e61bc 100644
--- a/src/backend/commands/sequence.c
+++ b/src/backend/commands/sequence.c
@@ -172,7 +172,6 @@ DefineSequence(ParseState *pstate, CreateSeqStmt *seq)
coldef->is_local = true;
coldef->is_not_null = true;
coldef->is_from_type = false;
- coldef->is_from_parent = false;
coldef->storage = 0;
coldef->raw_default = NULL;
coldef->cooked_default = NULL;
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 971a8721e1..6bd356d5a0 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -1650,17 +1650,6 @@ MergeAttributes(List *schema, List *supers, char relpersistence,
MaxHeapAttributeNumber)));
/*
- * In case of a partition, there are no new column definitions, only dummy
- * ColumnDefs created for column constraints. We merge them with the
- * constraints inherited from the parent.
- */
- if (is_partition)
- {
- saved_schema = schema;
- schema = NIL;
- }
-
- /*
* Check for duplicate names in the explicit list of attributes.
*
* Although we might consider merging such entries in the same way that we
@@ -1673,8 +1662,8 @@ MergeAttributes(List *schema, List *supers, char relpersistence,
ListCell *rest = lnext(entry);
ListCell *prev = entry;
- if (coldef->typeName == NULL)
-
+ if (!is_partition && coldef->typeName == NULL)
+ {
/*
* Typed table column option that does not belong to a column from
* the type. This works because the columns from the type come
@@ -1684,6 +1673,7 @@ MergeAttributes(List *schema, List *supers, char relpersistence,
(errcode(ERRCODE_UNDEFINED_COLUMN),
errmsg("column \"%s\" does not exist",
coldef->colname)));
+ }
while (rest != NULL)
{
@@ -1717,6 +1707,17 @@ MergeAttributes(List *schema, List *supers, char relpersistence,
}
/*
+ * In case of a partition, there are no new column definitions, only dummy
+ * ColumnDefs created for column constraints. We merge them with the
+ * constraints inherited from the parent.
+ */
+ if (is_partition)
+ {
+ saved_schema = schema;
+ schema = NIL;
+ }
+
+ /*
* Scan the parents left-to-right, and merge their attributes to form a
* list of inherited attributes (inhSchema). Also check to see if we need
* to inherit an OID column.
@@ -1931,7 +1932,6 @@ MergeAttributes(List *schema, List *supers, char relpersistence,
def->is_local = false;
def->is_not_null = attribute->attnotnull;
def->is_from_type = false;
- def->is_from_parent = true;
def->storage = attribute->attstorage;
def->raw_default = NULL;
def->cooked_default = NULL;
@@ -2187,56 +2187,50 @@ MergeAttributes(List *schema, List *supers, char relpersistence,
* actually exist. Also, we merge the constraints into the corresponding
* column definitions.
*/
- if (is_partition && list_length(saved_schema) > 0)
+ if (is_partition)
{
- schema = list_concat(schema, saved_schema);
+ ListCell *l1,
+ *l2;
- foreach(entry, schema)
+ foreach (l1, saved_schema)
{
- ColumnDef *coldef = lfirst(entry);
- ListCell *rest = lnext(entry);
- ListCell *prev = entry;
+ ColumnDef *restdef = lfirst(l1);
+ bool found = false;
- /*
- * Partition column option that does not belong to a column from
- * the parent. This works because the columns from the parent
- * come first in the list (see above).
- */
- if (coldef->typeName == NULL)
- ereport(ERROR,
- (errcode(ERRCODE_UNDEFINED_COLUMN),
- errmsg("column \"%s\" does not exist",
- coldef->colname)));
- while (rest != NULL)
+ foreach(l2, schema)
{
- ColumnDef *restdef = lfirst(rest);
- ListCell *next = lnext(rest); /* need to save it in case we
- * delete it */
+ ColumnDef *coldef = lfirst(l2);
if (strcmp(coldef->colname, restdef->colname) == 0)
{
+ found = true;
+ coldef->is_not_null |= restdef->is_not_null;
+
/*
- * merge the column options into the column from the
- * parent
+ * Override the parent's default value for this column
+ * (coldef->cooked_default) with the partition's local
+ * definition (restdef->raw_default), if there's one.
+ * It should be physically impossible to get a cooked
+ * default in the local definition or a raw default in
+ * the inherited definition, but make sure they're nulls,
+ * for future-proofing.
*/
- if (coldef->is_from_parent)
+ Assert(restdef->cooked_default == NULL);
+ Assert(coldef->raw_default == NULL);
+ if (restdef->raw_default)
{
- coldef->is_not_null = restdef->is_not_null;
coldef->raw_default = restdef->raw_default;
- coldef->cooked_default = restdef->cooked_default;
- coldef->constraints = restdef->constraints;
- coldef->is_from_parent = false;
- list_delete_cell(schema, rest, prev);
+ coldef->cooked_default = NULL;
}
- else
- ereport(ERROR,
- (errcode(ERRCODE_DUPLICATE_COLUMN),
- errmsg("column \"%s\" specified more than once",
- coldef->colname)));
}
- prev = rest;
- rest = next;
}
+
+ /* complain for constraints on columns not in parent */
+ if (!found)
+ ereport(ERROR,
+ (errcode(ERRCODE_UNDEFINED_COLUMN),
+ errmsg("column \"%s\" does not exist",
+ restdef->colname)));
}
}
diff --git a/src/backend/nodes/equalfuncs.c b/src/backend/nodes/equalfuncs.c
index 68d38fcba1..c5bb817ba1 100644
--- a/src/backend/nodes/equalfuncs.c
+++ b/src/backend/nodes/equalfuncs.c
@@ -2539,7 +2539,6 @@ _equalColumnDef(const ColumnDef *a, const ColumnDef *b)
COMPARE_SCALAR_FIELD(is_local);
COMPARE_SCALAR_FIELD(is_not_null);
COMPARE_SCALAR_FIELD(is_from_type);
- COMPARE_SCALAR_FIELD(is_from_parent);
COMPARE_SCALAR_FIELD(storage);
COMPARE_NODE_FIELD(raw_default);
COMPARE_NODE_FIELD(cooked_default);
diff --git a/src/backend/nodes/makefuncs.c b/src/backend/nodes/makefuncs.c
index 0755039da9..87ffe82530 100644
--- a/src/backend/nodes/makefuncs.c
+++ b/src/backend/nodes/makefuncs.c
@@ -494,7 +494,6 @@ makeColumnDef(const char *colname, Oid typeOid, int32 typmod, Oid collOid)
n->is_local = true;
n->is_not_null = false;
n->is_from_type = false;
- n->is_from_parent = false;
n->storage = 0;
n->raw_default = NULL;
n->cooked_default = NULL;
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index 025e82b30f..a97f51ce37 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -3240,7 +3240,6 @@ columnDef: ColId Typename create_generic_options ColQualList
n->is_local = true;
n->is_not_null = false;
n->is_from_type = false;
- n->is_from_parent = false;
n->storage = 0;
n->raw_default = NULL;
n->cooked_default = NULL;
@@ -3262,7 +3261,6 @@ columnOptions: ColId ColQualList
n->is_local = true;
n->is_not_null = false;
n->is_from_type = false;
- n->is_from_parent = false;
n->storage = 0;
n->raw_default = NULL;
n->cooked_default = NULL;
@@ -3281,7 +3279,6 @@ columnOptions: ColId ColQualList
n->is_local = true;
n->is_not_null = false;
n->is_from_type = false;
- n->is_from_parent = false;
n->storage = 0;
n->raw_default = NULL;
n->cooked_default = NULL;
@@ -11884,7 +11881,6 @@ TableFuncElement: ColId Typename opt_collate_clause
n->is_local = true;
n->is_not_null = false;
n->is_from_type = false;
- n->is_from_parent = false;
n->storage = 0;
n->raw_default = NULL;
n->cooked_default = NULL;
diff --git a/src/backend/parser/parse_utilcmd.c b/src/backend/parser/parse_utilcmd.c
index b3367f0cd4..0a03658e66 100644
--- a/src/backend/parser/parse_utilcmd.c
+++ b/src/backend/parser/parse_utilcmd.c
@@ -1025,7 +1025,6 @@ transformTableLikeClause(CreateStmtContext *cxt, TableLikeClause *table_like_cla
def->is_local = true;
def->is_not_null = attribute->attnotnull;
def->is_from_type = false;
- def->is_from_parent = false;
def->storage = 0;
def->raw_default = NULL;
def->cooked_default = NULL;
@@ -1293,7 +1292,6 @@ transformOfType(CreateStmtContext *cxt, TypeName *ofTypename)
n->is_local = true;
n->is_not_null = false;
n->is_from_type = true;
- n->is_from_parent = false;
n->storage = 0;
n->raw_default = NULL;
n->cooked_default = NULL;
diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h
index 1f80dfaa85..57fa4db0d3 100644
--- a/src/include/nodes/parsenodes.h
+++ b/src/include/nodes/parsenodes.h
@@ -642,7 +642,7 @@ typedef struct ColumnDef
bool is_local; /* column has local (non-inherited) def'n */
bool is_not_null; /* NOT NULL constraint specified? */
bool is_from_type; /* column definition came from table type */
- bool is_from_parent; /* column def came from partition parent */
+ bool is_from_parent; /* XXX unused */
char storage; /* attstorage setting, or 0 for default */
Node *raw_default; /* default value (untransformed parse tree) */
Node *cooked_default; /* default value (transformed expr tree) */
diff --git a/src/test/regress/expected/create_table.out b/src/test/regress/expected/create_table.out
index 481510c2bb..70c7a73689 100644
--- a/src/test/regress/expected/create_table.out
+++ b/src/test/regress/expected/create_table.out
@@ -657,6 +657,22 @@ ERROR: column "c" named in partition key does not exist
CREATE TABLE part_c PARTITION OF parted (b WITH OPTIONS NOT NULL DEFAULT 0) FOR VALUES IN ('c') PARTITION BY RANGE ((b));
-- create a level-2 partition
CREATE TABLE part_c_1_10 PARTITION OF part_c FOR VALUES FROM (1) TO (10);
+-- check that NOT NULL and default value are inherited correctly
+create table parted_notnull_inh_test (a int default 1, b int not null default 0) partition by list (a);
+create table parted_notnull_inh_test1 partition of parted_notnull_inh_test (a not null, b default 1) for values in (1);
+insert into parted_notnull_inh_test (b) values (null);
+ERROR: null value in column "b" violates not-null constraint
+DETAIL: Failing row contains (1, null).
+-- note that while b's default is overriden, a's default is preserved
+\d parted_notnull_inh_test1
+ Table "public.parted_notnull_inh_test1"
+ Column | Type | Collation | Nullable | Default
+--------+---------+-----------+----------+---------
+ a | integer | | not null | 1
+ b | integer | | not null | 1
+Partition of: parted_notnull_inh_test FOR VALUES IN (1)
+
+drop table parted_notnull_inh_test;
-- Partition bound in describe output
\d+ part_b
Table "public.part_b"
diff --git a/src/test/regress/sql/create_table.sql b/src/test/regress/sql/create_table.sql
index bc6191f4ac..d8776a81ec 100644
--- a/src/test/regress/sql/create_table.sql
+++ b/src/test/regress/sql/create_table.sql
@@ -604,6 +604,14 @@ CREATE TABLE part_c PARTITION OF parted (b WITH OPTIONS NOT NULL DEFAULT 0) FOR
-- create a level-2 partition
CREATE TABLE part_c_1_10 PARTITION OF part_c FOR VALUES FROM (1) TO (10);
+-- check that NOT NULL and default value are inherited correctly
+create table parted_notnull_inh_test (a int default 1, b int not null default 0) partition by list (a);
+create table parted_notnull_inh_test1 partition of parted_notnull_inh_test (a not null, b default 1) for values in (1);
+insert into parted_notnull_inh_test (b) values (null);
+-- note that while b's default is overriden, a's default is preserved
+\d parted_notnull_inh_test1
+drop table parted_notnull_inh_test;
+
-- Partition bound in describe output
\d+ part_b
Hi,
Thank you for looking at this.
On 2018/11/06 7:25, Alvaro Herrera wrote:
On 2018-Aug-07, Amit Langote wrote:
But in
case of partitioning there is only one parent and hence
coldef->constraints is NULL and hence just overwriting it works. I
think we need comments mentioning this special case.That's what I wrote in this comment:
/*
* Parent's constraints, if any, have been saved in
* 'constraints', which are passed back to the caller,
* so it's okay to overwrite the variable like this.
*/What is this for? I tried commenting out that line to see what
test breaks, and found none.
The quoted comment is an explanation I wrote to describe why I think the
*existing* statement (shown below) is correct.
coldef->constraints = restdef->constraints;
As you've already figured out, 'coldef' here refers (referred) to the
inherited column definition and 'restdef' to the partition's local
definition. Ashutosh asked in his review why doing this is OK, because it
appeared that it's essentially leaking/overwriting inherited constraints.
The comment I added was to try to assure future readers that the inherited
constraints have already been linked into into another output variable and
so no constraints are being leaked.
As for why ignoring partition's local constraints (restdef->constraints)
didn't break anything, I see that's because transformCreateStmt would
already have added them to CreateStmt.constraints, so they're already
taken care of.
I tried to figure it out, so while thinking what exactly is "restdef" in
that block, it struck me that we seem to be doing things in quite a
strange way there by concatenating both schema lists. I changed it so
that that block scans only the "saved_schema" list (ie. the
partition-local column list definition), searching the other list for
each matching item. This seems a much more natural implementation -- at
least, it results in less list mangling and shorter code, so.One thing that broke was that duplicate columns in the partition-local
definition would not be caught. However, we already have that check a
few hundred lines above in the same function ... which was skipped for
partitions because of list-mangling that was done before that. I moved
the NIL-setting of schema one block below, and then all tests pass.
I had hit some error when I made the partition case reuse the code that
handles the OF type case, the details of which unfortunately I don't
remember. Looking at your take, I can't now think of some case that's
being broken with it. It's definitely nice that the same strange piece of
code is not repeated twice now.
One thing that results is that is_from_parent becomes totally useless
and can be removed. It could in theory be removed from ColumnDef, if it
wasn't for the ABI incompatibility that would cause.
:(. That thing is never meaningful/true outside MergeAttributes().
BTW this line:
coldef->is_not_null == (coldef->is_not_null || restdef->is_not_null)
can be written more easily like:
coldef->is_not_null |= restdef->is_not_null;
Yeah, although there seems to be a typo above: s/==/=/g
Thanks,
Amit
Looking over the stuff in gram.y (to make sure there's nothing that
could be lost), I noticed that we're losing the COLLATE clause when they
are added to columns in partitions. I would expect part1 to end up with
collation es_CL, or alternatively that the command throws an error:
55462 10.6 138851=# create table part (a text collate "en_US") partition by range (a);
CREATE TABLE
Duración: 23,511 ms
55462 10.6 138851=# create table part1 partition of part (a collate "es_CL") for values from ('ca') to ('cu');
CREATE TABLE
Duración: 111,551 ms
55462 10.6 138851=# \d part
Tabla «public.part»
Columna │ Tipo │ Collation │ Nullable │ Default
─────────┼──────┼───────────┼──────────┼─────────
a │ text │ en_US │ │
Partition key: RANGE (a)
Number of partitions: 1 (Use \d+ to list them.)
55462 10.6 138851=# \d part1
Tabla «public.part1»
Columna │ Tipo │ Collation │ Nullable │ Default
─────────┼──────┼───────────┼──────────┼─────────
a │ text │ en_US │ │
Partition of: part FOR VALUES FROM ('ca') TO ('cu')
(This case is particularly bothersome because the column is the
partition key, so the partition range bounds would differ depending on
which collation is used to compare. I assume we'd always want to use
the parent table's collation; so there's even a stronger case for
raising an error if it doesn't match the parent's. However, this case
could arise for other columns too, where it's not *so* bad, but still
not completely correct I think.)
This happens on unpatched code, and doesn't seem covered by any tests.
However, this seems an independent bug that isn't affected by this
patch.
The only other things there are deferrability markers, which seem to be
propagated in a relatively sane fashion (but no luck if you want to add
foreign keys with mismatching deferrability than the parent's -- you
just get a dupe, and there's no way in the grammar to change the flags
for the existing constraint). But you can add a UNIQUE DEFERRED
constraint in a partition that wasn't there in the parent, for example.
--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Here's the patch I intend to push to branches 10 and 11. The patch in
master is almost identical -- the only difference is that
ColumnDef->is_from_parent is removed.
--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Attachments:
0001-Revise-attribute-handling-code-on-partition-creation.patchtext/x-diff; charset=iso-8859-1Download
From 1a74c06b4ca66d10669a62c7877dd5d6525c5580 Mon Sep 17 00:00:00 2001
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
Date: Mon, 5 Nov 2018 12:37:37 -0300
Subject: [PATCH] Revise attribute handling code on partition creation
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
The original code to propagate NOT NULL and default expressions
specified when creating a partition was mostly copied from typed-tables
creation, but not being a great match it contained some duplicity,
inefficiency and bugs. This rewrite makes it simpler:
1. instead of checking for duplicate column names in its own block,
reuse the original one that already did that;
2. instead of concatenating the list of columns from parent and the one
declared in the partition and scanning the result to (incorrectly)
propagate defaults and not-null constraints, just scan the latter
searching the former for a match, and merging sensibly. This works
because we know the list in the parent is already correct and there can
only be one parent.
This fixes the bug that NOT NULL constraints declared in the parent
table would not be honored in the partition. There are other issues
nearby, such as DEFAULT values declared in the partition that are not
honored if inserting through the parent; but that's a larger problem
that would not become backpatched. Also, collation declarations in the
partition are silently lost, but fixing this seems a potential landmine.
This rewrite makes ColumnDef->is_from_parent unused, so it's removed
on branch master; on released branches, it's kept as an unused field in
order not to cause ABI incompatibilities.
Amit Langote wrote a less invasive fix for this, but while I kept the
test he added, I ended up not using his code. Ashutosh Bapat reviewed
Amit's fix.
Author: Ãlvaro Herrera, Amit Langote
Reviewed-by: Ashutosh Bapat, Amit Langote
Reported-by: Jürgen Strobel (bug #15212)
Discussion: https://postgr.es/m/152746742177.1291.9847032632907407358@wrigleys.postgresql.org
---
src/backend/commands/sequence.c | 1 -
src/backend/commands/tablecmds.c | 99 ++++++++++++++----------------
src/backend/nodes/equalfuncs.c | 1 -
src/backend/nodes/makefuncs.c | 1 -
src/backend/parser/gram.y | 4 --
src/backend/parser/parse_utilcmd.c | 2 -
src/include/nodes/parsenodes.h | 2 +-
src/test/regress/expected/create_table.out | 16 +++++
src/test/regress/sql/create_table.sql | 8 +++
9 files changed, 71 insertions(+), 63 deletions(-)
diff --git a/src/backend/commands/sequence.c b/src/backend/commands/sequence.c
index 2f3ee46236..74cb2e61bc 100644
--- a/src/backend/commands/sequence.c
+++ b/src/backend/commands/sequence.c
@@ -172,7 +172,6 @@ DefineSequence(ParseState *pstate, CreateSeqStmt *seq)
coldef->is_local = true;
coldef->is_not_null = true;
coldef->is_from_type = false;
- coldef->is_from_parent = false;
coldef->storage = 0;
coldef->raw_default = NULL;
coldef->cooked_default = NULL;
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 971a8721e1..b4a19771a2 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -1650,17 +1650,6 @@ MergeAttributes(List *schema, List *supers, char relpersistence,
MaxHeapAttributeNumber)));
/*
- * In case of a partition, there are no new column definitions, only dummy
- * ColumnDefs created for column constraints. We merge them with the
- * constraints inherited from the parent.
- */
- if (is_partition)
- {
- saved_schema = schema;
- schema = NIL;
- }
-
- /*
* Check for duplicate names in the explicit list of attributes.
*
* Although we might consider merging such entries in the same way that we
@@ -1673,17 +1662,19 @@ MergeAttributes(List *schema, List *supers, char relpersistence,
ListCell *rest = lnext(entry);
ListCell *prev = entry;
- if (coldef->typeName == NULL)
-
+ if (!is_partition && coldef->typeName == NULL)
+ {
/*
* Typed table column option that does not belong to a column from
* the type. This works because the columns from the type come
- * first in the list.
+ * first in the list. (We omit this check for partition column
+ * lists; those are processed separately below.)
*/
ereport(ERROR,
(errcode(ERRCODE_UNDEFINED_COLUMN),
errmsg("column \"%s\" does not exist",
coldef->colname)));
+ }
while (rest != NULL)
{
@@ -1717,6 +1708,17 @@ MergeAttributes(List *schema, List *supers, char relpersistence,
}
/*
+ * In case of a partition, there are no new column definitions, only dummy
+ * ColumnDefs created for column constraints. Set them aside for now and
+ * process them at the end.
+ */
+ if (is_partition)
+ {
+ saved_schema = schema;
+ schema = NIL;
+ }
+
+ /*
* Scan the parents left-to-right, and merge their attributes to form a
* list of inherited attributes (inhSchema). Also check to see if we need
* to inherit an OID column.
@@ -1931,7 +1933,6 @@ MergeAttributes(List *schema, List *supers, char relpersistence,
def->is_local = false;
def->is_not_null = attribute->attnotnull;
def->is_from_type = false;
- def->is_from_parent = true;
def->storage = attribute->attstorage;
def->raw_default = NULL;
def->cooked_default = NULL;
@@ -2184,59 +2185,51 @@ MergeAttributes(List *schema, List *supers, char relpersistence,
/*
* Now that we have the column definition list for a partition, we can
* check whether the columns referenced in the column constraint specs
- * actually exist. Also, we merge the constraints into the corresponding
- * column definitions.
+ * actually exist. Also, we merge NOT NULL and defaults into each
+ * corresponding column definition.
*/
- if (is_partition && list_length(saved_schema) > 0)
+ if (is_partition)
{
- schema = list_concat(schema, saved_schema);
-
- foreach(entry, schema)
+ foreach(entry, saved_schema)
{
- ColumnDef *coldef = lfirst(entry);
- ListCell *rest = lnext(entry);
- ListCell *prev = entry;
+ ColumnDef *restdef = lfirst(entry);
+ bool found = false;
+ ListCell *l;
- /*
- * Partition column option that does not belong to a column from
- * the parent. This works because the columns from the parent
- * come first in the list (see above).
- */
- if (coldef->typeName == NULL)
- ereport(ERROR,
- (errcode(ERRCODE_UNDEFINED_COLUMN),
- errmsg("column \"%s\" does not exist",
- coldef->colname)));
- while (rest != NULL)
+ foreach(l, schema)
{
- ColumnDef *restdef = lfirst(rest);
- ListCell *next = lnext(rest); /* need to save it in case we
- * delete it */
+ ColumnDef *coldef = lfirst(l);
if (strcmp(coldef->colname, restdef->colname) == 0)
{
+ found = true;
+ coldef->is_not_null |= restdef->is_not_null;
+
/*
- * merge the column options into the column from the
- * parent
+ * Override the parent's default value for this column
+ * (coldef->cooked_default) with the partition's local
+ * definition (restdef->raw_default), if there's one. It
+ * should be physically impossible to get a cooked default
+ * in the local definition or a raw default in the
+ * inherited definition, but make sure they're nulls, for
+ * future-proofing.
*/
- if (coldef->is_from_parent)
+ Assert(restdef->cooked_default == NULL);
+ Assert(coldef->raw_default == NULL);
+ if (restdef->raw_default)
{
- coldef->is_not_null = restdef->is_not_null;
coldef->raw_default = restdef->raw_default;
- coldef->cooked_default = restdef->cooked_default;
- coldef->constraints = restdef->constraints;
- coldef->is_from_parent = false;
- list_delete_cell(schema, rest, prev);
+ coldef->cooked_default = NULL;
}
- else
- ereport(ERROR,
- (errcode(ERRCODE_DUPLICATE_COLUMN),
- errmsg("column \"%s\" specified more than once",
- coldef->colname)));
}
- prev = rest;
- rest = next;
}
+
+ /* complain for constraints on columns not in parent */
+ if (!found)
+ ereport(ERROR,
+ (errcode(ERRCODE_UNDEFINED_COLUMN),
+ errmsg("column \"%s\" does not exist",
+ restdef->colname)));
}
}
diff --git a/src/backend/nodes/equalfuncs.c b/src/backend/nodes/equalfuncs.c
index 68d38fcba1..c5bb817ba1 100644
--- a/src/backend/nodes/equalfuncs.c
+++ b/src/backend/nodes/equalfuncs.c
@@ -2539,7 +2539,6 @@ _equalColumnDef(const ColumnDef *a, const ColumnDef *b)
COMPARE_SCALAR_FIELD(is_local);
COMPARE_SCALAR_FIELD(is_not_null);
COMPARE_SCALAR_FIELD(is_from_type);
- COMPARE_SCALAR_FIELD(is_from_parent);
COMPARE_SCALAR_FIELD(storage);
COMPARE_NODE_FIELD(raw_default);
COMPARE_NODE_FIELD(cooked_default);
diff --git a/src/backend/nodes/makefuncs.c b/src/backend/nodes/makefuncs.c
index 0755039da9..87ffe82530 100644
--- a/src/backend/nodes/makefuncs.c
+++ b/src/backend/nodes/makefuncs.c
@@ -494,7 +494,6 @@ makeColumnDef(const char *colname, Oid typeOid, int32 typmod, Oid collOid)
n->is_local = true;
n->is_not_null = false;
n->is_from_type = false;
- n->is_from_parent = false;
n->storage = 0;
n->raw_default = NULL;
n->cooked_default = NULL;
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index 025e82b30f..a97f51ce37 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -3240,7 +3240,6 @@ columnDef: ColId Typename create_generic_options ColQualList
n->is_local = true;
n->is_not_null = false;
n->is_from_type = false;
- n->is_from_parent = false;
n->storage = 0;
n->raw_default = NULL;
n->cooked_default = NULL;
@@ -3262,7 +3261,6 @@ columnOptions: ColId ColQualList
n->is_local = true;
n->is_not_null = false;
n->is_from_type = false;
- n->is_from_parent = false;
n->storage = 0;
n->raw_default = NULL;
n->cooked_default = NULL;
@@ -3281,7 +3279,6 @@ columnOptions: ColId ColQualList
n->is_local = true;
n->is_not_null = false;
n->is_from_type = false;
- n->is_from_parent = false;
n->storage = 0;
n->raw_default = NULL;
n->cooked_default = NULL;
@@ -11884,7 +11881,6 @@ TableFuncElement: ColId Typename opt_collate_clause
n->is_local = true;
n->is_not_null = false;
n->is_from_type = false;
- n->is_from_parent = false;
n->storage = 0;
n->raw_default = NULL;
n->cooked_default = NULL;
diff --git a/src/backend/parser/parse_utilcmd.c b/src/backend/parser/parse_utilcmd.c
index b3367f0cd4..0a03658e66 100644
--- a/src/backend/parser/parse_utilcmd.c
+++ b/src/backend/parser/parse_utilcmd.c
@@ -1025,7 +1025,6 @@ transformTableLikeClause(CreateStmtContext *cxt, TableLikeClause *table_like_cla
def->is_local = true;
def->is_not_null = attribute->attnotnull;
def->is_from_type = false;
- def->is_from_parent = false;
def->storage = 0;
def->raw_default = NULL;
def->cooked_default = NULL;
@@ -1293,7 +1292,6 @@ transformOfType(CreateStmtContext *cxt, TypeName *ofTypename)
n->is_local = true;
n->is_not_null = false;
n->is_from_type = true;
- n->is_from_parent = false;
n->storage = 0;
n->raw_default = NULL;
n->cooked_default = NULL;
diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h
index 1f80dfaa85..57fa4db0d3 100644
--- a/src/include/nodes/parsenodes.h
+++ b/src/include/nodes/parsenodes.h
@@ -642,7 +642,7 @@ typedef struct ColumnDef
bool is_local; /* column has local (non-inherited) def'n */
bool is_not_null; /* NOT NULL constraint specified? */
bool is_from_type; /* column definition came from table type */
- bool is_from_parent; /* column def came from partition parent */
+ bool is_from_parent; /* XXX unused */
char storage; /* attstorage setting, or 0 for default */
Node *raw_default; /* default value (untransformed parse tree) */
Node *cooked_default; /* default value (transformed expr tree) */
diff --git a/src/test/regress/expected/create_table.out b/src/test/regress/expected/create_table.out
index 481510c2bb..70c7a73689 100644
--- a/src/test/regress/expected/create_table.out
+++ b/src/test/regress/expected/create_table.out
@@ -657,6 +657,22 @@ ERROR: column "c" named in partition key does not exist
CREATE TABLE part_c PARTITION OF parted (b WITH OPTIONS NOT NULL DEFAULT 0) FOR VALUES IN ('c') PARTITION BY RANGE ((b));
-- create a level-2 partition
CREATE TABLE part_c_1_10 PARTITION OF part_c FOR VALUES FROM (1) TO (10);
+-- check that NOT NULL and default value are inherited correctly
+create table parted_notnull_inh_test (a int default 1, b int not null default 0) partition by list (a);
+create table parted_notnull_inh_test1 partition of parted_notnull_inh_test (a not null, b default 1) for values in (1);
+insert into parted_notnull_inh_test (b) values (null);
+ERROR: null value in column "b" violates not-null constraint
+DETAIL: Failing row contains (1, null).
+-- note that while b's default is overriden, a's default is preserved
+\d parted_notnull_inh_test1
+ Table "public.parted_notnull_inh_test1"
+ Column | Type | Collation | Nullable | Default
+--------+---------+-----------+----------+---------
+ a | integer | | not null | 1
+ b | integer | | not null | 1
+Partition of: parted_notnull_inh_test FOR VALUES IN (1)
+
+drop table parted_notnull_inh_test;
-- Partition bound in describe output
\d+ part_b
Table "public.part_b"
diff --git a/src/test/regress/sql/create_table.sql b/src/test/regress/sql/create_table.sql
index bc6191f4ac..d8776a81ec 100644
--- a/src/test/regress/sql/create_table.sql
+++ b/src/test/regress/sql/create_table.sql
@@ -604,6 +604,14 @@ CREATE TABLE part_c PARTITION OF parted (b WITH OPTIONS NOT NULL DEFAULT 0) FOR
-- create a level-2 partition
CREATE TABLE part_c_1_10 PARTITION OF part_c FOR VALUES FROM (1) TO (10);
+-- check that NOT NULL and default value are inherited correctly
+create table parted_notnull_inh_test (a int default 1, b int not null default 0) partition by list (a);
+create table parted_notnull_inh_test1 partition of parted_notnull_inh_test (a not null, b default 1) for values in (1);
+insert into parted_notnull_inh_test (b) values (null);
+-- note that while b's default is overriden, a's default is preserved
+\d parted_notnull_inh_test1
+drop table parted_notnull_inh_test;
+
-- Partition bound in describe output
\d+ part_b
--
2.11.0
Hi,
On 2018/11/07 0:10, Alvaro Herrera wrote:
Looking over the stuff in gram.y (to make sure there's nothing that
could be lost), I noticed that we're losing the COLLATE clause when they
are added to columns in partitions. I would expect part1 to end up with
collation es_CL, or alternatively that the command throws an error:55462 10.6 138851=# create table part (a text collate "en_US") partition by range (a);
CREATE TABLE
Duración: 23,511 ms
55462 10.6 138851=# create table part1 partition of part (a collate "es_CL") for values from ('ca') to ('cu');
CREATE TABLE
Duración: 111,551 ms
55462 10.6 138851=# \d part
Tabla «public.part»
Columna │ Tipo │ Collation │ Nullable │ Default
─────────┼──────┼───────────┼──────────┼─────────
a │ text │ en_US │ │
Partition key: RANGE (a)
Number of partitions: 1 (Use \d+ to list them.)55462 10.6 138851=# \d part1
Tabla «public.part1»
Columna │ Tipo │ Collation │ Nullable │ Default
─────────┼──────┼───────────┼──────────┼─────────
a │ text │ en_US │ │
Partition of: part FOR VALUES FROM ('ca') TO ('cu')(This case is particularly bothersome because the column is the
partition key, so the partition range bounds would differ depending on
which collation is used to compare. I assume we'd always want to use
the parent table's collation; so there's even a stronger case for
raising an error if it doesn't match the parent's. However, this case
could arise for other columns too, where it's not *so* bad, but still
not completely correct I think.)
Thank you for investigating.
I think the result in this case should be an error, just as it would in
the regular inheritance case.
create table parent (a text);
create table child (a text collate "en_US") inherits (parent);
NOTICE: merging column "a" with inherited definition
ERROR: column "a" has a collation conflict
DETAIL: "default" versus "en_US"
In fact, I see that ATTACH PARTITION rejects a partition if collations
don't match.
create table part (a text collate "en_US") partition by range (a);
create table part1 (a text collate "es_CL");
alter table part attach partition part1 for values from ('ca') to ('cu');
ERROR: child table "part1" has different collation for column "a"
This happens on unpatched code, and doesn't seem covered by any tests.
However, this seems an independent bug that isn't affected by this
patch.The only other things there are deferrability markers, which seem to be
propagated in a relatively sane fashion (but no luck if you want to add
foreign keys with mismatching deferrability than the parent's -- you
just get a dupe, and there's no way in the grammar to change the flags
for the existing constraint). But you can add a UNIQUE DEFERRED
constraint in a partition that wasn't there in the parent, for example.
Looking again at MergeAttributes, it seems that the fix for disallowing
mismatching collations is not that invasive. PFA a patch that applies on
top of your 0001-Revise-attribute-handling-code-on-partition-creation.patch.
I haven't looked closely at the cases involving deferrability markers though.
Thanks,
Amit
Attachments:
0002-Disallow-creating-partitions-with-mismatching-collat.patchtext/plain; charset=UTF-8; name=0002-Disallow-creating-partitions-with-mismatching-collat.patchDownload
From d2d2489e4978366b4fb489c60d85540264ba1064 Mon Sep 17 00:00:00 2001
From: amit <amitlangote09@gmail.com>
Date: Wed, 7 Nov 2018 10:32:14 +0900
Subject: [PATCH 2/2] Disallow creating partitions with mismatching collations
for columns
---
src/backend/commands/tablecmds.c | 21 +++++++++++++++++++++
src/test/regress/expected/create_table.out | 6 ++++++
src/test/regress/sql/create_table.sql | 5 +++++
3 files changed, 32 insertions(+)
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index a0279ae383..fbf902143b 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -2446,6 +2446,10 @@ MergeAttributes(List *schema, List *supers, char relpersistence,
if (strcmp(coldef->colname, restdef->colname) == 0)
{
+ Oid defTypeId;
+ int32 deftypmod;
+ Oid newCollId;
+
found = true;
coldef->is_not_null |= restdef->is_not_null;
@@ -2465,6 +2469,23 @@ MergeAttributes(List *schema, List *supers, char relpersistence,
coldef->raw_default = restdef->raw_default;
coldef->cooked_default = NULL;
}
+
+ /*
+ * Collation must be same, so error out if a different one
+ * specified for the partition.
+ */
+ typenameTypeIdAndMod(NULL, coldef->typeName,
+ &defTypeId, &deftypmod);
+ newCollId = GetColumnDefCollation(NULL, restdef,
+ defTypeId);
+ if (newCollId != coldef->collOid)
+ ereport(ERROR,
+ (errcode(ERRCODE_COLLATION_MISMATCH),
+ errmsg("column \"%s\" has a collation conflict",
+ coldef->colname),
+ errdetail("\"%s\" versus \"%s\"",
+ get_collation_name(newCollId),
+ get_collation_name(coldef->collOid))));
}
}
diff --git a/src/test/regress/expected/create_table.out b/src/test/regress/expected/create_table.out
index bfc0dc7f6d..c921c8dc36 100644
--- a/src/test/regress/expected/create_table.out
+++ b/src/test/regress/expected/create_table.out
@@ -744,6 +744,12 @@ DETAIL: Failing row contains (1, null).
Partition of: parted_notnull_inh_test FOR VALUES IN (1)
drop table parted_notnull_inh_test;
+-- check that conflicting COLLATE clause for partition disallowed
+create table parted_collate_must_match (a text collate "C") partition by range (a);
+create table parted_collate_must_match1 partition of parted_collate_must_match (a collate "POSIX") for values from ('a') to ('z');
+ERROR: column "a" has a collation conflict
+DETAIL: "POSIX" versus "C"
+drop table parted_collate_must_match;
-- Partition bound in describe output
\d+ part_b
Table "public.part_b"
diff --git a/src/test/regress/sql/create_table.sql b/src/test/regress/sql/create_table.sql
index ee49e82a62..3875b38fbd 100644
--- a/src/test/regress/sql/create_table.sql
+++ b/src/test/regress/sql/create_table.sql
@@ -662,6 +662,11 @@ insert into parted_notnull_inh_test (b) values (null);
\d parted_notnull_inh_test1
drop table parted_notnull_inh_test;
+-- check that conflicting COLLATE clause for partition disallowed
+create table parted_collate_must_match (a text collate "C") partition by range (a);
+create table parted_collate_must_match1 partition of parted_collate_must_match (a collate "POSIX") for values from ('a') to ('z');
+drop table parted_collate_must_match;
+
-- Partition bound in describe output
\d+ part_b
--
2.11.0
On 2018-Nov-07, Amit Langote wrote:
I think the result in this case should be an error, just as it would in
the regular inheritance case.create table parent (a text);
create table child (a text collate "en_US") inherits (parent);
NOTICE: merging column "a" with inherited definition
ERROR: column "a" has a collation conflict
DETAIL: "default" versus "en_US"In fact, I see that ATTACH PARTITION rejects a partition if collations
don't match.
Hmm, I'm thinking perhaps we shouldn't backpatch this part. It's
obviously a bug, but we might break somebody's working apps. Therefore
I think I'd rather leave it out of the current bugfix and commit
separately.
--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Fri, Nov 9, 2018 at 1:03 AM Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
On 2018-Nov-07, Amit Langote wrote:
I think the result in this case should be an error, just as it would in
the regular inheritance case.create table parent (a text);
create table child (a text collate "en_US") inherits (parent);
NOTICE: merging column "a" with inherited definition
ERROR: column "a" has a collation conflict
DETAIL: "default" versus "en_US"In fact, I see that ATTACH PARTITION rejects a partition if collations
don't match.Hmm, I'm thinking perhaps we shouldn't backpatch this part. It's
obviously a bug, but we might break somebody's working apps. Therefore
I think I'd rather leave it out of the current bugfix and commit
separately.
Okay, that may be fine because nothing wrong is happening by silently
ignoring the partition's specified collation.
Thanks,
Amit
On 2018-Nov-09, Amit Langote wrote:
On Fri, Nov 9, 2018 at 1:03 AM Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
On 2018-Nov-07, Amit Langote wrote:
Hmm, I'm thinking perhaps we shouldn't backpatch this part. It's
obviously a bug, but we might break somebody's working apps. Therefore
I think I'd rather leave it out of the current bugfix and commit
separately.Okay, that may be fine because nothing wrong is happening by silently
ignoring the partition's specified collation.
Actually, how about we reduce the message to a WARNING in released
branches, and ERROR in master? Broken applications would continue to
work, but users might become aware that there might be a problem.
--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Pushed.
I included the test case for collations to the three branches, but no
code changes. We can patch master for the handling of collations per
your patch, and if somebody has it, a change to how defaults are applied
when routing tuples.
Thanks to J�rgen for reporting the bug.
--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 2018/11/09 1:38, Alvaro Herrera wrote:
On 2018-Nov-09, Amit Langote wrote:
On Fri, Nov 9, 2018 at 1:03 AM Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
On 2018-Nov-07, Amit Langote wrote:
Hmm, I'm thinking perhaps we shouldn't backpatch this part. It's
obviously a bug, but we might break somebody's working apps. Therefore
I think I'd rather leave it out of the current bugfix and commit
separately.Okay, that may be fine because nothing wrong is happening by silently
ignoring the partition's specified collation.Actually, how about we reduce the message to a WARNING in released
branches, and ERROR in master? Broken applications would continue to
work, but users might become aware that there might be a problem.
That might work too. Would you like me to create and post patches like
that for 10 and 11 branches?
Thanks,
Amit
On 2018/11/09 4:39, Alvaro Herrera wrote:
Pushed.
Thank you for committing. I've closed the CF entry.
I included the test case for collations to the three branches, but no
code changes. We can patch master for the handling of collations per
your patch,
Okay, but should we back-patch it by adding WARNING to back-branches as
you suggest?
and if somebody has it, a change to how defaults are applied
when routing tuples.
I haven't written such a patch yet. Do we want such a feature?
Thanks,
Amit
On 2018/11/09 14:04, Amit Langote wrote:
On 2018/11/09 4:39, Alvaro Herrera wrote:
and if somebody has it, a change to how defaults are applied
when routing tuples.I haven't written such a patch yet. Do we want such a feature?
Or is it a *bug* of tuple-routing that it doesn't substitute default
values that may be defined for partitions? It kind of looks like one if
you see an example like this.
create table p (a int, b int) partition by list (a);
create table p1 partition of p (b not null default 1) for values in (1);
insert into p1 values (1);
table p;
a │ b
───┼───
1 │ 1
(1 row)
insert into p values (1);
ERROR: null value in column "b" violates not-null constraint
DETAIL: Failing row contains (1, null).
Thanks,
Amit
On 2018-Nov-09, Amit Langote wrote:
Or is it a *bug* of tuple-routing that it doesn't substitute default
values that may be defined for partitions? It kind of looks like one if
you see an example like this.create table p (a int, b int) partition by list (a);
create table p1 partition of p (b not null default 1) for values in (1);
insert into p1 values (1);
table p;
a │ b
───┼───
1 │ 1
(1 row)insert into p values (1);
ERROR: null value in column "b" violates not-null constraint
DETAIL: Failing row contains (1, null).
I don't know. I wonder if the bug isn't that we allow the default to be
specified for the partition at all -- why shouldn't we just reject that
with an error?
See this example, where the inserts give values that could be said to be
inconsistent:
create table p (a int , b int default 3) partition by list (a);
create table p1 partition of p (b default 42) for values in (1);
insert into p (a) values (1);
insert into p1 (a) values (1);
select * from p;
a │ b
───┼────
1 │ 3
1 │ 42
(2 filas)
--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 2018-11-09 16:59, Alvaro Herrera wrote:
On 2018-Nov-09, Amit Langote wrote:
Or is it a *bug* of tuple-routing that it doesn't substitute default
values that may be defined for partitions? It kind of looks like one if
you see an example like this.create table p (a int, b int) partition by list (a);
create table p1 partition of p (b not null default 1) for values in (1);
insert into p1 values (1);
table p;
a │ b
───┼───
1 │ 1
(1 row)insert into p values (1);
ERROR: null value in column "b" violates not-null constraint
DETAIL: Failing row contains (1, null).I don't know. I wonder if the bug isn't that we allow the default to be
specified for the partition at all -- why shouldn't we just reject that
with an error?See this example, where the inserts give values that could be said to be
inconsistent:create table p (a int , b int default 3) partition by list (a);
create table p1 partition of p (b default 42) for values in (1);
insert into p (a) values (1);
insert into p1 (a) values (1);
select * from p;a │ b
───┼────
1 │ 3
1 │ 42
(2 filas)
I found this problem while attempting to create sub-ids with partition
defaults using distinct per-partition sequences.
I can think of other useful scenarios too, but if you don't want to
support it because of unexpected complexities all my cases can still be
implemented using custom triggers albeit slower and more inconvenient.
Regarding your example, what I expected is that *both* inserts would
consistently result in a tuple of (1, 42) since p should route the
insert to p1 and use p1's defaults. The current inconsistent behavior is
the heart of the bug.
Best regards,
Jürgen
On 2018-Nov-09, J�rgen Strobel wrote:
Regarding your example, what I expected is that *both* inserts would
consistently result in a tuple of (1, 42) since p should route the
insert to p1 and use p1's defaults. The current inconsistent behavior is
the heart of the bug.
I think that would be sensible behavior, as long as the partition
doesn't override values for the partitioning column -- i.e. if the
default values don't re-route the tuple to another partition, I think we
should use the partition's default rather than the parent. This says we
should expand defaults after routing. However, can we really route if
we haven't expanded defaults? In general, we may be lacking values for
some columns of the partition key. Another point: if we've already
expanded defaults when processing at the parent level, when we reach the
partition we don't know which values are still missing default
expansion, or which defaults coming from the parent ought to be
re-expanded.
So this looks to be a bit of a landmine.
In any case it seems really hard to see this is as a bug that we would
fix in back-branches.
--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 2018-11-09 18:07, Alvaro Herrera wrote:
On 2018-Nov-09, Jürgen Strobel wrote:
Regarding your example, what I expected is that *both* inserts would
consistently result in a tuple of (1, 42) since p should route the
insert to p1 and use p1's defaults. The current inconsistent behavior is
the heart of the bug.I think that would be sensible behavior, as long as the partition
doesn't override values for the partitioning column -- i.e. if the
default values don't re-route the tuple to another partition, I think we
should use the partition's default rather than the parent. This says we
should expand defaults after routing. However, can we really route if
we haven't expanded defaults? In general, we may be lacking values for
some columns of the partition key. Another point: if we've already
expanded defaults when processing at the parent level, when we reach the
partition we don't know which values are still missing default
expansion, or which defaults coming from the parent ought to be
re-expanded.So this looks to be a bit of a landmine.
In any case it seems really hard to see this is as a bug that we would
fix in back-branches.
I am slightly confused now, I thought this landmine was already fixed in
master and what remains is only whether to backport it or not? Which I
guess depends on whether this is classified as a bug or not.
Since the fix has the potential to break current applications and the
documentation is vague about wanted behavior I think it would be
sensible to add a warning only, even if we agree to call it a bug.
Best regards,
Jürgen
On 2018-Nov-09, J�rgen Strobel wrote:
I am slightly confused now, I thought this landmine was already fixed in
master and what remains is only whether to backport it or not? Which I
guess depends on whether this is classified as a bug or not.
Hmm, I changed (and back-patched) what happens to the NOT NULL clauses,
but as I understand the behavior of the DEFAULT clauses has not changed.
Since the fix has the potential to break current applications and the
documentation is vague about wanted behavior I think it would be
sensible to add a warning only, even if we agree to call it a bug.
Hmm, the part I was first proposing to backpatch as an ERROR was a
mismatch in the COLLATE clause; then I talked about using a WARNING. I
ended up backpatching neither -- only immortalized the current behavior
in a test case. I think it should be an ERROR, but in master only.
The DEFAULT clauses are a different problem (landmine) :-)
--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 2018-11-09 22:11, Alvaro Herrera wrote:
On 2018-Nov-09, Jürgen Strobel wrote:
I am slightly confused now, I thought this landmine was already fixed in
master and what remains is only whether to backport it or not? Which I
guess depends on whether this is classified as a bug or not.Hmm, I changed (and back-patched) what happens to the NOT NULL clauses,
but as I understand the behavior of the DEFAULT clauses has not changed.Since the fix has the potential to break current applications and the
documentation is vague about wanted behavior I think it would be
sensible to add a warning only, even if we agree to call it a bug.Hmm, the part I was first proposing to backpatch as an ERROR was a
mismatch in the COLLATE clause; then I talked about using a WARNING. I
ended up backpatching neither -- only immortalized the current behavior
in a test case. I think it should be an ERROR, but in master only.The DEFAULT clauses are a different problem (landmine) :-)
OK got it. I agree about differing COLLATE clauses making no sense and
with the ERROR+WARNING solution, but I didn't report that.
The NULL violation is obvious too.
I still hope for a bug fix for the DEFAULT clause with sensible
restrictions.
-Jürgen
On 2018-Nov-09, J�rgen Strobel wrote:
OK got it. I agree about differing COLLATE clauses making no sense and
with the ERROR+WARNING solution, but I didn't report that.
Yeah, I happened to notice it on code inspection.
The NULL violation is obvious too.
Yeah, IMO that was an obvious bugfix.
I still hope for a bug fix for the DEFAULT clause with sensible
restrictions.
Yeah, I understand that this is the one that you really care about.
However, I think a fix is unlikely to be back-patchable, because it may
break existing applications that are written to expect the current
behavior.
--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Alvaro Herrera <alvherre@2ndquadrant.com> writes:
On 2018-Nov-09, Jürgen Strobel wrote:
Regarding your example, what I expected is that *both* inserts would
consistently result in a tuple of (1, 42) since p should route the
insert to p1 and use p1's defaults. The current inconsistent behavior is
the heart of the bug.
I think that would be sensible behavior, as long as the partition
doesn't override values for the partitioning column -- i.e. if the
default values don't re-route the tuple to another partition, I think we
should use the partition's default rather than the parent. This says we
should expand defaults after routing.
I'd argue not, actually. I think there is plausible precedent in
updatable views, where what we use is the defaults associated with the
view, not the underlying table. Correspondingly, what ought to govern
in a partitioned insert is the defaults associated with the table actually
named in the insert command, never mind what its partitions might say.
That is useful for many situations, and it avoids all the logical
inconsistencies you get into if you find that the defaults associated
with some partition would force re-routing elsewhere.
In any case, you can't make this happen without basically blowing up
default processing altogether. Defaults are currently expanded by the
planner, and pretty early too. To make it work like the OP wishes,
we'd have to insert them sometime late in the executor.
In any case it seems really hard to see this is as a bug that we would
fix in back-branches.
Backwards compatibility considerations would prevent that in any case.
regards, tom lane
On 2018/11/10 7:33, Tom Lane wrote:
Alvaro Herrera <alvherre@2ndquadrant.com> writes:
On 2018-Nov-09, Jürgen Strobel wrote:
Regarding your example, what I expected is that *both* inserts would
consistently result in a tuple of (1, 42) since p should route the
insert to p1 and use p1's defaults. The current inconsistent behavior is
the heart of the bug.I think that would be sensible behavior, as long as the partition
doesn't override values for the partitioning column -- i.e. if the
default values don't re-route the tuple to another partition, I think we
should use the partition's default rather than the parent. This says we
should expand defaults after routing.I'd argue not, actually. I think there is plausible precedent in
updatable views, where what we use is the defaults associated with the
view, not the underlying table. Correspondingly, what ought to govern
in a partitioned insert is the defaults associated with the table actually
named in the insert command, never mind what its partitions might say.
That is useful for many situations, and it avoids all the logical
inconsistencies you get into if you find that the defaults associated
with some partition would force re-routing elsewhere.In any case, you can't make this happen without basically blowing up
default processing altogether. Defaults are currently expanded by the
planner, and pretty early too. To make it work like the OP wishes,
we'd have to insert them sometime late in the executor.
Considering multi-level partitioning, that'd mean the tuple being routed
would need to be filled with the defaults of every table on the way to a
leaf partition, including that of the leaf partition where the tuple
finally ends up. If we re-route upon the final leaf partition's defaults
leading to partition constraint violation, it's possible that someone
might end up setting up an infinite re-routing loop with that behavior.
create table p (a int) partition by list (a);
create table p1 partition of p (a default 2) for values in (1);
create table p2 partition of p (a default 1) for values in (2);
It won't be fun for the server to try to prevent that kind of thing.
IOW, it might be a good idea to call the ability to set partition-level
defaults a deprecated feature?
Thanks,
Amit
Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> writes:
On 2018/11/10 7:33, Tom Lane wrote:
I'd argue not, actually. I think there is plausible precedent in
updatable views, where what we use is the defaults associated with the
view, not the underlying table. Correspondingly, what ought to govern
in a partitioned insert is the defaults associated with the table actually
named in the insert command, never mind what its partitions might say.
That is useful for many situations, and it avoids all the logical
inconsistencies you get into if you find that the defaults associated
with some partition would force re-routing elsewhere.
...
IOW, it might be a good idea to call the ability to set partition-level
defaults a deprecated feature?
Not necessarily. They'd apply when you insert directly into a particular
partition by name.
regards, tom lane
On 2018/11/12 12:59, Tom Lane wrote:
Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> writes:
On 2018/11/10 7:33, Tom Lane wrote:
I'd argue not, actually. I think there is plausible precedent in
updatable views, where what we use is the defaults associated with the
view, not the underlying table. Correspondingly, what ought to govern
in a partitioned insert is the defaults associated with the table actually
named in the insert command, never mind what its partitions might say.
That is useful for many situations, and it avoids all the logical
inconsistencies you get into if you find that the defaults associated
with some partition would force re-routing elsewhere....
IOW, it might be a good idea to call the ability to set partition-level
defaults a deprecated feature?Not necessarily. They'd apply when you insert directly into a particular
partition by name.
Yes. Maybe, we should document that the partition default are not honored
when inserting through the parent.
Thanks,
Amit
On 2018-11-09 23:33, Tom Lane wrote:
Alvaro Herrera <alvherre@2ndquadrant.com> writes:
On 2018-Nov-09, Jürgen Strobel wrote:
Regarding your example, what I expected is that *both* inserts would
consistently result in a tuple of (1, 42) since p should route the
insert to p1 and use p1's defaults. The current inconsistent behavior is
the heart of the bug.I think that would be sensible behavior, as long as the partition
doesn't override values for the partitioning column -- i.e. if the
default values don't re-route the tuple to another partition, I think we
should use the partition's default rather than the parent. This says we
should expand defaults after routing.I'd argue not, actually. I think there is plausible precedent in
updatable views, where what we use is the defaults associated with the
view, not the underlying table. Correspondingly, what ought to govern
in a partitioned insert is the defaults associated with the table actually
named in the insert command, never mind what its partitions might say.
That is useful for many situations, and it avoids all the logical
inconsistencies you get into if you find that the defaults associated
with some partition would force re-routing elsewhere.In any case, you can't make this happen without basically blowing up
default processing altogether. Defaults are currently expanded by the
planner, and pretty early too. To make it work like the OP wishes,
we'd have to insert them sometime late in the executor.In any case it seems really hard to see this is as a bug that we would
fix in back-branches.Backwards compatibility considerations would prevent that in any case.
I don't really think the view comparison holds water. Views are layered
above tables without affecting them by design, but partitions are
intrinsic parts of sharded tables and only make sense as a whole.
With all due respect I suspect your view of implementation issues biases
your judgement here. For me the partition's default handling was
completely unexpected, especially with the documentation silent about
default-applying behavior, and inconsistent with how check constraints
work with partitions.
Maybe there's another way to implement this. Like mark early and apply
defaults later, and handle meta data like constraints. Sorry if this
sounds stupid.
Btw I fully agree about not back-patching changes to this, but a
documentation update would be nice.
Best regards,
Jürgen
On 2018-Nov-12, J�rgen Strobel wrote:
With all due respect I suspect your view of implementation issues biases
your judgement here. For me the partition's default handling was
completely unexpected, especially with the documentation silent about
default-applying behavior, and inconsistent with how check constraints
work with partitions.
This is valuable input. It would be good to have more opinions from
users, particularly those accustomed to how things work in other RDBMS
systems, so that we can inform our own opinions on how we should make it
work here in PG going forward.
One of the guiding principles that I think we should hold for
partitioning is that operating directly into the partition should be
seen as only an optimization compared to inserting into the parent table
-- thus it should not behave differently. Applying different default
values depending on where you're inserting into goes counter to that
principle.
Maybe there's another way to implement this. Like mark early and apply
defaults later, and handle meta data like constraints. Sorry if this
sounds stupid.
It does not sound stupid -- it seems exactly what I was thinking.
Admittedly, it's a bit hand-wavy and we'd need to understand more how it
work in more detail, as well as how it works currently (and why it
does).
However, I'm not ATM in a position to design or write a patch for this.
--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Alvaro Herrera <alvherre@2ndquadrant.com> writes:
One of the guiding principles that I think we should hold for
partitioning is that operating directly into the partition should be
seen as only an optimization compared to inserting into the parent table
-- thus it should not behave differently. Applying different default
values depending on where you're inserting into goes counter to that
principle.
I'm not entirely convinced that I buy that argument, especially not in
a case like this where it introduces logical inconsistencies where there
otherwise wouldn't be any.
regards, tom lane
On 2018-11-12 17:33, Tom Lane wrote:
Alvaro Herrera <alvherre@2ndquadrant.com> writes:
One of the guiding principles that I think we should hold for
partitioning is that operating directly into the partition should be
seen as only an optimization compared to inserting into the parent table
-- thus it should not behave differently. Applying different default
values depending on where you're inserting into goes counter to that
principle.I'm not entirely convinced that I buy that argument, especially not in
a case like this where it introduces logical inconsistencies where there
otherwise wouldn't be any.
I think I missed something, what are the *introduced* logical problems?
Apart from implementation issues the only logical problems I see are if
you allow to change defaults of the partition key columns, and these are
problematic (nonsensical really) in either case.
Regards,
Jürgen
=?UTF-8?Q?J=c3=bcrgen_Strobel?= <juergen+postgresql@strobel.info> writes:
On 2018-11-12 17:33, Tom Lane wrote:
I'm not entirely convinced that I buy that argument, especially not in
a case like this where it introduces logical inconsistencies where there
otherwise wouldn't be any.
I think I missed something, what are the *introduced* logical problems?
What to do if a partition introduces a default value that would force
re-routing to some other partition.
Apart from implementation issues the only logical problems I see are if
you allow to change defaults of the partition key columns, and these are
problematic (nonsensical really) in either case.
Just claiming that it's nonsensical doesn't fix the problem. Also, it
is neither problematic nor nonsensical for the root to provide defaults
for partition key columns. So if we go this route, we are giving up
useful behavior (key-column defaults on the root) for useless behavior
(key-column defaults on the partitions).
regards, tom lane
On 2018-11-13 00:57, Tom Lane wrote:
=?UTF-8?Q?J=c3=bcrgen_Strobel?= <juergen+postgresql@strobel.info> writes:
On 2018-11-12 17:33, Tom Lane wrote:
I'm not entirely convinced that I buy that argument, especially not in
a case like this where it introduces logical inconsistencies where there
otherwise wouldn't be any.I think I missed something, what are the *introduced* logical problems?
What to do if a partition introduces a default value that would force
re-routing to some other partition.
I would claim this is a problem which already exists with the current
behavior:
a) If a tuple is inserted through the parent it's ignored and useless.
b) If a tuple is inserted through the partition it can lead to
inconsistency and a constraint violation (possibly rectifiable by a
re-route which I think we all agree is out of the question).
Apart from implementation issues the only logical problems I see are if
you allow to change defaults of the partition key columns, and these are
problematic (nonsensical really) in either case.Just claiming that it's nonsensical doesn't fix the problem. Also, it
is neither problematic nor nonsensical for the root to provide defaults
for partition key columns. So if we go this route, we are giving up
useful behavior (key-column defaults on the root) for useless behavior
(key-column defaults on the partitions).
Yes, I strongly believe defaults on the key columns in partitions should
be disallowed, that's why I called them nonsensical in both cases, see
above a) and b) for why this applies even to current behavior.
Of course disallowing all DEFAULT clauses on partitions is the easiest
way out, and I'd rather see this than inserts through partitions being
more than a pure optimization.
Best regards,
Jürgen
On Tue, 2018-11-13 at 13:44 +0100, Jürgen Strobel wrote:
On 2018-11-13 00:57, Tom Lane wrote:
=?UTF-8?Q?J=c3=bcrgen_Strobel?= <juergen+postgresql@strobel.info>
writes:On 2018-11-12 17:33, Tom Lane wrote:
I'm not entirely convinced that I buy that argument, especially
not in
a case like this where it introduces logical inconsistencies
where there
otherwise wouldn't be any.I think I missed something, what are the *introduced* logical
problems?What to do if a partition introduces a default value that would
force
re-routing to some other partition.I would claim this is a problem which already exists with the current
behavior:
I think the question is what part of the current behavior is
intentional (albeit with corner-cases not handled or rejected
properly), and what part is just not thought through. Not sure.
a) If a tuple is inserted through the parent it's ignored and
useless.
b) If a tuple is inserted through the partition it can lead to
inconsistency and a constraint violation (possibly rectifiable by a
re-route which I think we all agree is out of the question).
Yeah, allowing defaults mismatching the partition seems like a can of
worms. We certainly should not allow such rows to be inserted in the
partition. Routing them somewhere else seems tricky, so perhaps it'd be
better to just error-out in this case.
regards
--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Mon, 2018-11-12 at 11:33 -0500, Tom Lane wrote:
Alvaro Herrera <alvherre@2ndquadrant.com> writes:
One of the guiding principles that I think we should hold for
partitioning is that operating directly into the partition should
be
seen as only an optimization compared to inserting into the parent
table
-- thus it should not behave differently. Applying different
default
values depending on where you're inserting into goes counter to
that
principle.I'm not entirely convinced that I buy that argument, especially not
in a case like this where it introduces logical inconsistencies where
there otherwise wouldn't be any.
Not sure what new logical inconsistencies you have in mind, but the
principle proposed by Alvaro seems sensible to me. I too see insertions
into the partition primarily as an optimization, and would not expect
it to use other defaults than when inserting through the parent.
Particularly not defaults contradicting the partition definition and
forcing rerouting the tuple somewhere else automatically. (IME when
inserting directly into partitions people expect that data to end in
that partition, and if not it's an error - bogus data, ...).
I'm not claiming there are no use cases for defaults on partitions. For
example I can imagine multi-level partitioning, where each layer can
specify defaults for the lower levels. But it seems rather surprising
to only apply the partition defaults for direct insertions.
regards
--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 2018/11/09 14:04, Amit Langote wrote:
On 2018/11/09 4:39, Alvaro Herrera wrote:
I included the test case for collations to the three branches, but no
code changes. We can patch master for the handling of collations per
your patch,Okay, but should we back-patch it by adding WARNING to back-branches as
you suggest?
I was looking at the pending patches that I'd sent and noticed this one to
throw an error when a partition specifies a collation for a column that
doesn't match the parent's. Do we want to apply the attached rebased
patch to HEAD and leave the back-branches (10 and 11) alone?
Thanks,
Amit
Attachments:
0001-Disallow-creating-partitions-with-mismatching-collat.patchtext/plain; charset=UTF-8; name=0001-Disallow-creating-partitions-with-mismatching-collat.patchDownload
From b802e96a78f1d52571c1a83dcfd9053ad32f81b8 Mon Sep 17 00:00:00 2001
From: amit <amitlangote09@gmail.com>
Date: Wed, 7 Nov 2018 10:32:14 +0900
Subject: [PATCH] Disallow creating partitions with mismatching collations for
columns
---
src/backend/commands/tablecmds.c | 21 +++++++++++++++++++++
src/test/regress/expected/create_table.out | 4 ++++
2 files changed, 25 insertions(+)
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index d6d0de1b01..ec583bb74c 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -2413,6 +2413,10 @@ MergeAttributes(List *schema, List *supers, char relpersistence,
if (strcmp(coldef->colname, restdef->colname) == 0)
{
+ Oid defTypeId;
+ int32 deftypmod;
+ Oid newCollId;
+
found = true;
coldef->is_not_null |= restdef->is_not_null;
@@ -2432,6 +2436,23 @@ MergeAttributes(List *schema, List *supers, char relpersistence,
coldef->raw_default = restdef->raw_default;
coldef->cooked_default = NULL;
}
+
+ /*
+ * Collation must be same, so error out if a different one
+ * specified for the partition.
+ */
+ typenameTypeIdAndMod(NULL, coldef->typeName,
+ &defTypeId, &deftypmod);
+ newCollId = GetColumnDefCollation(NULL, restdef,
+ defTypeId);
+ if (newCollId != coldef->collOid)
+ ereport(ERROR,
+ (errcode(ERRCODE_COLLATION_MISMATCH),
+ errmsg("column \"%s\" has a collation conflict",
+ coldef->colname),
+ errdetail("\"%s\" versus \"%s\"",
+ get_collation_name(newCollId),
+ get_collation_name(coldef->collOid))));
}
}
diff --git a/src/test/regress/expected/create_table.out b/src/test/regress/expected/create_table.out
index 7e52c27e3f..f3dd9d964f 100644
--- a/src/test/regress/expected/create_table.out
+++ b/src/test/regress/expected/create_table.out
@@ -770,9 +770,13 @@ create table parted_collate_must_match (a text collate "C", b text collate "C")
-- on the partition key
create table parted_collate_must_match1 partition of parted_collate_must_match
(a collate "POSIX") for values from ('a') to ('m');
+ERROR: column "a" has a collation conflict
+DETAIL: "POSIX" versus "C"
-- on another column
create table parted_collate_must_match2 partition of parted_collate_must_match
(b collate "POSIX") for values from ('m') to ('z');
+ERROR: column "b" has a collation conflict
+DETAIL: "POSIX" versus "C"
drop table parted_collate_must_match;
-- Partition bound in describe output
\d+ part_b
--
2.11.0