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