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.