On 2016/07/22 0:38, Robert Haas wrote:
> On Wed, Jul 13, 2016 at 5:22 AM, Amit Langote
> <langote_amit...@lab.ntt.co.jp> wrote:
>> Consider a scenario where one adds a *valid* constraint on a inheritance
>> parent which is then merged with a child table's *not valid* constraint
>> during inheritance recursion. If merged, the constraint is not checked
>> for the child data even though it may have some. Is that an oversight?
>
> Seems like it. I'd recommend we just error out in that case and tell
> the user that they should validate the child's constraint first.
Agreed.
Patch attached. In addition to the recursion from parent case, this seems
to be broken for the alter table child inherit parent case as well. So,
fixed both MergeWithExistingConstraint (called from
AddRelationNewConstraints) and MergeConstraintsIntoExisting (called from
ATExecAddInherit). I had to add a new argument is_not_valid to the former
to signal whether the constraint being propagated itself is declared NOT
VALID, in which we can proceed with merging. Also added some tests for
both cases.
Thanks,
Amit
diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c
index e997b57..f6f0691 100644
--- a/src/backend/catalog/heap.c
+++ b/src/backend/catalog/heap.c
@@ -105,7 +105,7 @@ static void StoreConstraints(Relation rel, List *cooked_constraints,
bool is_internal);
static bool MergeWithExistingConstraint(Relation rel, char *ccname, Node *expr,
bool allow_merge, bool is_local,
- bool is_no_inherit);
+ bool is_no_inherit, bool is_not_valid);
static void SetRelationNumChecks(Relation rel, int numchecks);
static Node *cookConstraint(ParseState *pstate,
Node *raw_constraint,
@@ -2301,7 +2301,8 @@ AddRelationNewConstraints(Relation rel,
*/
if (MergeWithExistingConstraint(rel, ccname, expr,
allow_merge, is_local,
- cdef->is_no_inherit))
+ cdef->is_no_inherit,
+ cdef->skip_validation))
continue;
}
else
@@ -2389,7 +2390,7 @@ AddRelationNewConstraints(Relation rel,
static bool
MergeWithExistingConstraint(Relation rel, char *ccname, Node *expr,
bool allow_merge, bool is_local,
- bool is_no_inherit)
+ bool is_no_inherit, bool is_not_valid)
{
bool found;
Relation conDesc;
@@ -2452,6 +2453,14 @@ MergeWithExistingConstraint(Relation rel, char *ccname, Node *expr,
errmsg("constraint \"%s\" conflicts with non-inherited constraint on relation \"%s\"",
ccname, RelationGetRelationName(rel))));
+ /* Cannot merge if the child's constraint is yet invalid. */
+ if (!is_local && !is_not_valid && !con->convalidated)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
+ errmsg("cannot merge constraint \"%s\" with NOT VALID constraint on relation \"%s\"",
+ ccname, RelationGetRelationName(rel)),
+ errhint("Please validate the constraint on the child table first.")));
+
if (is_local)
con->conislocal = true;
else
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 86e9814..5e72029 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -10380,6 +10380,14 @@ MergeConstraintsIntoExisting(Relation child_rel, Relation parent_rel)
NameStr(child_con->conname),
RelationGetRelationName(child_rel))));
+ /* Cannot merge if the child's constraint is yet invalid. */
+ if (parent_con->convalidated && !child_con->convalidated)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
+ errmsg("cannot merge constraint \"%s\" with NOT VALID constraint on child table \"%s\"",
+ NameStr(parent_con->conname), RelationGetRelationName(child_rel)),
+ errhint("Please validate the constraint on the child table first.")));
+
/*
* OK, bump the child constraint's inheritance count. (If we fail
* later on, this change will just roll back.)
diff --git a/src/test/regress/expected/alter_table.out b/src/test/regress/expected/alter_table.out
index 3232cda..9f13c47 100644
--- a/src/test/regress/expected/alter_table.out
+++ b/src/test/regress/expected/alter_table.out
@@ -460,6 +460,43 @@ Check constraints:
Inherits: nv_parent
-- we leave nv_parent and children around to help test pg_dump logic
+-- prevent merge of constraint with NOT VALID constraint on a child table
+create table parent(a int);
+create table child () inherits (parent);
+alter table child add constraint chk_a check (a > 0) not valid;
+-- ok to merge a NOT VALID constraint itself
+alter table parent add constraint chk_a check (a > 0) not valid;
+NOTICE: merging constraint "chk_a" with inherited definition
+alter table parent drop constraint chk_a;
+-- error
+alter table parent add constraint chk_a check (a > 0);
+ERROR: cannot merge constraint "chk_a" with NOT VALID constraint on relation "child"
+HINT: Please validate the constraint on the child table first.
+-- ok to merge upon validate
+alter table child validate constraint chk_a;
+alter table parent add constraint chk_a check (a > 0);
+NOTICE: merging constraint "chk_a" with inherited definition
+-- check the ALTER TABLE INHERIT case as well
+drop table child;
+create table child(a int);
+alter table child add constraint chk_a check (a > 0) not valid;
+-- ok to merge a NOT VALID constraint itself
+alter table parent drop constraint chk_a;
+alter table parent add constraint chk_a check (a > 0) not valid;
+alter table child inherit parent;
+alter table child no inherit parent;
+alter table parent drop constraint chk_a;
+alter table parent add constraint chk_a check (a > 0);
+-- error
+alter table child inherit parent;
+ERROR: cannot merge constraint "chk_a" with NOT VALID constraint on child table "child"
+HINT: Please validate the constraint on the child table first.
+-- ok to merge upon validate
+alter table child validate constraint chk_a;
+alter table child inherit parent;
+-- clean up
+drop table parent cascade;
+NOTICE: drop cascades to table child
-- Foreign key adding test with mixed types
-- Note: these tables are TEMP to avoid name conflicts when this test
-- is run in parallel with foreign_key.sql.
diff --git a/src/test/regress/sql/alter_table.sql b/src/test/regress/sql/alter_table.sql
index 72e65d4..55bf2e9 100644
--- a/src/test/regress/sql/alter_table.sql
+++ b/src/test/regress/sql/alter_table.sql
@@ -348,6 +348,45 @@ alter table nv_parent add check (d between '2001-01-01'::date and '2099-12-31'::
\d nv_child_2009
-- we leave nv_parent and children around to help test pg_dump logic
+-- prevent merge of constraint with NOT VALID constraint on a child table
+
+create table parent(a int);
+create table child () inherits (parent);
+alter table child add constraint chk_a check (a > 0) not valid;
+
+-- ok to merge a NOT VALID constraint itself
+alter table parent add constraint chk_a check (a > 0) not valid;
+
+alter table parent drop constraint chk_a;
+-- error
+alter table parent add constraint chk_a check (a > 0);
+-- ok to merge upon validate
+alter table child validate constraint chk_a;
+alter table parent add constraint chk_a check (a > 0);
+
+-- check the ALTER TABLE INHERIT case as well
+
+drop table child;
+create table child(a int);
+alter table child add constraint chk_a check (a > 0) not valid;
+
+-- ok to merge a NOT VALID constraint itself
+alter table parent drop constraint chk_a;
+alter table parent add constraint chk_a check (a > 0) not valid;
+alter table child inherit parent;
+
+alter table child no inherit parent;
+alter table parent drop constraint chk_a;
+alter table parent add constraint chk_a check (a > 0);
+-- error
+alter table child inherit parent;
+-- ok to merge upon validate
+alter table child validate constraint chk_a;
+alter table child inherit parent;
+
+-- clean up
+drop table parent cascade;
+
-- Foreign key adding test with mixed types
-- Note: these tables are TEMP to avoid name conflicts when this test
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
