From: Alvaro Herrera <alvherre@2ndquadrant.com>
To: Amit Langote <Langote_Amit_f8@lab.ntt.co.jp>
Cc: Ashutosh Bapat <ashutosh.bapat@enterprisedb.com>
Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Cc: Amit Langote <amitlangote09@gmail.com>
Subject: Re: BUG #15212: Default values in partition tables don't work as expected and allow NOT NULL violation
Date: Mon, 5 Nov 2018 19:25:09 -0300
Message-ID: <20181105222509.k26zipgeixc6fkjf@alvherre.pgsql> (raw)
In-Reply-To: <bca0f1f7-aa10-4719-7cd8-08b82fcb2fd1@lab.ntt.co.jp>
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.patch (text/x-diff, 11.5K) downloaddiff --git a/src/backend/commands/sequence.c b/src/backend/commands/sequence.cindex 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.cindex 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.cindex 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.cindex 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.yindex 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.cindex 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.hindex 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.outindex 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.sqlindex 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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: alvherre@2ndquadrant.com, Langote_Amit_f8@lab.ntt.co.jp, ashutosh.bapat@enterprisedb.com, pgsql-hackers@lists.postgresql.org, amitlangote09@gmail.com
Subject: Re: BUG #15212: Default values in partition tables don't work as expected and allow NOT NULL violation
In-Reply-To: <20181105222509.k26zipgeixc6fkjf@alvherre.pgsql>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox