Include RELKIND_TOASTVALUE in get_relkind_objtype

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?

It seems get_relkind_objtype(...) is only used as part of aclcheck_error(...)
I've attached a patch to include RELKIND_TOASTVALUE in get_relkind_objtype.
Now it fails with the proper error message.

postgres=> \c
You are now connected to database "postgres" as user "testuser".
postgres=> REINDEX TABLE pg_toast.pg_toast_16388;
ERROR: must be owner of table pg_toast_16388

Here is a set of commands to see the failure:
=# CREATE ROLE testuser LOGIN;
=# GRANT USAGE ON SCHEMA pg_toast TO testuser;
\c postgres testuser
=> REINDEX TABLE pg_toast.pg_toast_2609;
ERROR: XX000: unexpected relkind: 116
=> REINDEX INDEX pg_toast.pg_toast_2609_index;
ERROR: 42501: must be owner of index pg_toast_2609_index
LOCATION: aclcheck_error, aclchk.c:3623

As you wrote, get_relkind_objtype() is primarily used for ACL errors,
but we have another set of code paths with get_object_type() which
gets called for a subset of ALTER TABLE commands. So this error can
be triggered in more ways, though you had better not do the following
one:
=# ALTER TABLE pg_toast.pg_toast_1260 rename to popo;
ERROR: XX000: unexpected relkind: 116

The comment about OBJECT_* in get_relkind_objtype() is here because
there is no need for toast objects to have object address support
(there is a test in object_address.sql about that), and ObjectTypeMap
has no mapping OBJECT_* <-> toast table, so the change proposed is not
correct from this perspective.
--
Michael

Michael Paquier <michael@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

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

On Fri, Oct 04, 2019 at 05:55:40PM +0900, Michael Paquier wrote:

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?

Okay. Attached is what I was thinking about, with extra regression
tests to cover the ground for toast tables and indexes that are able
to reproduce the original failure, and more comments for the routines
as they should be used only for ACL error messages.

Any thoughts?
--
Michael

Michael Paquier <michael@paquier.xyz> writes:

Okay. Attached is what I was thinking about, with extra regression
tests to cover the ground for toast tables and indexes that are able
to reproduce the original failure, and more comments for the routines
as they should be used only for ACL error messages.

I'd rather do something like the attached, which makes it more of an
explicit goal that we won't fail on bad input. (As written, we'd only
fail on bad classId, which is a case that really shouldn't happen.)

Tests are the same as yours, but I revised the commentary and got
rid of the elog-for-bad-relkind. I also made some cosmetic changes
in commands/alter.c, so as to (1) make it clear by inspection that
those calls are only used to feed aclcheck_error, and (2) avoid
uselessly computing a value that we won't need in normal non-error
cases.

regards, tom lane

On Mon, Nov 04, 2019 at 03:31:27PM -0500, Tom Lane wrote:

I'd rather do something like the attached, which makes it more of an
explicit goal that we won't fail on bad input. (As written, we'd only
fail on bad classId, which is a case that really shouldn't happen.)

Okay, that part looks fine.

Tests are the same as yours, but I revised the commentary and got
rid of the elog-for-bad-relkind.

No objections on that part either.

I also made some cosmetic changes in commands/alter.c, so as to (1)
make it clear by inspection that those calls are only used to feed
aclcheck_error, and (2) avoid uselessly computing a value that we
won't need in normal non-error cases.

Makes also sense. Thanks for looking at it!
--
Michael

Michael Paquier <michael@paquier.xyz> writes:

On Mon, Nov 04, 2019 at 03:31:27PM -0500, Tom Lane wrote:

I'd rather do something like the attached, which makes it more of an
explicit goal that we won't fail on bad input. (As written, we'd only
fail on bad classId, which is a case that really shouldn't happen.)

Okay, that part looks fine.

Pushed like that, then.

regards, tom lane

On Tue, Nov 05, 2019 at 01:41:28PM -0500, Tom Lane wrote:

Pushed like that, then.

Thanks for the commit.
--
Michael