On Sat, Feb 5, 2011 at 7:44 PM, Noah Misch <n...@leadboat.com> wrote:
>> But this is a little unsatisfying, for two reasons. First, the error
>> message will be subtly wrong: we can make it complain about a table or
>> a type, but not a foreign table. At a quick look, it likes the right
>> fix might be to replace the second and third arguments to
>> find_composite_type_dependencies() with a Relation.
>
> Seems like a clear improvement.
That didn't quite work, because there's a caller in typecmds.c that
doesn't have the relation handy. So I made it take a relkind and a
name, which works fine.
>> Second, I wonder
>> if we shouldn't refactor things so that all the checks fire in
>> ATRewriteTables() rather than doing them in different places. Seems
>> like that might be cleaner.
>
> Offhand, this seems reasonable, too. I assumed there was some good reason it
> couldn't be done there for non-tables, but nothing comes to mind.
Actually, thinking about this more, I'm thinking if we're going to
change anything, it seems we ought to go the other way. If we ever
actually did support recursing into wherever the composite type
dependencies take us, we'd want to detect that before phase 3 and add
work-queue items for each table that we needed to futz with.
The attached patch takes this approach. It's very slightly more code,
but it reduces the amount of spooky action at a distance. The
existing coding is basically relying on the assumption that operations
which require finding composite type dependencies also require a table
rewrite. That was probably true at one point in time, but it's not
really quite right. It already requires compensating code foreign
tables and composite types (which can require this treatment even
though there's nothing to rewrite) and your proposed changes to avoid
table rewrites in cases where a type is changed to a compatible type
would break it in the opposite direction (the check would still be
needed even if the parent table doesn't need a rewrite, because the
rowtype could be indexed in some fashion that depends on the type of
the column being changed).
Comments?
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 6a17399..83f8be0 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -3378,23 +3378,6 @@ ATRewriteTable(AlteredTableInfo *tab, Oid OIDNewHeap, LOCKMODE lockmode)
}
/*
- * If we change column data types or add/remove OIDs, the operation has to
- * be propagated to tables that use this table's rowtype as a column type.
- * newrel will also be non-NULL in the case where we're adding a column
- * with a default. We choose to forbid that case as well, since composite
- * types might eventually support defaults.
- *
- * (Eventually we'll probably need to check for composite type
- * dependencies even when we're just scanning the table without a rewrite,
- * but at the moment a composite type does not enforce any constraints,
- * so it's not necessary/appropriate to enforce them just during ALTER.)
- */
- if (newrel)
- find_composite_type_dependencies(oldrel->rd_rel->reltype,
- oldrel->rd_rel->relkind,
- RelationGetRelationName(oldrel));
-
- /*
* Generate the constraint and default execution states
*/
@@ -4055,7 +4038,7 @@ ATExecAddColumn(AlteredTableInfo *tab, Relation rel,
Oid typeOid;
int32 typmod;
Form_pg_type tform;
- Expr *defval;
+ Expr *defval = NULL;
attrdesc = heap_open(AttributeRelationId, RowExclusiveLock);
@@ -4304,6 +4287,20 @@ ATExecAddColumn(AlteredTableInfo *tab, Relation rel,
tab->new_changeoids = true;
/*
+ * If we're adding an OID column, the operation has to be propagated to
+ * tables that use this table's rowtype as a column type. But we don't
+ * currently have the infrastructure for that, so just throw an error.
+ * We also forbid the case where we're adding a column with a default, since
+ * composite types might eventually support defaults, and ALTER TABLE ..
+ * ADD COLUMN .. DEFAULT would be expected to initialize the newly added
+ * column to the default in each instantiation of the rowtype.
+ */
+ if (isOid || defval)
+ find_composite_type_dependencies(rel->rd_rel->reltype,
+ rel->rd_rel->relkind,
+ RelationGetRelationName(rel));
+
+ /*
* Add needed dependency entries for the new column.
*/
add_column_datatype_dependency(myrelid, newattnum, attribute.atttypid);
@@ -4975,6 +4972,16 @@ ATExecDropColumn(List **wqueue, Relation rel, const char *colName,
/* Tell Phase 3 to physically remove the OID column */
tab->new_changeoids = true;
+
+ /*
+ * The OID removal operation needs to be propagated to tables that use
+ * this table's rowtype as a column type. But we don't currently have
+ * the infrastructure for that, so just throw an error if it would be
+ * required.
+ */
+ find_composite_type_dependencies(rel->rd_rel->reltype,
+ rel->rd_rel->relkind,
+ RelationGetRelationName(rel));
}
}
@@ -6442,17 +6449,15 @@ ATPrepAlterColumnType(List **wqueue,
errmsg("ALTER TYPE USING is not supported on foreign tables")));
}
- if (tab->relkind == RELKIND_COMPOSITE_TYPE
- || tab->relkind == RELKIND_FOREIGN_TABLE)
- {
- /*
- * For composite types, do this check now. Tables will check
- * it later when the table is being rewritten.
- */
- find_composite_type_dependencies(rel->rd_rel->reltype,
- rel->rd_rel->relkind,
- RelationGetRelationName(rel));
- }
+ /*
+ * If we change column data types, the operation has to be propagated to
+ * tables that use this table's rowtype as a column type. But we don't
+ * currently have the infrastructure for that, so just throw an error
+ * if it would be required.
+ */
+ find_composite_type_dependencies(rel->rd_rel->reltype,
+ rel->rd_rel->relkind,
+ RelationGetRelationName(rel));
ReleaseSysCache(tuple);
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
