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
