On Sun, Feb 06, 2011 at 02:15:47AM -0500, Robert Haas wrote: > 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.
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
