Re: [HACKERS] replace GrantObjectType with ObjectType

On Tue, Jan 16, 2018 at 04:18:29PM -0500, Peter Eisentraut wrote:
> So I was looking into how we can do it without OBJECT_RELATION. For the
> first patch, that was obviously easy, because that's what my initial
> proposal did. We just treat OBJECT_TABLE within the context of
> GRANT/REVOKE as "might also be another kind of relation".
>
> The second class replaced ACL_KIND_CLASS with OBJECT_RELATION in the
> context of aclcheck_error(), so that was harder to unwind. But I wrote
> some additional code that resolves the actual relkind and gives a more
> precise error message, e.g., about "view" or "index" instead of just
> "relation". So overall this is a net win, I think.
>
> Attached is an updated patch set. It's a bit messy because I first want
> to get confirmation on the approach we are choosing, and then I can
> smash them together in the right way. 0001 and 0002 are the original
> patches, and then 0003, 0004, 0005 undo the OBJECT_RELATION addition and
> replace them with new facilities.

Looking at 0003, which is a nice addition:

+ObjectType
+relkind_get_objtype(char relkind)
+ /* other relkinds are not supported here because they don't map to OBJECT_* values */
+ default:
+ elog(ERROR, "unexpected relkind: %d", relkind);
+ return 0;
So this concerns RELKIND_TOASTVALUE and RELKIND_COMPOSITE_TYPE. At least
a comment at the top of relkind_get_objtype?

if (!pg_class_ownercheck(RelationGetRelid(rel), GetUserId()))
- aclcheck_error(ACLCHECK_NOT_OWNER, OBJECT_RELATION,
+ aclcheck_error(ACLCHECK_NOT_OWNER, relkind_get_objtype(get_rel_relkind(RelationGetRelid(rel))),
It seems to me that you could just use rel->rd_rel->relkind instead of
get_rel_relkind(RelationGetRelid(rel)).

0004 alone fails to compile as OBJECT_RELATION is still listed in
objectaddress.c. This is corrected in 0005.

+ if (prop->objtype == OBJECT_TABLE)
+ /*
+ * If the property data says it's a table, dig a little deeper to get
+ * the real relation kind, so that callers can produce more precise
+ * error messages.
+ */
+ return relkind_get_objtype(get_rel_relkind(object_id));
I guess that this is the price to pay as OBJECT_RELATION gets
removed, but it seems to me that we want to keep the OBJECT_RELATION
layer and look in depth at the relkind if is found... By the way, I
personally favor the use of brackets in the case of a single line in an
if() if there is a comment block within the condition.

+-- Clean up in case a prior regression run failed
+SET client_min_messages TO 'warning';
+DROP ROLE IF EXISTS regress_alter_user1;
+RESET client_min_messages;
+
+CREATE USER regress_alter_user1;
Er, if you need that it seems to me that this regression test design is
wrong. I would have thought that roles should be contained within a
single file. And the role is dropped at the end of alter_table.sql as
your patch does. So perhaps something crashed during your own tests and
you added this DROP ROLE to ease things?

As a whole, I like this patch series, except that OBJECT_RELATION should
be kept for get_object_type() as this shaves a couple of cycles if an
object is marked as OBJECT_TABLE and its real relkind is OBJECT_TABLE.
--
Michael