From: Tom Lane <tgl@sss.pgh.pa.us>
To: pgsql-hackers@postgreSQL.org
Subject: ALTER COLUMN TYPE vs. domain constraints
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="----- =_aaaaaaaaaa0"
Content-ID: <30599.1509128040.0@sss.pgh.pa.us>
Date: Fri, 27 Oct 2017 14:15:30 -0400
Message-ID: <30656.1509128130@sss.pgh.pa.us>
Precedence: bulk
Sender: pgsql-hackers-owner@postgresql.org

------- =_aaaaaaaaaa0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <30599.1509128040.1@sss.pgh.pa.us>
Content-Transfer-Encoding: quoted-printable

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=3D# create type comptype as (r float8, i float8);
CREATE TYPE
regression=3D# create domain silly as float8 check ((row(value,0)::comptype=
).r > 0);
CREATE DOMAIN
regression=3D# 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=3D# select 0::silly;
ERROR:  ROW() column has type double precision instead of type character va=
rying

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


------- =_aaaaaaaaaa0
Content-Type: text/x-diff;
	name="handle-domain-constraints-when-altering-column-type.patch";
	charset="us-ascii"
Content-ID: <30599.1509128040.2@sss.pgh.pa.us>
Content-Description: handle-domain-constraints-when-altering-column-type.patch
Content-Transfer-Encoding: quoted-printable

diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablec=
mds.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 pas=
s,
+ 							   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 =3D (Constraint *) cmd->def;
*************** ATRewriteCatalogs(List **wqueue, LOCKMOD
*** 3819,3825 ****
  			rel =3D relation_open(tab->relid, NoLock);
=20=20
  			foreach(lcmd, subcmds)
! 				ATExecCmd(wqueue, tab, rel, (AlterTableCmd *) lfirst(lcmd), lockmode);
=20=20
  			/*
  			 * After the ALTER TYPE pass, do cleanup work (this is not done in
--- 3822,3830 ----
  			rel =3D relation_open(tab->relid, NoLock);
=20=20
  			foreach(lcmd, subcmds)
! 				ATExecCmd(wqueue, tab, rel,
! 						  castNode(AlterTableCmd, lfirst(lcmd)),
! 						  lockmode);
=20=20
  			/*
  			 * 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 =3D
+ 				AlterDomainAddConstraint(((AlterDomainStmt *) cmd->def)->typeName,
+ 										 ((AlterDomainStmt *) cmd->def)->def,
+ 										 NULL);
+ 			break;
  		case AT_ReAddComment:	/* Re-add existing comment */
  			address =3D 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 =3D (Form_pg_constraint) GETSTRUCT(tup);
! 		relid =3D con->conrelid;
  		confrelid =3D con->confrelid;
  		conislocal =3D con->conislocal;
  		ReleaseSysCache(tup);
--- 9628,9642 ----
  		if (!HeapTupleIsValid(tup)) /* should not happen */
  			elog(ERROR, "cache lookup failed for constraint %u", oldId);
  		con =3D (Form_pg_constraint) GETSTRUCT(tup);
! 		if (OidIsValid(con->conrelid))
! 			relid =3D con->conrelid;
! 		else
! 		{
! 			/* must be a domain constraint */
! 			relid =3D get_typ_typrelid(getBaseType(con->contypid));
! 			if (!OidIsValid(relid))
! 				elog(ERROR, "could not identify relation associated with constraint %=
u", oldId);
! 		}
  		confrelid =3D con->confrelid;
  		conislocal =3D con->conislocal;
  		ReleaseSysCache(tup);
*************** ATPostAlterTypeParse(Oid oldId, Oid oldR
*** 9753,9759 ****
=20=20
  			foreach(lcmd, stmt->cmds)
  			{
! 				AlterTableCmd *cmd =3D (AlterTableCmd *) lfirst(lcmd);
=20=20
  				if (cmd->subtype =3D=3D AT_AddIndex)
  				{
--- 9773,9779 ----
=20=20
  			foreach(lcmd, stmt->cmds)
  			{
! 				AlterTableCmd *cmd =3D castNode(AlterTableCmd, lfirst(lcmd));
=20=20
  				if (cmd->subtype =3D=3D AT_AddIndex)
  				{
*************** ATPostAlterTypeParse(Oid oldId, Oid oldR
*** 9781,9789 ****
  				}
  				else if (cmd->subtype =3D=3D AT_AddConstraint)
  				{
! 					Constraint *con;
=20=20
- 					con =3D castNode(Constraint, cmd->def);
  					con->old_pktable_oid =3D refRelId;
  					/* rewriting neither side of a FK */
  					if (con->contype =3D=3D CONSTR_FOREIGN &&
--- 9801,9808 ----
  				}
  				else if (cmd->subtype =3D=3D AT_AddConstraint)
  				{
! 					Constraint *con =3D castNode(Constraint, cmd->def);
=20=20
  					con->old_pktable_oid =3D refRelId;
  					/* rewriting neither side of a FK */
  					if (con->contype =3D=3D CONSTR_FOREIGN &&
*************** ATPostAlterTypeParse(Oid oldId, Oid oldR
*** 9804,9809 ****
--- 9823,9853 ----
  						 (int) cmd->subtype);
  			}
  		}
+ 		else if (IsA(stm, AlterDomainStmt))
+ 		{
+ 			AlterDomainStmt *stmt =3D (AlterDomainStmt *) stm;
+=20
+ 			if (stmt->subtype =3D=3D 'C')	/* ADD CONSTRAINT */
+ 			{
+ 				Constraint *con =3D castNode(Constraint, stmt->def);
+ 				AlterTableCmd *cmd =3D makeNode(AlterTableCmd);
+=20
+ 				cmd->subtype =3D AT_ReAddDomainConstraint;
+ 				cmd->def =3D (Node *) stmt;
+ 				tab->subcmds[AT_PASS_OLD_CONSTR] =3D
+ 					lappend(tab->subcmds[AT_PASS_OLD_CONSTR], cmd);
+=20
+ 				/* 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 ----
  }
=20=20
  /*
+  * 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;
+=20
+ 	/* Look for comment for object wanted, and leave if none */
+ 	comment_str =3D GetComment(objid, ConstraintRelationId, 0);
+ 	if (comment_str =3D=3D NULL)
+ 		return;
+=20
+ 	/* Build node CommentStmt */
+ 	cmd =3D makeNode(CommentStmt);
+ 	cmd->objtype =3D OBJECT_DOMCONSTRAINT;
+ 	cmd->object =3D (Node *) list_make2(makeTypeNameFromNameList(domname),
+ 									  makeString(pstrdup(conname)));
+ 	cmd->comment =3D comment_str;
+=20
+ 	/* Append it to list of commands */
+ 	newcmd =3D makeNode(AlterTableCmd);
+ 	newcmd->subtype =3D AT_ReAddComment;
+ 	newcmd->def =3D (Node *) cmd;
+ 	tab->subcmds[pass] =3D lappend(tab->subcmds[pass], newcmd);
+ }
+=20
+ /*
   * Subroutine for ATPostAlterTypeParse().  Calls out to CheckIndexCompati=
ble()
   * for the real analysis, then mutates the IndexStmt based on that verdic=
t.
   */
diff --git a/src/backend/utils/adt/ruleutils.c b/src/backend/utils/adt/rule=
utils.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=20
*** 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);
=20=20
*************** pg_get_constraintdef_worker(Oid constrai
*** 1867,1881 ****
=20=20
  	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)));
  	}
=20=20
  	switch (conForm->contype)
--- 1868,1894 ----
=20=20
  	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)));
! 		}
  	}
=20=20
  	switch (conForm->contype)
*************** generate_operator_name(Oid operid, Oid a
*** 10779,10784 ****
--- 10792,10833 ----
  }
=20=20
  /*
+  * 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;
+=20
+ 	tp =3D SearchSysCache1(TYPEOID, ObjectIdGetDatum(typid));
+ 	if (!HeapTupleIsValid(tp))
+ 		elog(ERROR, "cache lookup failed for type %u", typid);
+ 	typtup =3D (Form_pg_type) GETSTRUCT(tp);
+ 	typname =3D NameStr(typtup->typname);
+=20
+ 	nspname =3D get_namespace_name(typtup->typnamespace);
+ 	if (!nspname)
+ 		elog(ERROR, "cache lookup failed for namespace %u",
+ 			 typtup->typnamespace);
+=20
+ 	result =3D quote_qualified_identifier(nspname, typname);
+=20
+ 	ReleaseSysCache(tp);
+=20
+ 	return result;
+ }
+=20
+ /*
   * 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/expect=
ed/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 n=
eed 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 object=
s 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;
=20=20
=20=20
+ -- 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';
+=20
+ select row(0,1)::dcomptype;  -- fail
+=20
+ alter type comptype alter attribute r type varchar;  -- fail
+ alter type comptype alter attribute r type bigint;
+=20
+ alter type comptype drop attribute r;  -- fail
+ alter type comptype drop attribute i;
+=20
+ drop type comptype cascade;
+=20
+=20
  -- Test domains over arrays of composite
=20=20
  create type comptype as (r float8, i float8);

------- =_aaaaaaaaaa0
Content-Type: text/plain
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0


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

------- =_aaaaaaaaaa0--