Re: [HACKERS] replace GrantObjectType with ObjectType

2017-10-16 Thread Michael Paquier 
On Thu, Oct 12, 2017 at 5:43 PM, Alvaro Herrera  wrote:
> Michael Paquier wrote:
>> On Thu, Oct 12, 2017 at 7:55 AM, Peter Eisentraut
>>  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

Re: [HACKERS] replace GrantObjectType with ObjectType

2017-10-12 Thread Stephen Frost 
Peter,

* Peter Eisentraut (peter.eisentr...@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


signature.asc
Description: Digital signature

Re: [HACKERS] replace GrantObjectType with ObjectType

2017-10-12 Thread Alvaro Herrera 
Michael Paquier wrote:
> On Thu, Oct 12, 2017 at 7:55 AM, Peter Eisentraut
>  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 Herrerahttps://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

Re: [HACKERS] replace GrantObjectType with ObjectType

2017-10-12 Thread Michael Paquier 
On Thu, Oct 12, 2017 at 7:55 AM, Peter Eisentraut
 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

[HACKERS] replace GrantObjectType with ObjectType

2017-10-11 Thread Peter Eisentraut 
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
From f80e5aced1cedd2682fd50f6fa067bf455f66f4d Mon Sep 17 00:00:00 2001
From: Peter Eisentraut 
Date: Wed, 11 Oct 2017 18:35:19 -0400
Subject: [PATCH] Replace GrantObjectType with ObjectType

There used to be a lot of different *Type and *Kind symbol groups to
address objects within different commands, most of which have been
replaced by ObjectType, starting with
b256f2426433c56b4bea3a8102757749885b81ba.  But this conversion was never
done for the ACL commands until now.

This change ends up being just a plain replacement of the types and
symbols, without any code restructuring needed, except deleting some now
redundant code.
---
 src/backend/catalog/aclchk.c | 208 +--
 src/backend/catalog/heap.c   |   4 +-
 src/backend/catalog/pg_namespace.c   |   2 +-
 src/backend/catalog/pg_proc.c|   2 +-
 src/backend/catalog/pg_type.c|   2 +-
 src/backend/commands/event_trigger.c | 109 +++---
 src/backend/parser/gram.y|  44 
 src/backend/tcop/utility.c   |   2 +-
 src/backend/utils/adt/acl.c  |  56 +-
 src/include/commands/event_trigger.h |   1 -
 src/include/nodes/parsenodes.h   |  19 +---
 src/include/tcop/deparse_utility.h   |   2 +-
 src/include/utils/acl.h  |   4 +-
 src/include/utils/aclchk_internal.h  |   2 +-
 14 files changed, 203 insertions(+), 254 deletions(-)

diff --git a/src/backend/catalog/aclchk.c b/src/backend/catalog/aclchk.c
index ccde66a7dd..2aee4f2751 100644
--- a/src/backend/catalog/aclchk.c
+++ b/src/backend/catalog/aclchk.c
@@ -86,7 +86,7 @@ typedef struct
Oid nspid;  /* namespace, or 
InvalidOid if none */
/* remaining fields are same as in InternalGrant: */
boolis_grant;
-   GrantObjectType objtype;
+   ObjectType  objtype;
boolall_privs;
AclMode privileges;
List   *grantees;
@@ -116,8 +116,8 @@ static void ExecGrant_Type(InternalGrant *grantStmt);
 static void SetDefaultACLsInSchemas(InternalDefaultACL *iacls, List *nspnames);
 static void SetDefaultACL(InternalDefaultACL *iacls);
 
-static List *objectNamesToOids(GrantObjectType objtype, List *objnames);
-static List *objectsInSchemaToOids(GrantObjectType objtype, List *nspnames);
+static List *objectNamesToOids(ObjectType objtype, List *objnames);
+static List *objectsInSchemaToOids(ObjectType objtype, List *nspnames);
 static List *getRelationsInNamespace(Oid namespaceId, char relkind);
 static void expand_col_privileges(List *colnames, Oid table_oid,
  AclMode this_privileges,
@@ -441,60 +441,60 @@ ExecuteGrantStmt(GrantStmt *stmt)
 
/*
 * Convert stmt->privileges, a list of AccessPriv nodes, into an AclMode
-* bitmask.  Note: objtype can't be ACL_OBJECT_COLUMN.
+* bitmask.  Note: objtype can't be OBJECT_COLUMN.
 */
switch (stmt->objtype)
{
+   case OBJECT_TABLE:
/*
 * Because this might be a sequence, we test both 
relation and
 * sequence bits, and later do a more limited test when 
we know
 * the object type.
 */
-   case ACL_OBJECT_RELATION:
all_privileges = ACL_ALL_RIGHTS_RELATION | 
ACL_ALL_RIGHTS_SEQUENCE;
errormsg = gettext_noop("invalid privilege type %s for 
relation");
break;
-   case ACL_OBJECT_SEQUENCE:
+   case OBJECT_SEQUENCE:
all_privileges = ACL_ALL_RIGHTS_SEQUENCE;
errormsg = gettext_noop("invalid privilege type %s for 
sequence");
break;
-   case ACL_OBJECT_DATABASE:
+   case OBJECT_DATABASE:
all_privileges = ACL_ALL_RIGHTS_DATABASE;
errormsg = gettext_noop("invalid privilege type %s for 
database");
break;
-   case ACL_OBJECT_DOMAIN:
+   case OBJECT_DOMAIN:
all_privileges = ACL_ALL_RIGHTS_TYPE;
errormsg = gettext_noop("invalid privilege type %s for 
domain");
break;
-   case ACL_OBJECT_FUNCTION:
+   case OBJECT_FUNCTION:
all_privileges =