On Sun, Feb 06, 2011 at 07:54:52AM -0500, Robert Haas wrote:
> On Sun, Feb 6, 2011 at 4:15 AM, Noah Misch <noah@leadboat.com> wrote:
> >> 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.
>
> Yeeeeeeah, that's actually a little ugly. It's actually a domain
> over a composite type, not a composite type proper, IIUC. Better
> ideas?
There are no domains over composite types. get_rels_with_domain() finds all
relations having columns of the (scalar) domain type. It then calls
find_composite_type_dependencies to identify uses of the composite types
discovered in the previous step.
Honestly, RELKIND_COMPOSITE_TYPE is a reasonable choice despite the technical
mismatch. One more-correct approach would be to have two arguments, a catalog
OID (pg_type or pg_class, currently) and a relkind, 0 when the catalog OID !=
pg_class. Might be an improvement, albeit a minor one.
> >> 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.
>
> Well, what's your suggestion, then? Your patch pops the test up from
> ATRewriteTable() up to ATRewriteTables(), but that's not obviously
> correct either, and it's an awkward place to do it because we don't
> have the Relation object handy at the point where the check needs to
> be done.
Agreed. I couldn't think of any grand improvements, so my patch bore the
conceptually-smallest change I could come up with to handle that particular
issue. That is, I felt it was the change that could most easily be verified as
rejecting the same cases as the old test.
nm