Fixing inheritance merge behavior in ALTER TABLE ADD CONSTRAINT - Mailing list pgsql-hackers

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.)
               */