Re: [HACKERS] replace GrantObjectType with ObjectType

Michael, Peter, all,

* Michael Paquier (michael(dot)paquier(at)gmail(dot)com) wrote:
> On Thu, Dec 21, 2017 at 1:19 AM, Peter Eisentraut
> <peter(dot)eisentraut(at)2ndquadrant(dot)com> wrote:
> > On 12/20/17 10:37, Alvaro Herrera wrote:
> >> I think Michael's point is that instead of a "default:" clause, this
> >> switch should list all the known values of the enum and throw an
> >> "unsupported object type" error for them. So whenever somebody adds a
> >> new object type, the compiler will complain that this switch doesn't
> >> handle it and the developer will have to think about this function.
>
> Thanks Álvaro, that's exactly the point I am coming at. The previous
> version of the patch was breaking an existing flow.
>
> > Right. I was actually looking at a later patch that I had not sent in
> > yet that had already addressed that. So here it is.
>
> Thanks for the new version. I have looked at 0001, and this looks
> acceptable for me in this shape.
>
> In the set of things that could be improved, but I am of course not
> asking about those being addressed in this patch... Things could be
> made more consistent for ExecGrantStmt_oids, objectNamesToOids,
> objectsInSchemaToOids, SetDefaultACL and
> ExecAlterDefaultPrivilegesStmt for the switch/case handlings.

I looked into various bits while considering this change.

One concern I have here is that it's introducing OBJECT_RELATION as a
new object type when we already have OBJECT_TABLE, OBJECT_VIEW and
OBJECT_SEQUENCE. In some ways it makes sense- the GRANT command allows
the user to be ambiguous when it comes to the details of the object
type:

GRANT SELECT ON foo TO bar;

In this case, foo could be a table, a view, or a sequence, so we have
the grammer code is as OBJECT_RELATION and then use RangeVarGetRelOids
to get the OIDs for it, since that function doesn't much care what kind
of relation it is, and off we go.

There's some downsides to this approach though: we do an initial set of
checks in ExecGrantStmt, but we can't do all of them because we don't
know if it's a sequence or not, so we end up with some additional
special checks to see if the GRANT is valid down in ExecGrant_Relation
after we figure out what kind of relation it is.

Further, anything which handles an OBJECT_TABLE or OBJECT_VIEW or
OBJECT_SEQUENCE today might be expected to now be able to handle an
OBJECT_RELATION instead, though it doesn't look like this patch makes
any attempt to address that.

Lastly, and I'm not sure if we'd actually want to change it, but:

GRANT SELECT ON TABLE sequence1 TO role1;

works just fine even though it's not really correct. The other way:

GRANT SELECT ON SEQUENCE table1 TO role1;

doesn't work though, and neither does GRANT SELECT ON VIEW (at all).

In the end, I'd personally prefer refactoring ExecGrantStmt and friends
to move the GRANT-type checks down into the individual functions and,
ideally, avoid having to have OBJECT_RELATION at all, though that seems
like it'd be a good bit of work.

My second preference would be to at least add some commentary about
OBJECT_RELATION vs. OBJECT_(TABLE|VIEW|SEQUENCE), when which should be
used and why, and review functions that accept objects of those types
to see if they should be extended to also accept OBJECT_RELATION.

> I have not looked at 0002 in details.

Neither have I.

Thanks!

Stephen