Re: ALTER TYPE 2: skip already-provable no-work rewrites

On Sat, Feb 05, 2011 at 10:03:59AM -0500, Robert Haas wrote:
> Looking at this still more, it appears that independent of any change
> this patch may wish to make, there's a live bug here related to the
> foreign table patch I committed back in December. Creating a foreign
> table creates an eponymous rowtype, which can be used as a column in a
> regular table. You can then change the data type of a column in the
> foreign table, read from the regular table, and crash the server.
>
> The simple fix for this is to just change the code in
> ATPrepAlterColumnType to handle the foreign table case also:
>
> if (tab->relkind == RELKIND_COMPOSITE_TYPE)
> {
> /*
> * 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,
> NULL,
> RelationGetRelationName(rel));
> }
>
> 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.

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