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!
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, i
* 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
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
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 do
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 'check_ro
* 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,
> > ac
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
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 acltypes.
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
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!
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 bu
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 c
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
>
On Tue, Dec 9, 2014 at 12:10 AM, Adam Brightwell
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 capable of moving it, if tha
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,
On Mon, Dec 8, 2014 at 12:34 PM, Adam Brightwell
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 the mentioned patch should be co
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 tha
On Sun, Dec 7, 2014 at 1:50 AM, Stephen Frost 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 require u
* 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
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
> ro
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
> }
>
> /*
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_REPLICAT
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
>
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 b/src/backend/acces
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.
>
> h
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 raise
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 b/src/backend/catalog/aclch
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 rolatt
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.
>
Stephen,
Having an array sounds pretty reasonable to me.
>
Ok, sounds good, I think so too.
Users interested in having a string instead could use array_to_string().
> Having to go the other way isn't as nice, imo.
>
My thoughts exactly, but wanted to at least put it out there.
Thanks,
Adam
-
Adam,
* Adam Brightwell (adam.brightw...@crunchydatasolutions.com) wrote:
> Giving this some thought, I'm curious what would be acceptable as an end
> result, specifically related to how a query on pg_authid might look/work.
> I was able to preserve the structure of results from pg_roles, however,
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_*_
>> fu
* 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
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/
* 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 st
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 previ
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/
> 2
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.
> * replac
All,
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
).
A few related threads/discussions/posts:
*
http://www.postgresql.org/message-id/20141016115914.gq28...@tam
40 matches
Mail list logo
