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);