I've been looking into the misbehavior complained of here: https://www.postgresql.org/message-id/CADbMkNPT-Jz5PRSQ4RbUASYAjocV_KHUWapR%2Bg8fNvhUAyRpxA%40mail.gmail.com
I originally thought this was pg_dump's fault, but I now think it's not, or at least that changing the backend's behavior would be a superior solution. The problem can be described thus: create table parent(f1 int); create table child() inherits(parent); alter table parent add constraint inh_check check(f1 > 0) not valid; alter table child add constraint inh_check check(f1 > 0) not valid; The above sequence fails with ERROR: constraint "inh_check" for relation "child" already exists However, if you swap the order of the two ALTER ADD commands, it succeeds, with the second ALTER instead saying NOTICE: merging constraint "inh_check" with inherited definition In that case you end up with a child constraint marked as having both local origin (conislocal=true) and inherited origin (coninhcount=1). This is the situation that Benedikt's example has, and the problem is for pg_dump to restore this state. Now, pg_dump needs to issue separate commands along these lines to restore NOT VALID constraints, because it has to copy data into the tables before adding the constraints. (Otherwise, if any rows not satisfying the constraint are still in the table, the restore would fail.) The problem from pg_dump's viewpoint is that there's nothing particularly guaranteeing that the two ALTERs will be issued in the "right" order. Absent other considerations, it'll tend to issue the ALTERs in alphabetical order, which means dumping this example exactly as given will work, but not so much if the table names are like "foo" and "foo_child". We could possibly try to fix this by adding dependencies, but the dependencies would have to say that the parent table's constraint depends on the child table's constraint, which seems pretty weird. What seems like a saner answer to me is to change the backend so that it will accept these ALTER commands in either order, with the same end state. The reason it throws an error now, IMO, is just so that blindly issuing the same ALTER ADD CONSTRAINT twice will fail. But we could deal with that by saying that it's okay as long as the initially-targeted constraint doesn't already have conislocal = true. While I was poking at this, I came across a second problem in the same area, which is that this succeeds: alter table child add constraint inh_check check(f1 > 0) not valid; alter table parent add constraint inh_check check(f1 > 0); After that, you have a parent constraint that claims to be valid and inheritable, but nonetheless there might be rows in a child table that don't meet the constraint. That is BAD. It would break planner deductions that depend on believing that check constraints hold for all rows emitted by an inherited table scan. (I'm not certain whether there are any affected cases right now, but it's certainly plausible that there might be such in future.) Whatever you think of the other situation, we need to disallow this. The attached proposed patch (sans test cases as yet) deals with both of these things, and doesn't change the results of any existing regression test cases. I'm inclined to treat this as a bug and back-patch it as far as we allow NOT VALID check constraints, which seems to be 9.2. Comments? regards, tom lane
diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c index dbd6094..ea06a57 100644 *** a/src/backend/catalog/heap.c --- b/src/backend/catalog/heap.c *************** static void StoreConstraints(Relation re *** 105,110 **** --- 105,111 ---- bool is_internal); static bool MergeWithExistingConstraint(Relation rel, char *ccname, Node *expr, bool allow_merge, bool is_local, + bool is_initially_valid, bool is_no_inherit); static void SetRelationNumChecks(Relation rel, int numchecks); static Node *cookConstraint(ParseState *pstate, *************** AddRelationNewConstraints(Relation rel, *** 2301,2306 **** --- 2302,2308 ---- */ if (MergeWithExistingConstraint(rel, ccname, expr, allow_merge, is_local, + cdef->initially_valid, cdef->is_no_inherit)) continue; } *************** AddRelationNewConstraints(Relation rel, *** 2389,2394 **** --- 2391,2397 ---- static bool MergeWithExistingConstraint(Relation rel, char *ccname, Node *expr, bool allow_merge, bool is_local, + bool is_initially_valid, bool is_no_inherit) { bool found; *************** MergeWithExistingConstraint(Relation rel *** 2436,2457 **** if (equal(expr, stringToNode(TextDatumGetCString(val)))) found = true; } if (!found || !allow_merge) ereport(ERROR, (errcode(ERRCODE_DUPLICATE_OBJECT), errmsg("constraint \"%s\" for relation \"%s\" already exists", ccname, RelationGetRelationName(rel)))); ! tup = heap_copytuple(tup); ! con = (Form_pg_constraint) GETSTRUCT(tup); ! ! /* If the constraint is "no inherit" then cannot merge */ if (con->connoinherit) ereport(ERROR, (errcode(ERRCODE_INVALID_OBJECT_DEFINITION), errmsg("constraint \"%s\" conflicts with non-inherited constraint on relation \"%s\"", ccname, RelationGetRelationName(rel)))); if (is_local) con->conislocal = true; else --- 2439,2485 ---- if (equal(expr, stringToNode(TextDatumGetCString(val)))) found = true; } + + /* + * If the existing constraint is purely inherited (no local + * definition) then interpret addition of a local constraint as a + * legal merge. This allows ALTER ADD CONSTRAINT on parent and + * child tables to be given in either order with same end state. + */ + if (is_local && !con->conislocal) + allow_merge = true; + if (!found || !allow_merge) ereport(ERROR, (errcode(ERRCODE_DUPLICATE_OBJECT), errmsg("constraint \"%s\" for relation \"%s\" already exists", ccname, RelationGetRelationName(rel)))); ! /* If the child constraint is "no inherit" then cannot merge */ if (con->connoinherit) ereport(ERROR, (errcode(ERRCODE_INVALID_OBJECT_DEFINITION), errmsg("constraint \"%s\" conflicts with non-inherited constraint on relation \"%s\"", ccname, RelationGetRelationName(rel)))); + /* + * If the child constraint is "not valid" then cannot merge with a + * valid parent constraint + */ + if (is_initially_valid && !con->convalidated) + ereport(ERROR, + (errcode(ERRCODE_INVALID_OBJECT_DEFINITION), + errmsg("constraint \"%s\" conflicts with NOT VALID constraint on relation \"%s\"", + ccname, RelationGetRelationName(rel)))); + + /* OK to update the tuple */ + ereport(NOTICE, + (errmsg("merging constraint \"%s\" with inherited definition", + ccname))); + + tup = heap_copytuple(tup); + con = (Form_pg_constraint) GETSTRUCT(tup); + if (is_local) con->conislocal = true; else *************** MergeWithExistingConstraint(Relation rel *** 2461,2470 **** Assert(is_local); con->connoinherit = true; } - /* OK to update the tuple */ - ereport(NOTICE, - (errmsg("merging constraint \"%s\" with inherited definition", - ccname))); simple_heap_update(conDesc, &tup->t_self, tup); CatalogUpdateIndexes(conDesc, tup); break; --- 2489,2494 ---- diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index d312762..f822ed9 100644 *** a/src/backend/commands/tablecmds.c --- b/src/backend/commands/tablecmds.c *************** MergeConstraintsIntoExisting(Relation ch *** 10373,10379 **** RelationGetRelationName(child_rel), NameStr(parent_con->conname)))); ! /* If the constraint is "no inherit" then cannot merge */ if (child_con->connoinherit) ereport(ERROR, (errcode(ERRCODE_INVALID_OBJECT_DEFINITION), --- 10373,10379 ---- RelationGetRelationName(child_rel), NameStr(parent_con->conname)))); ! /* If the child constraint is "no inherit" then cannot merge */ if (child_con->connoinherit) ereport(ERROR, (errcode(ERRCODE_INVALID_OBJECT_DEFINITION), *************** MergeConstraintsIntoExisting(Relation ch *** 10382,10387 **** --- 10382,10398 ---- RelationGetRelationName(child_rel)))); /* + * If the child constraint is "not valid" then cannot merge with a + * valid parent constraint + */ + if (parent_con->convalidated && !child_con->convalidated) + ereport(ERROR, + (errcode(ERRCODE_INVALID_OBJECT_DEFINITION), + errmsg("constraint \"%s\" conflicts with NOT VALID constraint on child table \"%s\"", + NameStr(child_con->conname), + RelationGetRelationName(child_rel)))); + + /* * OK, bump the child constraint's inheritance count. (If we fail * later on, this change will just roll back.) */
-- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers