On Sat, Feb 5, 2011 at 7:44 PM, Noah Misch <noah@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