Michael Paquier <mich...@paquier.xyz> writes:
> On Tue, Oct 01, 2019 at 12:10:50AM +0000, Hsu, John wrote:
>> get_relkind_objtype(...) was introduced as part of 8b9e9644dc, and it
>> doesn't include
>> RELKIND_TOASTVALUE. As a result when a user who has usage rights on schema
>> pg_toast
>> and attempts to reindex a table it is not the owner of it fails with the
>> wrong error
>> message.
> (Adding Peter E. in CC)
> Sure. However this implies that the user doing the reindex not only
> has ownership of the relation worked on, but is also able to work
> directly on the schema pg_toast. Should we really encourage people to
> do that with non-superusers?
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.)
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.
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.
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.
regards, tom lane
