Re: [HACKERS] Role Attribute Bitmask Catalog Representation

2014-12-23 Thread Alvaro Herrera
Adam Brightwell wrote: Alvaro and Stephen, I propose this patch on top of Adam's v5. Also included is a full patch against master. I have attached an updated patch for review (role-attribute-bitmask-v7.patch). This patch incorporates the 'v5a' patch proposed by Alvaro, input

Re: [HACKERS] Role Attribute Bitmask Catalog Representation

2014-12-23 Thread Adam Brightwell
Alvarao, Pushed with a couple of small changes (Catalog.pm complained about the lack of a CATALOG() line in the new acldefs.h file; you had pg_role_all_attributes as returning bool in the table, but it returns text[]; and I added index entries for the new functions.) That's fantastic!

Re: [HACKERS] Role Attribute Bitmask Catalog Representation

2014-12-22 Thread Stephen Frost
Hey Alvaro, * Adam Brightwell (adam.brightw...@crunchydatasolutions.com) wrote: I have attached an updated patch for review (role-attribute-bitmask-v7.patch). This patch incorporates the 'v5a' patch proposed by Alvaro, input validation (Assert) check in 'check_role_attribute' and the

Re: [HACKERS] Role Attribute Bitmask Catalog Representation

2014-12-22 Thread Alvaro Herrera
Stephen Frost wrote: Hey Alvaro, * Adam Brightwell (adam.brightw...@crunchydatasolutions.com) wrote: I have attached an updated patch for review (role-attribute-bitmask-v7.patch). This patch incorporates the 'v5a' patch proposed by Alvaro, input validation (Assert) check in

Re: [HACKERS] Role Attribute Bitmask Catalog Representation

2014-12-22 Thread Stephen Frost
* Alvaro Herrera (alvhe...@2ndquadrant.com) wrote: Stephen Frost wrote: * Adam Brightwell (adam.brightw...@crunchydatasolutions.com) wrote: I have attached an updated patch for review (role-attribute-bitmask-v7.patch). This patch incorporates the 'v5a' patch proposed by Alvaro,

Re: [HACKERS] Role Attribute Bitmask Catalog Representation

2014-12-20 Thread Adam Brightwell
Alvaro and Stephen, I propose this patch on top of Adam's v5. Also included is a full patch against master. I have attached an updated patch for review (role-attribute-bitmask-v7.patch). This patch incorporates the 'v5a' patch proposed by Alvaro, input validation (Assert) check in

Re: [HACKERS] Role Attribute Bitmask Catalog Representation

2014-12-19 Thread Alvaro Herrera
Stephen Frost wrote: Alvaro, * Alvaro Herrera (alvhe...@2ndquadrant.com) wrote: FWIW I've been giving this patch a look and and adjusting some coding details here and there. Do you intend to commit it yourself? You're not listed as reviewer or committer for it in the commitfest app,

Re: [HACKERS] Role Attribute Bitmask Catalog Representation

2014-12-19 Thread Alvaro Herrera
The fact that the ROLE_ATTR_* definitions are in pg_authid.h means that there are now a lot of files that need to include that one. I think the includes relative to ACLs and roles is rather messy now, and this patch makes it a bit worse. I think we should create a new header file (maybe

Re: [HACKERS] Role Attribute Bitmask Catalog Representation

2014-12-19 Thread Alvaro Herrera
Alvaro Herrera wrote: The fact that the ROLE_ATTR_* definitions are in pg_authid.h means that there are now a lot of files that need to include that one. I think the includes relative to ACLs and roles is rather messy now, and this patch makes it a bit worse. I think we should create a new

Re: [HACKERS] Role Attribute Bitmask Catalog Representation

2014-12-19 Thread Stephen Frost
* Alvaro Herrera (alvhe...@2ndquadrant.com) wrote: Alvaro Herrera wrote: I think we should create a new header file (maybe acltypes.h or acldefs.h), which only contains the AclMode and RoleAttr typedefs and their associated defines; that one would be included from parsenodes.h, acl.h and

Re: [HACKERS] Role Attribute Bitmask Catalog Representation

2014-12-18 Thread Stephen Frost
Adam, * Adam Brightwell (adam.brightw...@crunchydatasolutions.com) wrote: I have attached an updated patch with initial documentation changes for review. Awesome, thanks. I'm going to continue looking at this in more detail, but wanted to mention a few things I noticed in the documentation

Re: [HACKERS] Role Attribute Bitmask Catalog Representation

2014-12-18 Thread Alvaro Herrera
FWIW I've been giving this patch a look and and adjusting some coding details here and there. Do you intend to commit it yourself? You're not listed as reviewer or committer for it in the commitfest app, FWIW. One thing I don't very much like is that check_role_attribute() receives a RoleAttr

Re: [HACKERS] Role Attribute Bitmask Catalog Representation

2014-12-18 Thread Stephen Frost
Alvaro, * Alvaro Herrera (alvhe...@2ndquadrant.com) wrote: FWIW I've been giving this patch a look and and adjusting some coding details here and there. Do you intend to commit it yourself? You're not listed as reviewer or committer for it in the commitfest app, FWIW. Oh, great, thanks!

Re: [HACKERS] Role Attribute Bitmask Catalog Representation

2014-12-15 Thread Adam Brightwell
All, Overall, I'm pretty happy with the patch and would suggest moving on to writing up the documentation changes to go along with the code changes. I'll continue to play around with it but it all seems pretty clean to me and will allow us to easily add the additiaonl role attributes being

Re: [HACKERS] Role Attribute Bitmask Catalog Representation

2014-12-08 Thread Adam Brightwell
Michael, This work will certainly continue to be pursued. If a simple move is possible/acceptable, then I think that would be the best option. I can handle that as it would appear that I am capable of moving it, if that is acceptable. Yes please. Actually I could have done it, just

Re: [HACKERS] Role Attribute Bitmask Catalog Representation

2014-12-08 Thread Michael Paquier
On Tue, Dec 9, 2014 at 12:10 AM, Adam Brightwell adam.brightw...@crunchydatasolutions.com wrote: Michael, This work will certainly continue to be pursued. If a simple move is possible/acceptable, then I think that would be the best option. I can handle that as it would appear that I am

Re: [HACKERS] Role Attribute Bitmask Catalog Representation

2014-12-07 Thread Michael Paquier
On Sun, Dec 7, 2014 at 1:50 AM, Stephen Frost sfr...@snowman.net wrote: * Adam Brightwell (adam.brightw...@crunchydatasolutions.com) wrote: I don't see any changes to the regression test files, were they forgotten in the patch? I would think that at least the view definition changes would

Re: [HACKERS] Role Attribute Bitmask Catalog Representation

2014-12-07 Thread Adam Brightwell
Michael, This patch (https://commitfest.postgresql.org/action/patch_view?id=1616) has not been updated in the commitfest app for two months, making its progress hard to track. I believe that the mentioned patch should be considered 'on hold' or 'dependent' upon the acceptance of the work that

Re: [HACKERS] Role Attribute Bitmask Catalog Representation

2014-12-07 Thread Michael Paquier
On Mon, Dec 8, 2014 at 12:34 PM, Adam Brightwell adam.brightw...@crunchydatasolutions.com wrote: Michael, This patch (https://commitfest.postgresql.org/action/patch_view?id=1616) has not been updated in the commitfest app for two months, making its progress hard to track. I believe that

Re: [HACKERS] Role Attribute Bitmask Catalog Representation

2014-12-06 Thread Stephen Frost
Adam, * Adam Brightwell (adam.brightw...@crunchydatasolutions.com) wrote: Attached is an updated patch. Awesome, thanks! diff --git a/src/backend/catalog/aclchk.c b/src/backend/catalog/aclchk.c *** pg_extension_ownercheck(Oid ext_oid, Oid *** 5051,5102 } /* ! *

Re: [HACKERS] Role Attribute Bitmask Catalog Representation

2014-12-06 Thread Adam Brightwell
Stephen, The comment above is pretty big and I don't think we want to completely remove it. Can you add it as another 'Note' in the 'has_role_attribute' comment and reword it accordingly? Ok, agreed. Will address. Whitespace issue that should be fixed- attributes should line up with

Re: [HACKERS] Role Attribute Bitmask Catalog Representation

2014-12-06 Thread Stephen Frost
* Adam Brightwell (adam.brightw...@crunchydatasolutions.com) wrote: I don't see any changes to the regression test files, were they forgotten in the patch? I would think that at least the view definition changes would require updates to the regression tests, though perhaps nothing else.

Re: [HACKERS] Role Attribute Bitmask Catalog Representation

2014-12-05 Thread Adam Brightwell
All, I have attached an updated patch that incorporates the feedback and recommendations provided. Thanks, Adam -- Adam Brightwell - adam.brightw...@crunchydatasolutions.com Database Engineer - www.crunchydatasolutions.com diff --git a/src/backend/access/transam/xlogfuncs.c

Re: [HACKERS] Role Attribute Bitmask Catalog Representation

2014-12-05 Thread Stephen Frost
Adam, * Adam Brightwell (adam.brightw...@crunchydatasolutions.com) wrote: I have attached an updated patch that incorporates the feedback and recommendations provided. Thanks. Comments below. diff --git a/src/backend/access/transam/xlogfuncs.c b/src/backend/access/transam/xlogfuncs.c ---

Re: [HACKERS] Role Attribute Bitmask Catalog Representation

2014-12-05 Thread Adam Brightwell
Stephen, Thanks for the feedback. diff --git a/src/backend/access/transam/xlogfuncs.c b/src/backend/access/transam/xlogfuncs.c --- 56,62 backupidstr = text_to_cstring(backupid); ! if (!superuser() !check_role_attribute(GetUserId(), ROLE_ATTR_REPLICATION))

Re: [HACKERS] Role Attribute Bitmask Catalog Representation

2014-12-02 Thread Stephen Frost
Adam, * Adam Brightwell (adam.brightw...@crunchydatasolutions.com) wrote: Ok. Though, this would affect how CATUPDATE is handled. Peter Eisentraut previously raised a question about whether superuser checks should be included with catupdate which led me to create the following post.

Re: [HACKERS] Role Attribute Bitmask Catalog Representation

2014-12-01 Thread Stephen Frost
Adam, * Adam Brightwell (adam.brightw...@crunchydatasolutions.com) wrote: Please let me know what you think, all feedback is greatly appreciated. Thanks for this. Found time to do more of a review and have a few comments: diff --git a/src/backend/catalog/aclchk.c

Re: [HACKERS] Role Attribute Bitmask Catalog Representation

2014-12-01 Thread Adam Brightwell
Stephen, Thanks for this. Found time to do more of a review and have a few comments: Thanks for taking a look at this and for the feedback. I'd put the superuser_arg() check in role_has_attribute. Ok. Though, this would affect how CATUPDATE is handled. Peter Eisentraut previously raised

Re: [HACKERS] Role Attribute Bitmask Catalog Representation

2014-11-28 Thread Adam Brightwell
All, I have attached a patch that addresses the current suggestions and recommendations: * Add 'get_all_role_attributes' SQL function - returns a text array representation of the attributes from a value passed to it. Example: postgres=# SELECT rolname, get_all_role_attributes(rolattr) AS

Re: [HACKERS] Role Attribute Bitmask Catalog Representation

2014-11-26 Thread Stephen Frost
Adam, * Adam Brightwell (adam.brightw...@crunchydatasolutions.com) wrote: I am simply breaking this out into its own thread from the discussion on additional role attributes ( http://www.postgresql.org/message-id/20141015052259.gg28...@tamriel.snowman.net ). Makes sense to me, thanks.

Re: [HACKERS] Role Attribute Bitmask Catalog Representation

2014-11-25 Thread Adam Brightwell
Andres, Thanks for the feedback. * int64 (C) to int8 (SQL) mapping for genbki. That definitely should be a separate patch. Which can be committed much earlier than the rest - even if we don't actually end up needing it for this feature, it's still good to have it. Agreed. I had

Re: [HACKERS] Role Attribute Bitmask Catalog Representation

2014-11-25 Thread Stephen Frost
* Adam Brightwell (adam.brightw...@crunchydatasolutions.com) wrote: * int64 (C) to int8 (SQL) mapping for genbki. That definitely should be a separate patch. Which can be committed much earlier than the rest - even if we don't actually end up needing it for this feature, it's still good

Re: [HACKERS] Role Attribute Bitmask Catalog Representation

2014-11-25 Thread Adam Brightwell
David, A few related threads/discussions/posts: * http://www.postgresql.org/message-id/ 20141016115914.GQ28859@.snowman * http://www.postgresql.org/message-id/CA+TgmobkYXNOWKEKzX2qGPSr_nvacFGueV= orxND-xmZvOVYvg@.gmail * http://www.postgresql.org/message-id/

Re: [HACKERS] Role Attribute Bitmask Catalog Representation

2014-11-25 Thread Stephen Frost
* Adam Brightwell (adam.brightw...@crunchydatasolutions.com) wrote: An array representation was also suggested by Simon ( http://www.postgresql.org/message-id/ca+u5nmjgvdz6jx_ybjk99nj7mwfgfvemxtdc44lvhq64gkn...@mail.gmail.com). Obviously there are pro's and con's to either approach. I'm not

Re: [HACKERS] Role Attribute Bitmask Catalog Representation

2014-11-25 Thread Adam Brightwell
All, I think if we're going to do this - and I'm not yet convinced that that's the best route, we should add returns all permissions a user has. Right now that's quite easily queryable, but it won't be after moving everything into one column. You'd need to manually use all has_*_

Re: [HACKERS] Role Attribute Bitmask Catalog Representation

2014-11-24 Thread Andres Freund
On 2014-11-24 15:39:22 -0500, Adam Brightwell wrote: * int64 (C) to int8 (SQL) mapping for genbki. That definitely should be a separate patch. Which can be committed much earlier than the rest - even if we don't actually end up needing it for this feature, it's still good to have it. * replace

Re: [HACKERS] Role Attribute Bitmask Catalog Representation

2014-11-24 Thread David G Johnston
Adam Brightwell wrote A few related threads/discussions/posts: * http://www.postgresql.org/message-id/ 20141016115914.GQ28859@.snowman * http://www.postgresql.org/message-id/CA+TgmobkYXNOWKEKzX2qGPSr_nvacFGueV= orxND-xmZvOVYvg@.gmail * http://www.postgresql.org/message-id/