Re: [HACKERS] ALTER COLUMN TYPE vs. domain constraints

[Tom Lane]

Michael Paquier  writes:
> On Fri, Oct 27, 2017 at 11:15 AM, Tom Lane  wrote:
>> 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?

> There are no real complaints about the current behavior, aren't there?
> So only patching HEAD seems enough to me.

Yeah, we can leave it till someone does complain.

> You have added a comment on the constraint to make sure that it
> remains up on basically this ALTER TYPE. Querying pg_obj_description
> would make sure that the comment on the constraint is still here.

Done.

> +RebuildDomainConstraintComment(AlteredTableInfo *tab, int pass, Oid objid,
> +  List *domname, char *conname)
> There is much duplication with RebuildConstraintComment. Why not
> grouping both under say RebuildObjectComment()?

True.  I'd originally expected the code to differ more, but we can
merge these easily enough.

> I would think about
> having cmd->objtype and cmd->object passed as arguments, and then
> remove rel and domname from the existing arguments.

Doing it like that requires the callers to do work (to prepare the object
ID data structure) that's wasted if there's no comment, which most often
there wouldn't be, I suspect.  Seems better to just pass the info the
caller does have and let this function do the rest.

Pushed, thanks for reviewing!

regards, tom lane


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

Re: [HACKERS] ALTER COLUMN TYPE vs. domain constraints

[Michael Paquier]

On Fri, Oct 27, 2017 at 11:15 AM, Tom Lane  wrote:
> 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?

There are no real complaints about the current behavior, aren't there?
So only patching HEAD seems enough to me.

+comment on constraint c1 on domain dcomptype is 'random commentary';
[...]
+alter type comptype alter attribute r type bigint;
You have added a comment on the constraint to make sure that it
remains up on basically this ALTER TYPE. Querying pg_obj_description
would make sure that the comment on the constraint is still here.

+static void
+RebuildDomainConstraintComment(AlteredTableInfo *tab, int pass, Oid objid,
+  List *domname, char *conname)
There is much duplication with RebuildConstraintComment. Why not
grouping both under say RebuildObjectComment()? I would think about
having cmd->objtype and cmd->object passed as arguments, and then
remove rel and domname from the existing arguments.

[nit]
foreach(lcmd, subcmds)
-   ATExecCmd(wqueue, tab, rel, (AlterTableCmd *)
lfirst(lcmd), lockmode);
+   ATExecCmd(wqueue, tab, rel,
+ castNode(AlterTableCmd, lfirst(lcmd)),
+ lockmode);
This does not really belong to this patch.. No objections to group things.
[/nit]
-- 
Michael


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

[HACKERS] ALTER COLUMN TYPE vs. domain constraints

[Tom Lane]

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 ==