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
signature.asc
Description: PGP signature
