[HACKERS] ALTER COLUMN TYPE vs. domain constraints

I found out that altering a column's type does not play nicely with
domain constraints: tablecmds.c expects that only table constraints
could depend on a column.  Now, it's easy to hit that with domains
over composite, so I propose to fix it in HEAD with the attached
patch.  However, if you really work at it, you can make it fail
in the back branches too:

regression=# create type comptype as (r float8, i float8);
CREATE TYPE
regression=# create domain silly as float8 check ((row(value,0)::comptype).r > 0);
CREATE DOMAIN
regression=# alter type comptype alter attribute r type varchar;
ERROR:  cache lookup failed for relation 0

Before commit 6784d7a1d, the ALTER actually went through, leaving a
mess.  Fortunately it doesn't actually crash afterwards, but you
get things like

regression=# select 0::silly;
ERROR:  ROW() column has type double precision instead of type character varying

We could consider back-patching the attached to cover this, but
I'm not entirely sure it's worth the trouble, because I haven't
thought of any non-silly use-cases in the absence of domains
over composite.  Comments?

            regards, tom lane

diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 3ab8087..fc93c4e 100644
*** a/src/backend/commands/tablecmds.c
--- b/src/backend/commands/tablecmds.c
*************** static void ATPostAlterTypeParse(Oid old
*** 426,431 ****
--- 426,433 ----
                       bool rewrite);
  static void RebuildConstraintComment(AlteredTableInfo *tab, int pass,
                           Oid objid, Relation rel, char *conname);
+ static void RebuildDomainConstraintComment(AlteredTableInfo *tab, int pass,
+                                Oid objid, 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,
*************** AlterTableGetLockLevel(List *cmds)
*** 3319,3324 ****
--- 3321,3327 ----
              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;
*************** ATRewriteCatalogs(List **wqueue, LOCKMOD
*** 3819,3825 ****
              rel = relation_open(tab->relid, NoLock);

              foreach(lcmd, subcmds)
!                 ATExecCmd(wqueue, tab, rel, (AlterTableCmd *) lfirst(lcmd), lockmode);

              /*
               * After the ALTER TYPE pass, do cleanup work (this is not done in
--- 3822,3830 ----
              rel = relation_open(tab->relid, NoLock);

              foreach(lcmd, subcmds)
!                 ATExecCmd(wqueue, tab, rel,
!                           castNode(AlterTableCmd, lfirst(lcmd)),
!                           lockmode);

              /*
               * After the ALTER TYPE pass, do cleanup work (this is not done in
*************** ATExecCmd(List **wqueue, AlteredTableInf
*** 3936,3941 ****
--- 3941,3953 ----
                  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;
*************** ATPostAlterTypeCleanup(List **wqueue, Al
*** 9616,9622 ****
          if (!HeapTupleIsValid(tup)) /* should not happen */
              elog(ERROR, "cache lookup failed for constraint %u", oldId);
          con = (Form_pg_constraint) GETSTRUCT(tup);
!         relid = con->conrelid;
          confrelid = con->confrelid;
          conislocal = con->conislocal;
          ReleaseSysCache(tup);
--- 9628,9642 ----
          if (!HeapTupleIsValid(tup)) /* should not happen */
              elog(ERROR, "cache lookup failed for constraint %u", oldId);
          con = (Form_pg_constraint) GETSTRUCT(tup);
!         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);
*************** ATPostAlterTypeParse(Oid oldId, Oid oldR
*** 9753,9759 ****

              foreach(lcmd, stmt->cmds)
              {
!                 AlterTableCmd *cmd = (AlterTableCmd *) lfirst(lcmd);

                  if (cmd->subtype == AT_AddIndex)
                  {
--- 9773,9779 ----

              foreach(lcmd, stmt->cmds)
              {
!                 AlterTableCmd *cmd = castNode(AlterTableCmd, lfirst(lcmd));

                  if (cmd->subtype == AT_AddIndex)
                  {
*************** ATPostAlterTypeParse(Oid oldId, Oid oldR
*** 9781,9789 ****
                  }
                  else if (cmd->subtype == AT_AddConstraint)
                  {
!                     Constraint *con;

-                     con = castNode(Constraint, cmd->def);
                      con->old_pktable_oid = refRelId;
                      /* rewriting neither side of a FK */
                      if (con->contype == CONSTR_FOREIGN &&
--- 9801,9808 ----
                  }
                  else if (cmd->subtype == AT_AddConstraint)
                  {
!                     Constraint *con = castNode(Constraint, cmd->def);

                      con->old_pktable_oid = refRelId;
                      /* rewriting neither side of a FK */
                      if (con->contype == CONSTR_FOREIGN &&
*************** ATPostAlterTypeParse(Oid oldId, Oid oldR
*** 9804,9809 ****
--- 9823,9853 ----
                           (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 */
+                 RebuildDomainConstraintComment(tab,
+                                                AT_PASS_OLD_CONSTR,
+                                                oldId,
+                                                stmt->typeName,
+                                                con->conname);
+             }
+             else
+                 elog(ERROR, "unexpected statement subtype: %d",
+                      (int) stmt->subtype);
+         }
          else
              elog(ERROR, "unexpected statement type: %d",
                   (int) nodeTag(stm));
*************** RebuildConstraintComment(AlteredTableInf
*** 9845,9850 ****
--- 9889,9925 ----
  }

  /*
+  * Subroutine for ATPostAlterTypeParse() to recreate a comment entry for
+  * a domain constraint that is being re-added.
+  */
+ static void
+ RebuildDomainConstraintComment(AlteredTableInfo *tab, int pass, Oid objid,
+                                List *domname, char *conname)
+ {
+     CommentStmt *cmd;
+     char       *comment_str;
+     AlterTableCmd *newcmd;
+
+     /* Look for comment for object wanted, and leave if none */
+     comment_str = GetComment(objid, ConstraintRelationId, 0);
+     if (comment_str == NULL)
+         return;
+
+     /* Build node CommentStmt */
+     cmd = makeNode(CommentStmt);
+     cmd->objtype = OBJECT_DOMCONSTRAINT;
+     cmd->object = (Node *) list_make2(makeTypeNameFromNameList(domname),
+                                       makeString(pstrdup(conname)));
+     cmd->comment = comment_str;
+
+     /* Append it to list of commands */
+     newcmd = makeNode(AlterTableCmd);
+     newcmd->subtype = AT_ReAddComment;
+     newcmd->def = (Node *) cmd;
+     tab->subcmds[pass] = lappend(tab->subcmds[pass], newcmd);
+ }
+
+ /*
   * Subroutine for ATPostAlterTypeParse().  Calls out to CheckIndexCompatible()
   * for the real analysis, then mutates the IndexStmt based on that verdict.
   */
diff --git a/src/backend/utils/adt/ruleutils.c b/src/backend/utils/adt/ruleutils.c
index b1e70a0..cc6cec7 100644
*** a/src/backend/utils/adt/ruleutils.c
--- b/src/backend/utils/adt/ruleutils.c
*************** static char *generate_function_name(Oid
*** 460,465 ****
--- 460,466 ----
                         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);

*************** pg_get_constraintdef_worker(Oid constrai
*** 1867,1881 ****

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

      switch (conForm->contype)
--- 1868,1894 ----

      if (fullCommand)
      {
!         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)
*************** generate_operator_name(Oid operid, Oid a
*** 10779,10784 ****
--- 10792,10833 ----
  }

  /*
+  * 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
   *
diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h
index 732e5d6..06a2b81 100644
*** a/src/include/nodes/parsenodes.h
--- b/src/include/nodes/parsenodes.h
*************** typedef enum AlterTableType
*** 1713,1718 ****
--- 1713,1719 ----
      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 */
diff --git a/src/test/regress/expected/domain.out b/src/test/regress/expected/domain.out
index f7f3948..0b3945f 100644
*** a/src/test/regress/expected/domain.out
--- b/src/test/regress/expected/domain.out
*************** Rules:
*** 286,291 ****
--- 286,309 ----
  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;
+ drop type comptype cascade;
+ NOTICE:  drop cascades to type dcomptype
  -- Test domains over arrays of composite
  create type comptype as (r float8, i float8);
  create domain dcomptypea as comptype[];
diff --git a/src/test/regress/sql/domain.sql b/src/test/regress/sql/domain.sql
index 5201f00..a861cbd 100644
*** a/src/test/regress/sql/domain.sql
--- b/src/test/regress/sql/domain.sql
*************** drop table dcomptable;
*** 159,164 ****
--- 159,181 ----
  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;
+
+ drop type comptype cascade;
+
+
  -- Test domains over arrays of composite

  create type comptype as (r float8, i float8);

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers