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
From 1158793b899bbc1f52e37b2255bd74121a293495 Mon Sep 17 00:00:00 2001
From: amit <amitlangot...@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
From 6ab930fa7e8f6f5e548b783451f2ab7d4515909b Mon Sep 17 00:00:00 2001
From: amit <amitlangot...@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
