replace GrantObjectType with ObjectType

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

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
/messages/by-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:

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:

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.

Thanks,
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