On Thu, Oct 03, 2019 at 09:52:34AM -0400, Tom Lane wrote:
> FWIW, I really dislike this patch, mainly because it is based on the
> assumption (as John said) that get_relkind_objtype is used only
> in aclcheck_error calls. However it's not obvious why that should
> be true, and there certainly is no documentation suggesting that
> it needs to be true. That's mainly because get_relkind_objtype has no
> documentation period, which if you ask me is flat out unacceptable
> for a globally-exposed function. (Same comment about its wrapper
> get_object_type.)
Yes, I agree that the expectations that the caller of this function
can have are hard to guess. So we could tackle this occasion to add
more comments. I could try to come up with a better patch. Or
perhaps you have already your mind on it?
> The patch also falsifies the comment just a few lines away that
>
> /*
> * other relkinds are not supported here because they don't map to
> * OBJECT_* values
> */
>
> without doing anything about that.
That's actually what I was referring to in my previous email.
> I'm inclined to think that we should redefine the charter of
> get_relkind_objtype/get_object_type to be that they'll produce
> some OBJECT_* value for any relkind whatever, on the grounds
> that throwing an error here isn't a particularly useful behavior;
> we'd rather come out with a possibly-slightly-inaccurate generic
> message about a "table". And they need to be documented that way.
This is tempting.
> Alternatively, instead of mapping other relkinds to OBJECT_TABLE,
> we could invent a new enum entry OBJECT_RELATION. There's precedent
> for that in OBJECT_ROUTINE ... but I don't know that we want to
> build out all the other infrastructure for a new ObjectType right now.
I am too lazy to check the thread that led to 8b9e964, but I recall
that Peter wanted to get rid of OBJECT_RELATION because that's
confusing as that's not an purely exclusive object type, and it mapped
with other object types.
--
Michael