On Sun, Feb 06, 2011 at 02:15:47AM -0500, Robert Haas wrote:
> 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.
Hmm, indeed. In get_rels_with_domain(), it's a scalar type.
> >> 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.
> Comments?
Your patch improves the code. My standard for commending a refactor-only patch
is rather high, though, and this patch doesn't reach it. The ancestral code
placement wasn't obviously correct, but neither is this. So I'd vote -0.
nm