Thread: [HACKERS] replace GrantObjectType with ObjectType
It seems to me that having ACL_OBJECT_* symbols alongside OBJECT_* symbols is not useful and leads to duplication. Digging around in the past suggests that we used to have a lot of these command-specific symbols but got rid of them over time, except that the ACL stuff was never touched. The attached patch accomplishes that. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Attachment
On Thu, Oct 12, 2017 at 7:55 AM, Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:
> It seems to me that having ACL_OBJECT_* symbols alongside OBJECT_*
> symbols is not useful and leads to duplication. Digging around in the
> past suggests that we used to have a lot of these command-specific
> symbols but got rid of them over time, except that the ACL stuff was
> never touched. The attached patch accomplishes that.
That's always welcome:14 files changed, 203 insertions(+), 254 deletions(-)
This needs an update:
$ git grep GrantObjectType
src/tools/pgindent/typedefs.list:GrantObjectType
-static const char *stringify_grantobjtype(GrantObjectType objtype);
-static const char *stringify_adefprivs_objtype(GrantObjectType objtype);
+static const char *stringify_grantobjtype(ObjectType objtype);
+static const char *stringify_adefprivs_objtype(ObjectType objtype);
stringify_grantobjtype should be renamed to stringify_objtype.
-bool
-EventTriggerSupportsGrantObjectType(GrantObjectType objtype)
-{
- switch (objtype)
- {
- case ACL_OBJECT_DATABASE:
- case ACL_OBJECT_TABLESPACE:
- /* no support for global objects */
- return false;
By removing that, if any GRANT/REVOKE support is added for another
object type, then EventTriggerSupportsObjectType would return true by
default instead of getting a compilation failure. I think that a
comment would be appropriate here: GrantStmt *stmt = (GrantStmt *) parsetree;
- if (EventTriggerSupportsGrantObjectType(stmt->objtype))
+ if (EventTriggerSupportsObjectType(stmt->objtype)) ProcessUtilitySlow(pstate, pstmt,
queryString,
Something like, "This checks for more object types than currently
supported by the GRANT statement"... Or at least something to outline
that risk.
--
Michael
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Michael Paquier wrote:
> On Thu, Oct 12, 2017 at 7:55 AM, Peter Eisentraut
> <peter.eisentraut@2ndquadrant.com> wrote:
> > It seems to me that having ACL_OBJECT_* symbols alongside OBJECT_*
> > symbols is not useful and leads to duplication. Digging around in the
> > past suggests that we used to have a lot of these command-specific
> > symbols but got rid of them over time, except that the ACL stuff was
> > never touched. The attached patch accomplishes that.
+1 for this.
> -bool
> -EventTriggerSupportsGrantObjectType(GrantObjectType objtype)
> -{
> - switch (objtype)
> - {
> - case ACL_OBJECT_DATABASE:
> - case ACL_OBJECT_TABLESPACE:
> - /* no support for global objects */
> - return false;
> By removing that, if any GRANT/REVOKE support is added for another
> object type, then EventTriggerSupportsObjectType would return true by
> default instead of getting a compilation failure.
Yeah, we've found it useful to remove default: clauses in some switch
blocks so that we get compile warnings when we forget to add a new type
(c.f. commit e84c0195980f). Let's not add any more of those.
--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Peter, * Peter Eisentraut (peter.eisentraut@2ndquadrant.com) wrote: > It seems to me that having ACL_OBJECT_* symbols alongside OBJECT_* > symbols is not useful and leads to duplication. Digging around in the > past suggests that we used to have a lot of these command-specific > symbols but got rid of them over time, except that the ACL stuff was > never touched. The attached patch accomplishes that. I'm generally supportive of this, but I'm not entirely thrilled with how this ends up conflating TABLEs and RELATIONs. From the GRANT perspective, there's no distinction, and that was clear from the language used and how things were handled, but the OBJECT enum has that distinction. This change just makes VIEWs be OBJECT_TABLE even though they actually aren't tables and there even is an OBJECT_VIEW value. This commit may be able to grok that and manage it properly, but later hackers might miss that. I would also suggest that the naming be consistent with the other bits of the GRANT system (eg: ACL_ALL_RIGHTS_NAMESPACE would be changed to ACL_ALL_RIGHTS_SCHEMA, to match OBJECT_SCHEMA). I also echo the concern raised by Alvaro. Thanks! Stephen
On Thu, Oct 12, 2017 at 5:43 PM, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> Michael Paquier wrote:
>> On Thu, Oct 12, 2017 at 7:55 AM, Peter Eisentraut
>> <peter.eisentraut@2ndquadrant.com> wrote:
>> > It seems to me that having ACL_OBJECT_* symbols alongside OBJECT_*
>> > symbols is not useful and leads to duplication. Digging around in the
>> > past suggests that we used to have a lot of these command-specific
>> > symbols but got rid of them over time, except that the ACL stuff was
>> > never touched. The attached patch accomplishes that.
>
> +1 for this.
>
>> -bool
>> -EventTriggerSupportsGrantObjectType(GrantObjectType objtype)
>> -{
>> - switch (objtype)
>> - {
>> - case ACL_OBJECT_DATABASE:
>> - case ACL_OBJECT_TABLESPACE:
>> - /* no support for global objects */
>> - return false;
>> By removing that, if any GRANT/REVOKE support is added for another
>> object type, then EventTriggerSupportsObjectType would return true by
>> default instead of getting a compilation failure.
>
> Yeah, we've found it useful to remove default: clauses in some switch
> blocks so that we get compile warnings when we forget to add a new type
> (c.f. commit e84c0195980f). Let's not add any more of those.
Here is an idea: let's keep EventTriggerSupportsGrantObjectType which
instead of ACL_OBJECT_* uses OBJECT_*, but complains with an error if
the object is not supported with GRANT. Not need for a default in this
case.
--
Michael
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 10/12/17 22:18, Stephen Frost wrote: > I'm generally supportive of this, but I'm not entirely thrilled with how > this ends up conflating TABLEs and RELATIONs. From the GRANT > perspective, there's no distinction, and that was clear from the > language used and how things were handled, but the OBJECT enum has that > distinction. This change just makes VIEWs be OBJECT_TABLE even though > they actually aren't tables and there even is an OBJECT_VIEW value. > This commit may be able to grok that and manage it properly, but later > hackers might miss that. > > I would also suggest that the naming be consistent with the other bits > of the GRANT system (eg: ACL_ALL_RIGHTS_NAMESPACE would be changed to > ACL_ALL_RIGHTS_SCHEMA, to match OBJECT_SCHEMA). OK, here is a bigger patch set that addresses these issues. I have added OBJECT_RELATION to reflect the difference between TABLE and RELATION. I have also renamed NAMESPACE to SCHEMA. And then I got rid of AclObjectKind as well, because it's just another enum for the same thing. This is now a bit bigger, so I'll put it in the commit fest for detailed review. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Attachment
On Wed, Dec 13, 2017 at 1:46 AM, Peter Eisentraut <peter.eisentraut@2ndquadrant.com> wrote: > OK, here is a bigger patch set that addresses these issues. I have > added OBJECT_RELATION to reflect the difference between TABLE and > RELATION. I have also renamed NAMESPACE to SCHEMA. And then I got rid > of AclObjectKind as well, because it's just another enum for the same thing. > > This is now a bit bigger, so I'll put it in the commit fest for detailed > review. Patch 0001 is simply removing EventTriggerSupportsGrantObjectType(), but shouldn't we keep it and return an error for objects that have no GRANT support? Returning conditionally true looks like a trap waiting to take someone in.. I mentioned that in https://www.postgresql.org/message-id/CAB7nPqSR8Rsh-rcMjv7_2D7oksByre4FrHUeyn_KreHgO_YUPg@mail.gmail.com already, but this has remained unanswered. Similarly, not using default in the switches of stringify_adefprivs_objtype() and stringify_grantobjtype() would be nice to grab warnings during compilation. And patch 0002 is doing it the correct way in aclcheck_error(). -- Michael
On Tue, Dec 12, 2017 at 10:16 PM, Peter Eisentraut <peter.eisentraut@2ndquadrant.com> wrote:
I quickly look the patch and I liked the
v2-0001-Replace-GrantObjectType-with-ObjectType.patch, it's very clean
and making things much better.
I looked at another patch
About v2-0002-Replace-AclObjectKind-with-ObjectType.patch:
I noted that no_priv_msg and not_owner_msg array been removed
and code fitted the code into aclcheck_error(). Actually that
makes the code more complex then what it used to be. I would
prefer the array rather then code been fitted into the function.
On 10/12/17 22:18, Stephen Frost wrote:
> I'm generally supportive of this, but I'm not entirely thrilled with how
> this ends up conflating TABLEs and RELATIONs. From the GRANT
> perspective, there's no distinction, and that was clear from the
> language used and how things were handled, but the OBJECT enum has that
> distinction. This change just makes VIEWs be OBJECT_TABLE even though
> they actually aren't tables and there even is an OBJECT_VIEW value.
> This commit may be able to grok that and manage it properly, but later
> hackers might miss that.
>
> I would also suggest that the naming be consistent with the other bits
> of the GRANT system (eg: ACL_ALL_RIGHTS_NAMESPACE would be changed to
> ACL_ALL_RIGHTS_SCHEMA, to match OBJECT_SCHEMA).
OK, here is a bigger patch set that addresses these issues. I have
added OBJECT_RELATION to reflect the difference between TABLE and
RELATION. I have also renamed NAMESPACE to SCHEMA. And then I got rid
of AclObjectKind as well, because it's just another enum for the same thing.
This is now a bit bigger, so I'll put it in the commit fest for detailed
review.
I quickly look the patch and I liked the
v2-0001-Replace-GrantObjectType-with-ObjectType.patch, it's very clean
and making things much better.
I looked at another patch
About v2-0002-Replace-AclObjectKind-with-ObjectType.patch:
I noted that no_priv_msg and not_owner_msg array been removed
and code fitted the code into aclcheck_error(). Actually that
makes the code more complex then what it used to be. I would
prefer the array rather then code been fitted into the function.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
--
Rushabh Lathia
On 12/13/17 02:35, Michael Paquier wrote: > Patch 0001 is simply removing EventTriggerSupportsGrantObjectType(), > but shouldn't we keep it and return an error for objects that have no > GRANT support? Returning conditionally true looks like a trap waiting > to take someone in. I don't understand the motivation for this. It would just be two lists for the same thing. I think the potential for omission would be much greater that way. > Similarly, not using default in the switches of > stringify_adefprivs_objtype() and stringify_grantobjtype() would be > nice to grab warnings during compilation. And patch 0002 is doing it > the correct way in aclcheck_error(). I'll fix that. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 12/14/17 22:59, Rushabh Lathia wrote: > I noted that no_priv_msg and not_owner_msg array been removed > and code fitted the code into aclcheck_error(). Actually that > makes the code more complex then what it used to be. I would > prefer the array rather then code been fitted into the function. There is an argument for having a big array versus the switch/case approach. But most existing code around object addresses uses the switch/case approach, so it's better to align it that way, I think. It's weird to have to maintain two different styles. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Fri, Dec 15, 2017 at 12:40 PM, Peter Eisentraut <peter.eisentraut@2ndquadrant.com> wrote: > On 12/14/17 22:59, Rushabh Lathia wrote: >> I noted that no_priv_msg and not_owner_msg array been removed >> and code fitted the code into aclcheck_error(). Actually that >> makes the code more complex then what it used to be. I would >> prefer the array rather then code been fitted into the function. > > There is an argument for having a big array versus the switch/case > approach. But most existing code around object addresses uses the > switch/case approach, so it's better to align it that way, I think. > It's weird to have to maintain two different styles. Eh, really? What about the two big arrays at the top of objectaddress.c? (This is just a drive-by comment; I haven't looked at the details of this patch.) -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Sat, Dec 16, 2017 at 2:39 AM, Peter Eisentraut <peter.eisentraut@2ndquadrant.com> wrote: > On 12/13/17 02:35, Michael Paquier wrote: >> Patch 0001 is simply removing EventTriggerSupportsGrantObjectType(), >> but shouldn't we keep it and return an error for objects that have no >> GRANT support? Returning conditionally true looks like a trap waiting >> to take someone in. > > I don't understand the motivation for this. It would just be two lists > for the same thing. Not really. What grant supports is a subset of what event triggers do. > I think the potential for omission would be much greater that way. That's the whole point of not having "default" in the switches, no? Any object additions would be caught at compilation time. -- Michael
On Sat, Dec 16, 2017 at 12:40 AM, Robert Haas <robertmhaas@gmail.com> wrote:
Thanks, On Fri, Dec 15, 2017 at 12:40 PM, Peter Eisentraut
<peter.eisentraut@2ndquadrant.com > wrote:
> On 12/14/17 22:59, Rushabh Lathia wrote:
>> I noted that no_priv_msg and not_owner_msg array been removed
>> and code fitted the code into aclcheck_error(). Actually that
>> makes the code more complex then what it used to be. I would
>> prefer the array rather then code been fitted into the function.
>
> There is an argument for having a big array versus the switch/case
> approach. But most existing code around object addresses uses the
> switch/case approach, so it's better to align it that way, I think.
> It's weird to have to maintain two different styles.
Only motivation is, earlier approach looks more cleaner. Also patch is
getting bigger - so if we continue with old approach it will make review
easy. Just in case switch/case approach is a go to, then it can be
done as part of separate clean up patch.
Rushabh Lathia
On 12/15/17 14:10, Robert Haas wrote: >> There is an argument for having a big array versus the switch/case >> approach. But most existing code around object addresses uses the >> switch/case approach, so it's better to align it that way, I think. >> It's weird to have to maintain two different styles. > Eh, really? What about the two big arrays at the top of objectaddress.c? They are not indexed by object type. I can't find any existing array or other structure that fits into what this patch is doing (other than the one this patch is removing). -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 12/18/17 02:38, Rushabh Lathia wrote: > Only motivation is, earlier approach looks more cleaner. Also patch is > getting bigger - so if we continue with old approach it will make review > easy. Just in case switch/case approach is a go to, then it can be > done as part of separate clean up patch. If find the approach with the giant array harder to maintain because you typically need to maintain a consistent order between an enum in one file and arrays in other files, and the only approaches to satisfy this are hope and 100% test coverage. And then if you want to reorder or insert something, you need to do it everywhere at once in a very careful manner. In this particular patch, it would also bloat the array even more, because we don't support grants on all object types, and with the switch approach we can easily omit those. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 12/15/17 17:34, Michael Paquier wrote: > On Sat, Dec 16, 2017 at 2:39 AM, Peter Eisentraut > <peter.eisentraut@2ndquadrant.com> wrote: >> On 12/13/17 02:35, Michael Paquier wrote: >>> Patch 0001 is simply removing EventTriggerSupportsGrantObjectType(), >>> but shouldn't we keep it and return an error for objects that have no >>> GRANT support? Returning conditionally true looks like a trap waiting >>> to take someone in. >> >> I don't understand the motivation for this. It would just be two lists >> for the same thing. > > Not really. What grant supports is a subset of what event triggers do. > >> I think the potential for omission would be much greater that way. > > That's the whole point of not having "default" in the switches, no? > Any object additions would be caught at compilation time. I think the purpose of EventTriggerSupportsGrantObjectType() is to tell which object types are supported for event triggers. The purpose is not to tell which object types are supported by GRANT. The way I have written it, if GRANT gets support for a new object type, then event trigger support automatically happens, without having to update another list. As a related example, we use the generic EventTriggerSupportsObjectType() for RenameStmt, even though we don't actually support RenameStmt on every ObjectType. And there are probably more examples like that. Taken to the extreme, you'd need to remove EventTriggerSupportsObjectType() altogether. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Peter Eisentraut wrote: > On 12/15/17 17:34, Michael Paquier wrote: > > On Sat, Dec 16, 2017 at 2:39 AM, Peter Eisentraut > > <peter.eisentraut@2ndquadrant.com> wrote: > > That's the whole point of not having "default" in the switches, no? > > Any object additions would be caught at compilation time. > > I think the purpose of EventTriggerSupportsGrantObjectType() is to tell > which object types are supported for event triggers. The purpose is not > to tell which object types are supported by GRANT. > > The way I have written it, if GRANT gets support for a new object type, > then event trigger support automatically happens, without having to > update another list. That's correct, and using a single implementation as in the posted patch is desirable. I was not happy about having to add EventTriggerSupportsGrantObjectType (c.f. commit 296f3a605384). -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Wed, Dec 20, 2017 at 5:43 AM, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> Peter Eisentraut wrote:
>> On 12/15/17 17:34, Michael Paquier wrote:
>> > On Sat, Dec 16, 2017 at 2:39 AM, Peter Eisentraut
>> > <peter.eisentraut@2ndquadrant.com> wrote:
>
>> > That's the whole point of not having "default" in the switches, no?
>> > Any object additions would be caught at compilation time.
>>
>> I think the purpose of EventTriggerSupportsGrantObjectType() is to tell
>> which object types are supported for event triggers. The purpose is not
>> to tell which object types are supported by GRANT.
>>
>> The way I have written it, if GRANT gets support for a new object type,
>> then event trigger support automatically happens, without having to
>> update another list.
>
> That's correct, and using a single implementation as in the posted patch
> is desirable. I was not happy about having to add
> EventTriggerSupportsGrantObjectType (c.f. commit 296f3a605384).
Hm. OK. I would have thought that allowing a new object to work
automatically is actually we would have liked to avoid for safety. So
complain withdrawn.
-stringify_adefprivs_objtype(GrantObjectType objtype)
+stringify_adefprivs_objtype(ObjectType objtype)
[...]
+ default:
+ elog(ERROR, "unrecognized grant object type: %d", (int) objtype);
+ return "???"; /* keep compiler quiet */
}
-
- elog(ERROR, "unrecognized grant object type: %d", (int) objtype);
- return "???"; /* keep compiler quiet */
Still this portion in 0001 is something that we try to avoid as much
as possible, no? I would have thought that all object types should be
listed directly so as nothing is missed in the future.
--
Michael
On 12/19/17 19:56, Michael Paquier wrote: > -stringify_adefprivs_objtype(GrantObjectType objtype) > +stringify_adefprivs_objtype(ObjectType objtype) > [...] > + default: > + elog(ERROR, "unrecognized grant object type: %d", (int) objtype); > + return "???"; /* keep compiler quiet */ > } > - > - elog(ERROR, "unrecognized grant object type: %d", (int) objtype); > - return "???"; /* keep compiler quiet */ > Still this portion in 0001 is something that we try to avoid as much > as possible, no? I would have thought that all object types should be > listed directly so as nothing is missed in the future. But we don't really know what such future GRANT commands would actually look like. What would the GRANT syntax corresponding to OBJECT_CAST or OBJECT_STATISTIC_EXT be? I think that's best filled in when we know. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Peter Eisentraut wrote: > On 12/19/17 19:56, Michael Paquier wrote: > > -stringify_adefprivs_objtype(GrantObjectType objtype) > > +stringify_adefprivs_objtype(ObjectType objtype) > > [...] > > + default: > > + elog(ERROR, "unrecognized grant object type: %d", (int) objtype); > > + return "???"; /* keep compiler quiet */ > > } > > - > > - elog(ERROR, "unrecognized grant object type: %d", (int) objtype); > > - return "???"; /* keep compiler quiet */ > > Still this portion in 0001 is something that we try to avoid as much > > as possible, no? I would have thought that all object types should be > > listed directly so as nothing is missed in the future. > > But we don't really know what such future GRANT commands would actually > look like. What would the GRANT syntax corresponding to OBJECT_CAST or > OBJECT_STATISTIC_EXT be? I think that's best filled in when we know. 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. -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
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. 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. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Attachment
On Thu, Dec 21, 2017 at 1:19 AM, Peter Eisentraut <peter.eisentraut@2ndquadrant.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 have not looked at 0002 in details. -- Michael
Michael, Peter, all, * Michael Paquier (michael.paquier@gmail.com) wrote: > On Thu, Dec 21, 2017 at 1:19 AM, Peter Eisentraut > <peter.eisentraut@2ndquadrant.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
Attachment
On 12/20/17 22:01, Stephen Frost wrote: > 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. I think that we allow a sequence to be treated like a table in GRANT (and other places) is a historical wart that we won't easily be able to get rid of. I don't think the object address system should be bent to accommodate that. I'd rather see the warts in the code explicitly. > 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. To some extent 0002 does that. > 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. I'm not sure I follow that, since it appears to be the opposite of what you said earlier, i.e., we should have OBJECT_RELATION so as to avoid using OBJECT_TABLE when we don't really know yet whether something is a table. > 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 can look into that. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Tue, Dec 26, 2017 at 02:15:18PM -0500, Peter Eisentraut wrote: > On 12/20/17 22:01, Stephen Frost wrote: > > 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. > > I think that we allow a sequence to be treated like a table in GRANT > (and other places) is a historical wart that we won't easily be able to > get rid of. I don't think the object address system should be bent to > accommodate that. I'd rather see the warts in the code explicitly. Yes, I agree with that. GRANT without an object defined works fine for sequences, so you want to keep an abstraction level where a relation means more than a simple table. -- Michael
Attachment
Peter, * Peter Eisentraut (peter.eisentraut@2ndquadrant.com) wrote: > On 12/20/17 22:01, Stephen Frost wrote: > > 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. > > I think that we allow a sequence to be treated like a table in GRANT > (and other places) is a historical wart that we won't easily be able to > get rid of. I don't think the object address system should be bent to > accommodate that. I'd rather see the warts in the code explicitly. I agree that we won't be able to stop allowing GRANT to accept various object types without an explicit type being declared. What might actually be nice is if we'd allow *more* things to be specified that way rather than trying to tighten it up- I can't count on all my fingers and toes the number of times I've done 'grant usage on myschema to joe;' and been most annoyed that it didn't work. Supporting that today would involve making a 'relation-or-schema' thing in the object system because we're forcing ourselves to call whatever is passed in to GRANT a certain kind of object before we've really figured out what kind of object it is, and that's what I'm objecting to here. I don't want to bend the object address system to handle that either, and so I agree that it'd be better to have the GRANT code deal with the ambiguity directly. > > 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. > > I'm not sure I follow that, since it appears to be the opposite of what > you said earlier, i.e., we should have OBJECT_RELATION so as to avoid > using OBJECT_TABLE when we don't really know yet whether something is a > table. I didn't actually suggest having an OBJECT_RELATION- I complained that you were stamping 'OBJECT_TABLE' on things that definitely weren't tables and that you were conflating tables and relations. I'm afraid that I wasn't terribly clear on a path forward two months ago because I didn't really have any great ideas on how to fix that while avoiding having overlapping object types, which I do think is something we should try to avoid. > > 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 can look into that. Yes, documenting it, at least, is necessary if we're going to go with this approach, though I wonder if that will end up making it difficult to remove it later if someone gets around to reworking the GRANT system to directly deal with this ambiguity without needing a special-case in the object address system for it. I guess one question coming out of this is- do you see another use for this OBJECT_RELATION object type..? If it's really only this one special case in GRANT that needs it then I think that makes it much less appealing to have. Thanks! Stephen
Attachment
Michael, * Michael Paquier (michael.paquier@gmail.com) wrote: > On Tue, Dec 26, 2017 at 02:15:18PM -0500, Peter Eisentraut wrote: > > On 12/20/17 22:01, Stephen Frost wrote: > > > 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. > > > > I think that we allow a sequence to be treated like a table in GRANT > > (and other places) is a historical wart that we won't easily be able to > > get rid of. I don't think the object address system should be bent to > > accommodate that. I'd rather see the warts in the code explicitly. > > Yes, I agree with that. GRANT without an object defined works fine for > sequences, so you want to keep an abstraction level where a relation > means more than a simple table. I'm not sure that I agree that this is really an 'abstraction' in this instance- it's not like we can keep considering the object passed in a generic 'relation' in the GRANT case throughout, instead we end up having to run down what it is later on to make sure that the GRANT command is valid, so it's really just that it's ambiguous at parse time and we don't try to resolve it right away to exactly what it is until we get a bit farther along. We could certainly just run down what it is right after the RangeVarGetRelId(), or have cases in the switch statements in objectNamesToOids() and ExecGrantStmt_oids() that handle a case where the object type isn't known yet and then do some kind of lookup to figure out what kind of object it actually is. It'd be nice to only do that lookup once (which is how things work today), but, as I mentioned up-thread, that would require moving more things down (not that I think that's really a bad thing...). Thanks! Stephen
Attachment
On 12/28/17 14:22, Stephen Frost wrote: > Yes, documenting it, at least, is necessary if we're going to go with > this approach, though I wonder if that will end up making it difficult > to remove it later if someone gets around to reworking the GRANT system > to directly deal with this ambiguity without needing a special-case in > the object address system for it. I guess one question coming out of > this is- do you see another use for this OBJECT_RELATION object type..? > If it's really only this one special case in GRANT that needs it then I > think that makes it much less appealing to have. 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. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Attachment
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
Attachment
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
On Wed, Jan 17, 2018 at 05:23:25PM -0500, Peter Eisentraut wrote: > On 1/16/18 23:38, Michael Paquier wrote: >> + 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. OK, I can live with that argument. -- Michael
Attachment
On 1/17/18 23:52, Michael Paquier wrote: > On Wed, Jan 17, 2018 at 05:23:25PM -0500, Peter Eisentraut wrote: >> On 1/16/18 23:38, Michael Paquier wrote: >>> + 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. > > OK, I can live with that argument. committed -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services