Re: [HACKERS] replace GrantObjectType with ObjectType

On 1/16/18 23:38, Michael Paquier wrote:
> It seems to me that you could just use rel->rd_rel->relkind instead of
> get_rel_relkind(RelationGetRelid(rel)).

right

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

Yeah, the ordering is a bit nonsensical at the moment.

> + 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...

The problem I'm trying to solve is that keeping OBJECT_RELATION anywhere
means it has to be handled everywhere. This is the only place where
it's interesting, but it's only used to produce some error messages, so
I think it doesn't have to be terribly efficient and elegant.

There is actually a set of related problems:

=> alter procedure test() rename to test1;
ERROR: must be owner of function test

The way this code is structured, it gets the object type from the system
catalog it is dealing with. But some system catalogs can contain
multiple types of objects, e.g., pg_proc, pg_class, arguably pg_type.
So we need some "post-processing" to differentiate these better.

> 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.

sure

> +-- 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?

I just copied this from alter_generic.sql. I'm not sure about it either.

--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services