Fix ALTER TABLE code to update domain constraints when needed.
authorTom Lane <tgl@sss.pgh.pa.us>
Wed, 1 Nov 2017 17:32:23 +0000 (13:32 -0400)
committerTom Lane <tgl@sss.pgh.pa.us>
Wed, 1 Nov 2017 17:32:23 +0000 (13:32 -0400)
It's possible for dropping a column, or altering its type, to require
changes in domain CHECK constraint expressions; but the code was
previously only expecting to find dependent table CHECK constraints.
Make the necessary adjustments.

This is a fairly old oversight, but it's a lot easier to encounter
the problem in the context of domains over composite types than it
was before.  Given the lack of field complaints, I'm not going to
bother with a back-patch, though I'd be willing to reconsider that
decision if someone does complain.

Patch by me, reviewed by Michael Paquier

Discussion: https://postgr.es/m/30656.1509128130@sss.pgh.pa.us

src/backend/commands/tablecmds.c
src/backend/utils/adt/ruleutils.c
src/include/nodes/parsenodes.h
src/test/regress/expected/domain.out
src/test/regress/sql/domain.sql

index 3ab808715b9e12b6e7a07c79f67f4cae840573b7..c902293741c96710f27873d167c92f9c6f0650aa 100644 (file)
@@ -425,7 +425,8 @@ static void ATPostAlterTypeParse(Oid oldId, Oid oldRelId, Oid refRelId,
                     char *cmd, List **wqueue, LOCKMODE lockmode,
                     bool rewrite);
 static void RebuildConstraintComment(AlteredTableInfo *tab, int pass,
-                        Oid objid, Relation rel, char *conname);
+                        Oid objid, Relation rel, List *domname,
+                        char *conname);
 static void TryReuseIndex(Oid oldId, IndexStmt *stmt);
 static void TryReuseForeignKey(Oid oldId, Constraint *con);
 static void change_owner_fix_column_acls(Oid relationOid,
@@ -3319,6 +3320,7 @@ AlterTableGetLockLevel(List *cmds)
            case AT_ProcessedConstraint:    /* becomes AT_AddConstraint */
            case AT_AddConstraintRecurse:   /* becomes AT_AddConstraint */
            case AT_ReAddConstraint:    /* becomes AT_AddConstraint */
+           case AT_ReAddDomainConstraint:  /* becomes AT_AddConstraint */
                if (IsA(cmd->def, Constraint))
                {
                    Constraint *con = (Constraint *) cmd->def;
@@ -3819,7 +3821,9 @@ ATRewriteCatalogs(List **wqueue, LOCKMODE lockmode)
            rel = relation_open(tab->relid, NoLock);
 
            foreach(lcmd, subcmds)
-               ATExecCmd(wqueue, tab, rel, (AlterTableCmd *) lfirst(lcmd), lockmode);
+               ATExecCmd(wqueue, tab, rel,
+                         castNode(AlterTableCmd, lfirst(lcmd)),
+                         lockmode);
 
            /*
             * After the ALTER TYPE pass, do cleanup work (this is not done in
@@ -3936,6 +3940,13 @@ ATExecCmd(List **wqueue, AlteredTableInfo *tab, Relation rel,
                ATExecAddConstraint(wqueue, tab, rel, (Constraint *) cmd->def,
                                    true, true, lockmode);
            break;
+       case AT_ReAddDomainConstraint:  /* Re-add pre-existing domain check
+                                        * constraint */
+           address =
+               AlterDomainAddConstraint(((AlterDomainStmt *) cmd->def)->typeName,
+                                        ((AlterDomainStmt *) cmd->def)->def,
+                                        NULL);
+           break;
        case AT_ReAddComment:   /* Re-add existing comment */
            address = CommentObject((CommentStmt *) cmd->def);
            break;
@@ -9616,7 +9627,15 @@ ATPostAlterTypeCleanup(List **wqueue, AlteredTableInfo *tab, LOCKMODE lockmode)
        if (!HeapTupleIsValid(tup)) /* should not happen */
            elog(ERROR, "cache lookup failed for constraint %u", oldId);
        con = (Form_pg_constraint) GETSTRUCT(tup);
-       relid = con->conrelid;
+       if (OidIsValid(con->conrelid))
+           relid = con->conrelid;
+       else
+       {
+           /* must be a domain constraint */
+           relid = get_typ_typrelid(getBaseType(con->contypid));
+           if (!OidIsValid(relid))
+               elog(ERROR, "could not identify relation associated with constraint %u", oldId);
+       }
        confrelid = con->confrelid;
        conislocal = con->conislocal;
        ReleaseSysCache(tup);
@@ -9753,7 +9772,7 @@ ATPostAlterTypeParse(Oid oldId, Oid oldRelId, Oid refRelId, char *cmd,
 
            foreach(lcmd, stmt->cmds)
            {
-               AlterTableCmd *cmd = (AlterTableCmd *) lfirst(lcmd);
+               AlterTableCmd *cmd = castNode(AlterTableCmd, lfirst(lcmd));
 
                if (cmd->subtype == AT_AddIndex)
                {
@@ -9777,13 +9796,14 @@ ATPostAlterTypeParse(Oid oldId, Oid oldRelId, Oid refRelId, char *cmd,
                    RebuildConstraintComment(tab,
                                             AT_PASS_OLD_INDEX,
                                             oldId,
-                                            rel, indstmt->idxname);
+                                            rel,
+                                            NIL,
+                                            indstmt->idxname);
                }
                else if (cmd->subtype == AT_AddConstraint)
                {
-                   Constraint *con;
+                   Constraint *con = castNode(Constraint, cmd->def);
 
-                   con = castNode(Constraint, cmd->def);
                    con->old_pktable_oid = refRelId;
                    /* rewriting neither side of a FK */
                    if (con->contype == CONSTR_FOREIGN &&
@@ -9797,13 +9817,41 @@ ATPostAlterTypeParse(Oid oldId, Oid oldRelId, Oid refRelId, char *cmd,
                    RebuildConstraintComment(tab,
                                             AT_PASS_OLD_CONSTR,
                                             oldId,
-                                            rel, con->conname);
+                                            rel,
+                                            NIL,
+                                            con->conname);
                }
                else
                    elog(ERROR, "unexpected statement subtype: %d",
                         (int) cmd->subtype);
            }
        }
+       else if (IsA(stm, AlterDomainStmt))
+       {
+           AlterDomainStmt *stmt = (AlterDomainStmt *) stm;
+
+           if (stmt->subtype == 'C')   /* ADD CONSTRAINT */
+           {
+               Constraint *con = castNode(Constraint, stmt->def);
+               AlterTableCmd *cmd = makeNode(AlterTableCmd);
+
+               cmd->subtype = AT_ReAddDomainConstraint;
+               cmd->def = (Node *) stmt;
+               tab->subcmds[AT_PASS_OLD_CONSTR] =
+                   lappend(tab->subcmds[AT_PASS_OLD_CONSTR], cmd);
+
+               /* recreate any comment on the constraint */
+               RebuildConstraintComment(tab,
+                                        AT_PASS_OLD_CONSTR,
+                                        oldId,
+                                        NULL,
+                                        stmt->typeName,
+                                        con->conname);
+           }
+           else
+               elog(ERROR, "unexpected statement subtype: %d",
+                    (int) stmt->subtype);
+       }
        else
            elog(ERROR, "unexpected statement type: %d",
                 (int) nodeTag(stm));
@@ -9813,12 +9861,19 @@ ATPostAlterTypeParse(Oid oldId, Oid oldRelId, Oid refRelId, char *cmd,
 }
 
 /*
- * Subroutine for ATPostAlterTypeParse() to recreate a comment entry for
- * a constraint that is being re-added.
+ * Subroutine for ATPostAlterTypeParse() to recreate any existing comment
+ * for a table or domain constraint that is being rebuilt.
+ *
+ * objid is the OID of the constraint.
+ * Pass "rel" for a table constraint, or "domname" (domain's qualified name
+ * as a string list) for a domain constraint.
+ * (We could dig that info, as well as the conname, out of the pg_constraint
+ * entry; but callers already have them so might as well pass them.)
  */
 static void
 RebuildConstraintComment(AlteredTableInfo *tab, int pass, Oid objid,
-                        Relation rel, char *conname)
+                        Relation rel, List *domname,
+                        char *conname)
 {
    CommentStmt *cmd;
    char       *comment_str;
@@ -9829,12 +9884,23 @@ RebuildConstraintComment(AlteredTableInfo *tab, int pass, Oid objid,
    if (comment_str == NULL)
        return;
 
-   /* Build node CommentStmt */
+   /* Build CommentStmt node, copying all input data for safety */
    cmd = makeNode(CommentStmt);
-   cmd->objtype = OBJECT_TABCONSTRAINT;
-   cmd->object = (Node *) list_make3(makeString(get_namespace_name(RelationGetNamespace(rel))),
-                                     makeString(pstrdup(RelationGetRelationName(rel))),
-                                     makeString(pstrdup(conname)));
+   if (rel)
+   {
+       cmd->objtype = OBJECT_TABCONSTRAINT;
+       cmd->object = (Node *)
+           list_make3(makeString(get_namespace_name(RelationGetNamespace(rel))),
+                      makeString(pstrdup(RelationGetRelationName(rel))),
+                      makeString(pstrdup(conname)));
+   }
+   else
+   {
+       cmd->objtype = OBJECT_DOMCONSTRAINT;
+       cmd->object = (Node *)
+           list_make2(makeTypeNameFromNameList(copyObject(domname)),
+                      makeString(pstrdup(conname)));
+   }
    cmd->comment = comment_str;
 
    /* Append it to list of commands */
index b1e70a0d19ef06b0cb2ef77b14b584a8be7b248e..cc6cec7877dafdd32cde57b28391f193b3ead2a8 100644 (file)
@@ -460,6 +460,7 @@ static char *generate_function_name(Oid funcid, int nargs,
                       bool has_variadic, bool *use_variadic_p,
                       ParseExprKind special_exprkind);
 static char *generate_operator_name(Oid operid, Oid arg1, Oid arg2);
+static char *generate_qualified_type_name(Oid typid);
 static text *string_to_text(char *str);
 static char *flatten_reloptions(Oid relid);
 
@@ -1867,15 +1868,27 @@ pg_get_constraintdef_worker(Oid constraintId, bool fullCommand,
 
    if (fullCommand)
    {
-       /*
-        * Currently, callers want ALTER TABLE (without ONLY) for CHECK
-        * constraints, and other types of constraints don't inherit anyway so
-        * it doesn't matter whether we say ONLY or not.  Someday we might
-        * need to let callers specify whether to put ONLY in the command.
-        */
-       appendStringInfo(&buf, "ALTER TABLE %s ADD CONSTRAINT %s ",
-                        generate_qualified_relation_name(conForm->conrelid),
-                        quote_identifier(NameStr(conForm->conname)));
+       if (OidIsValid(conForm->conrelid))
+       {
+           /*
+            * Currently, callers want ALTER TABLE (without ONLY) for CHECK
+            * constraints, and other types of constraints don't inherit
+            * anyway so it doesn't matter whether we say ONLY or not. Someday
+            * we might need to let callers specify whether to put ONLY in the
+            * command.
+            */
+           appendStringInfo(&buf, "ALTER TABLE %s ADD CONSTRAINT %s ",
+                            generate_qualified_relation_name(conForm->conrelid),
+                            quote_identifier(NameStr(conForm->conname)));
+       }
+       else
+       {
+           /* Must be a domain constraint */
+           Assert(OidIsValid(conForm->contypid));
+           appendStringInfo(&buf, "ALTER DOMAIN %s ADD CONSTRAINT %s ",
+                            generate_qualified_type_name(conForm->contypid),
+                            quote_identifier(NameStr(conForm->conname)));
+       }
    }
 
    switch (conForm->contype)
@@ -10778,6 +10791,42 @@ generate_operator_name(Oid operid, Oid arg1, Oid arg2)
    return buf.data;
 }
 
+/*
+ * generate_qualified_type_name
+ *     Compute the name to display for a type specified by OID
+ *
+ * This is different from format_type_be() in that we unconditionally
+ * schema-qualify the name.  That also means no special syntax for
+ * SQL-standard type names ... although in current usage, this should
+ * only get used for domains, so such cases wouldn't occur anyway.
+ */
+static char *
+generate_qualified_type_name(Oid typid)
+{
+   HeapTuple   tp;
+   Form_pg_type typtup;
+   char       *typname;
+   char       *nspname;
+   char       *result;
+
+   tp = SearchSysCache1(TYPEOID, ObjectIdGetDatum(typid));
+   if (!HeapTupleIsValid(tp))
+       elog(ERROR, "cache lookup failed for type %u", typid);
+   typtup = (Form_pg_type) GETSTRUCT(tp);
+   typname = NameStr(typtup->typname);
+
+   nspname = get_namespace_name(typtup->typnamespace);
+   if (!nspname)
+       elog(ERROR, "cache lookup failed for namespace %u",
+            typtup->typnamespace);
+
+   result = quote_qualified_identifier(nspname, typname);
+
+   ReleaseSysCache(tp);
+
+   return result;
+}
+
 /*
  * generate_collation_name
  *     Compute the name to display for a collation specified by OID
index 732e5d6788334dd9606d1c16e2533d54dc444ac6..06a2b81fb5b1e344fa1ea560b555707bc0147edd 100644 (file)
@@ -1713,6 +1713,7 @@ typedef enum AlterTableType
    AT_AddConstraint,           /* add constraint */
    AT_AddConstraintRecurse,    /* internal to commands/tablecmds.c */
    AT_ReAddConstraint,         /* internal to commands/tablecmds.c */
+   AT_ReAddDomainConstraint,   /* internal to commands/tablecmds.c */
    AT_AlterConstraint,         /* alter constraint */
    AT_ValidateConstraint,      /* validate constraint */
    AT_ValidateConstraintRecurse,   /* internal to commands/tablecmds.c */
index f7f3948d431da9c5b8b403ffb3f150a10e3e84c7..f4eebb75cf2805c31b100b7c48543d3dd26da014 100644 (file)
@@ -284,6 +284,31 @@ Rules:
   WHERE (dcomptable.d1).i > 0::double precision
 
 drop table dcomptable;
+drop type comptype cascade;
+NOTICE:  drop cascades to type dcomptype
+-- check altering and dropping columns used by domain constraints
+create type comptype as (r float8, i float8);
+create domain dcomptype as comptype;
+alter domain dcomptype add constraint c1 check ((value).r > 0);
+comment on constraint c1 on domain dcomptype is 'random commentary';
+select row(0,1)::dcomptype;  -- fail
+ERROR:  value for domain dcomptype violates check constraint "c1"
+alter type comptype alter attribute r type varchar;  -- fail
+ERROR:  operator does not exist: character varying > double precision
+HINT:  No operator matches the given name and argument types. You might need to add explicit type casts.
+alter type comptype alter attribute r type bigint;
+alter type comptype drop attribute r;  -- fail
+ERROR:  cannot drop composite type comptype column r because other objects depend on it
+DETAIL:  constraint c1 depends on composite type comptype column r
+HINT:  Use DROP ... CASCADE to drop the dependent objects too.
+alter type comptype drop attribute i;
+select conname, obj_description(oid, 'pg_constraint') from pg_constraint
+  where contypid = 'dcomptype'::regtype;  -- check comment is still there
+ conname |  obj_description  
+---------+-------------------
+ c1      | random commentary
+(1 row)
+
 drop type comptype cascade;
 NOTICE:  drop cascades to type dcomptype
 -- Test domains over arrays of composite
index 5201f008a1cd7ea04106d3e03c9398cf12506c1e..68da27de22b07400ed0b5d4678dd383bf6eb0560 100644 (file)
@@ -159,6 +159,26 @@ drop table dcomptable;
 drop type comptype cascade;
 
 
+-- check altering and dropping columns used by domain constraints
+create type comptype as (r float8, i float8);
+create domain dcomptype as comptype;
+alter domain dcomptype add constraint c1 check ((value).r > 0);
+comment on constraint c1 on domain dcomptype is 'random commentary';
+
+select row(0,1)::dcomptype;  -- fail
+
+alter type comptype alter attribute r type varchar;  -- fail
+alter type comptype alter attribute r type bigint;
+
+alter type comptype drop attribute r;  -- fail
+alter type comptype drop attribute i;
+
+select conname, obj_description(oid, 'pg_constraint') from pg_constraint
+  where contypid = 'dcomptype'::regtype;  -- check comment is still there
+
+drop type comptype cascade;
+
+
 -- Test domains over arrays of composite
 
 create type comptype as (r float8, i float8);