Re: [PATCH] pg_permissions
On 02/26/22 03:27, Joel Jacobson wrote: > On Fri, Feb 25, 2022, at 22:12, Chapman Flack wrote: >> I would be happy to review this patch, but a look through the email leaves me >> thinking it may still be waiting on a C implementation of pg_get_acl(). Is >> that >> right? > > Not sure. It looked to me as if the -hackers messages of 25 and 26 March 2021 had found a consensus that a pg_get_acl() function would be a good thing, with the views to be implemented over that. I'm just not seeing any later patch that adds such a function. Regards, -Chap
Re: [PATCH] pg_permissions
On Fri, Feb 25, 2022, at 22:12, Chapman Flack wrote: > I would be happy to review this patch, but a look through the email leaves me > thinking it may still be waiting on a C implementation of pg_get_acl(). Is > that > right? Not sure. > And perhaps a view rename to pg_privileges, following Peter's comment? +1 /Joel
Re: [PATCH] pg_permissions
I would be happy to review this patch, but a look through the email leaves me thinking it may still be waiting on a C implementation of pg_get_acl(). Is that right? And perhaps a view rename to pg_privileges, following Peter's comment?
Re: [PATCH] pg_permissions
On 11.03.21 08:00, Joel Jacobson wrote: Do we prefer "pg_permissions" or "pg_privileges"? pg_privileges would be better. "Permissions" is not an SQL term.
Re: [PATCH] pg_permissions
On Fri, Mar 26, 2021, at 14:16, Tom Lane wrote: > Alvaro Herrera mailto:alvherre%40alvh.no-ip.org>> > writes: > > On 2021-Mar-26, Joel Jacobson wrote: > >> On Thu, Mar 25, 2021, at 17:51, Tom Lane wrote: > >> I wonder what performance will be like with lots o' objects. > > > I guess he is concerned about the number of catalog accesses. > > My concern is basically that you're forcing the join between > pg_shdepend and $everything_else to be done as a nested loop. > It will work well, up to where you have so many objects that > it doesn't ... but the planner will have no way to improve it. Thanks Alvaro and Tom for explaining. > Having said that, I don't really see a better way either. > Materializing $everything_else via a UNION ALL seems like > no fun from a maintenance perspective, plus we're not that > great on optimizing such constructs either. I see why pg_shdepend+pg_get_acl() is to prefer. That said, I think maintenance of UNION ALL would actually not be too bad, since the system views could initially be generated by a query using information_schema, and the same query could update them when new catalogs are added. /Joel
Re: [PATCH] pg_permissions
Alvaro Herrera writes: > On 2021-Mar-26, Joel Jacobson wrote: >> On Thu, Mar 25, 2021, at 17:51, Tom Lane wrote: >> I wonder what performance will be like with lots o' objects. > I guess he is concerned about the number of catalog accesses. My concern is basically that you're forcing the join between pg_shdepend and $everything_else to be done as a nested loop. It will work well, up to where you have so many objects that it doesn't ... but the planner will have no way to improve it. Having said that, I don't really see a better way either. Materializing $everything_else via a UNION ALL seems like no fun from a maintenance perspective, plus we're not that great on optimizing such constructs either. regards, tom lane
Re: [PATCH] pg_permissions
On 2021-Mar-26, Joel Jacobson wrote: > On Fri, Mar 26, 2021, at 11:30, Alvaro Herrera wrote: > > On 2021-Mar-26, Joel Jacobson wrote: > > > > > On Thu, Mar 25, 2021, at 17:51, Tom Lane wrote: > > > > > > I wonder what performance will be like with lots o' objects. > > > > > > I guess pg_get_acl() would need to be implemented using a switch(classid) > > > with 36 cases (one for each class)? > > > > No, we have a generalized object query mechanism, see objectaddress.c > > That's where I was looking actually and noticed the switch with 36 cases, in > the function getObjectDescription(). Ah! well, you don't have to repeat that. AFAICS the way to do it is like AlterObjectOwner_internal obtains data -- first do get_catalog_object_by_oid (gives you the HeapTuple that represents the object), then heap_getattr( ..., get_object_attnum_acl(), ..), and there you have the ACL which you can "explode" (or maybe just return as-is). AFAICS if you do this, it's just one cache lookups per object, or one indexscan for the cases with no by-OID syscache. It should be much cheaper than the UNION ALL query. And you use pg_shdepend to guide this, so you only do it for the objects that you already know are interesting. -- Álvaro Herrera Valdivia, Chile "La conclusión que podemos sacar de esos estudios es que no podemos sacar ninguna conclusión de ellos" (Tanenbaum)
Re: [PATCH] pg_permissions
On Fri, Mar 26, 2021, at 11:30, Alvaro Herrera wrote: > On 2021-Mar-26, Joel Jacobson wrote: > > > On Thu, Mar 25, 2021, at 17:51, Tom Lane wrote: > > > > I wonder what performance will be like with lots o' objects. > > > > I guess pg_get_acl() would need to be implemented using a switch(classid) > > with 36 cases (one for each class)? > > No, we have a generalized object query mechanism, see objectaddress.c That's where I was looking actually and noticed the switch with 36 cases, in the function getObjectDescription(). /Joel
Re: [PATCH] pg_permissions
On 2021-Mar-26, Joel Jacobson wrote: > On Thu, Mar 25, 2021, at 17:51, Tom Lane wrote: > > I wonder what performance will be like with lots o' objects. > > I guess pg_get_acl() would need to be implemented using a switch(classid) > with 36 cases (one for each class)? No, we have a generalized object query mechanism, see objectaddress.c > Is your performance concern on how such switch statement will be optimized by > the C-compiler? I guess he is concerned about the number of catalog accesses. -- Álvaro Herrera39°49'30"S 73°17'W "Saca el libro que tu religión considere como el indicado para encontrar la oración que traiga paz a tu alma. Luego rebootea el computador y ve si funciona" (Carlos Duclós)
Re: [PATCH] pg_permissions
On Fri, Mar 26, 2021, at 07:53, Joel Jacobson wrote: > On Thu, Mar 25, 2021, at 17:51, Tom Lane wrote: >> "Joel Jacobson" mailto:joel%40compiler.org>> writes: >> > On Thu, Mar 25, 2021, at 16:16, Alvaro Herrera wrote: >> >> Ah, of course -- the only way to obtain the acl columns is by going >> >> through the catalogs individually, so it won't be possible. I think >> >> this could be fixed with some very simple, quick function pg_get_acl() >> >> that takes a catalog OID and object OID and returns the ACL; then >> >> use aclexplode() to obtain all those details. >> >> > +1 for adding pg_get_acl(). >> >> I wonder what performance will be like with lots o' objects. > > I guess pg_get_acl() would need to be implemented using a switch(classid) > with 36 cases (one for each class)? > > Is your performance concern on how such switch statement will be optimized by > the C-compiler? > ... > the classid case values are nicely ordered from > OCLASS_CLASS..OCLASS_TRANSFORM (0..37), so they should produce O(2) fast jump > tables. > > Maybe there is some other performance concern to reason about that I'm > missing here? Hmm, I think I understand your performance concern now: Am I right guessing the problem even with a jump table is going to be branch prediction, which will be poor due to many classids being common? Interesting, the long UNION ALL variant does not seem to suffer from this problem, thanks to explicitly specifying where to find the aclitem/owner-column. We pay the lookup-cost "compile time" when writing the pg_ownerships/pg_permissions system views, instead of having to lookup the classids at run-time to go fetch aclitem/owner-info. The query planner is also smart enough to understand not all the individuals queries needs to be executed, for the use-case when filtering on a specific classid. /Joel
Re: [PATCH] pg_permissions
On Thu, Mar 25, 2021, at 17:51, Tom Lane wrote: > "Joel Jacobson" mailto:joel%40compiler.org>> writes: > > On Thu, Mar 25, 2021, at 16:16, Alvaro Herrera wrote: > >> Ah, of course -- the only way to obtain the acl columns is by going > >> through the catalogs individually, so it won't be possible. I think > >> this could be fixed with some very simple, quick function pg_get_acl() > >> that takes a catalog OID and object OID and returns the ACL; then > >> use aclexplode() to obtain all those details. > > > +1 for adding pg_get_acl(). > > I wonder what performance will be like with lots o' objects. I guess pg_get_acl() would need to be implemented using a switch(classid) with 36 cases (one for each class)? Is your performance concern on how such switch statement will be optimized by the C-compiler? I can see how it would be annoyingly slow if the compiler would pick a branch table or binary search, instead of producing a O(2) fast jump table. On the topic of C switch statements: I think the Clang/GCC-compiler folks (anyone here?) could actually be inspired by PostgreSQL's PerfectHash.pm. I think the same strategy could be used in C compilers to optimize switch statements with sparse case values, which currently produce slow binary search code O(log n) while a perfect hash solution would be O(2). Example showing the unintelligent binary search code produced by GCC: https://godbolt.org/z/1G6G3vcjx (Clang is just as bad.) This is a hypothetical example with sparse case values. This is not the case here, since the classid case values are nicely ordered from OCLASS_CLASS..OCLASS_TRANSFORM (0..37), so they should produce O(2) fast jump tables. Maybe there is some other performance concern to reason about that I'm missing here? /Joel
Re: [PATCH] pg_permissions
"Joel Jacobson" writes: > On Thu, Mar 25, 2021, at 16:16, Alvaro Herrera wrote: >> Ah, of course -- the only way to obtain the acl columns is by going >> through the catalogs individually, so it won't be possible. I think >> this could be fixed with some very simple, quick function pg_get_acl() >> that takes a catalog OID and object OID and returns the ACL; then >> use aclexplode() to obtain all those details. > +1 for adding pg_get_acl(). I wonder what performance will be like with lots o' objects. regards, tom lane
Re: [PATCH] pg_permissions
On Thu, Mar 25, 2021, at 16:16, Alvaro Herrera wrote: > On 2021-Mar-25, Joel Jacobson wrote: > > > pg_shdepend doesn't contain the aclitem info though, > > so it won't work for pg_permissions if we want to expose > > privilege_type, is_grantable and grantor. > > Ah, of course -- the only way to obtain the acl columns is by going > through the catalogs individually, so it won't be possible. I think > this could be fixed with some very simple, quick function pg_get_acl() > that takes a catalog OID and object OID and returns the ACL; then > use aclexplode() to obtain all those details. +1 for adding pg_get_acl(). Do you want to write a patch for that? I could try implementing it otherwise, but would be good with buy-in from some more hackers on if we want these system views at all first. Maybe we can try to decide on that first, i.e. if we want them and what they should return? In the meantime, if people want to try out the views, I've modified the patch to use pg_shdepend for pg_ownerships, while pg_permissions is still UNION ALL. Both views now also use pg_identify_object(). Example usage: CREATE ROLE test_user; CREATE ROLE test_group; CREATE ROLE test_owner; CREATE SCHEMA test AUTHORIZATION test_owner; GRANT ALL ON SCHEMA test TO test_group; GRANT test_group TO test_user; SELECT * FROM pg_permissions WHERE grantor = 'test_owner'::regrole; classid| objid | objsubid | type | schema | name | identity | grantor | grantee | privilege_type | is_grantable --+---+--+++--+--++++-- pg_namespace | 37128 |0 | schema || test | test | test_owner | test_owner | USAGE | f pg_namespace | 37128 |0 | schema || test | test | test_owner | test_owner | CREATE | f pg_namespace | 37128 |0 | schema || test | test | test_owner | test_group | USAGE | f pg_namespace | 37128 |0 | schema || test | test | test_owner | test_group | CREATE | f (4 rows) SET ROLE TO test_user; CREATE TABLE test.a (); RESET ROLE; ALTER TABLE test.a OWNER TO test_owner; SELECT * FROM pg_ownerships WHERE owner = 'test_owner'::regrole; classid | objid | objsubid | type | schema | name | identity | owner -+---+--+++--+--+ 1259 | 37129 |0 | table | test | a| test.a | test_owner 2615 | 37128 |0 | schema || test | test | test_owner (2 rows) GRANT INSERT ON test.a TO test_group; SELECT * FROM pg_permissions WHERE grantee = 'test_group'::regrole; classid| objid | objsubid | type | schema | name | identity | grantor | grantee | privilege_type | is_grantable --+---+--+++--+--++++-- pg_class | 37129 |0 | table | test | a| test.a | test_owner | test_group | INSERT | f pg_namespace | 37128 |0 | schema || test | test | test_owner | test_group | USAGE | f pg_namespace | 37128 |0 | schema || test | test | test_owner | test_group | CREATE | f (3 rows) Looks good or can we improve them further? > > > The semantics will not be entirely the same, > > since internal objects are not tracked in pg_shdepend, > > but I think this is an improvement. > > I just realized that pg_shdepend will not show anything for pinned users > (the bootstrap superuser). I *think* this is not a problem. I also think it's not a problem. Doing a "SELECT * FROM pg_ownerships" would be very noisy if such objects would be included, as all pre-installed catalog objects would show up, but by excluding them, the user will only see relevant ownerships explicitly owned by "real" roles. We would get the same improvement for pg_permissions if pg_shdepend would be use there as well. Right now it's very noisy, as all permissions also for the bootstrap superuser are included. /Joel 0005-pg_permissions-and-pg_ownerships.patch Description: Binary data
Re: [PATCH] pg_permissions
On 2021-Mar-25, Joel Jacobson wrote: > pg_shdepend doesn't contain the aclitem info though, > so it won't work for pg_permissions if we want to expose > privilege_type, is_grantable and grantor. Ah, of course -- the only way to obtain the acl columns is by going through the catalogs individually, so it won't be possible. I think this could be fixed with some very simple, quick function pg_get_acl() that takes a catalog OID and object OID and returns the ACL; then use aclexplode() to obtain all those details. > The semantics will not be entirely the same, > since internal objects are not tracked in pg_shdepend, > but I think this is an improvement. I just realized that pg_shdepend will not show anything for pinned users (the bootstrap superuser). I *think* this is not a problem. -- Álvaro Herrera39°49'30"S 73°17'W "E pur si muove" (Galileo Galilei)
Re: [PATCH] pg_permissions
On Tue, Mar 23, 2021, at 21:39, Alvaro Herrera wrote: >I wonder if these views should be defined on top of pg_shdepend instead >of querying every single catalog. That would make for much shorter >queries. +1 pg_shdepend doesn't contain the aclitem info though, so it won't work for pg_permissions if we want to expose privilege_type, is_grantable and grantor. pg_shdepend should work fine for pg_ownerships though. The semantics will not be entirely the same, since internal objects are not tracked in pg_shdepend, but I think this is an improvement. Example: create role baz; create type foobar as ( foo int, bar boolean ); alter type foobar owner to baz; -- UNION ALL variant: select * from pg_ownerships where owner = 'baz'::regrole; classid | objid | objsubid | owner | type | schema | name | identity --++--+---+++-+- pg_class | 407858 |0 | baz | composite type | public | foobar | public.foobar pg_type | 407860 |0 | baz | type | public | foobar | public.foobar pg_type | 407859 |0 | baz | type | public | _foobar | public.foobar[] (3 rows) -- pg_shdepend variant: select * from pg_ownerships where owner = 'baz'::regrole; classid | objid | objsubid | owner | type | schema | name | identity -++--+---+--+++--- 1247 | 407860 |0 | baz | type | public | foobar | public.foobar (1 row) I'll update the patch. /Joel
Re: [PATCH] pg_permissions
On 2021-Mar-08, Joel Jacobson wrote: > $ dropuser test > dropuser: error: removal of role "test" failed: ERROR: role "test" cannot be > dropped because some objects depend on it > DETAIL: 1 object in database joel > > Hmmm. I wonder which 1 object that could be? BTW the easiest way to find out the answer to this question with current tech is to connect to database joel and attempt "DROP USER joel"; it will print a list of objects the user owns or has privileges for. > # SELECT * FROM pg_ownerships WHERE rolname = 'test'; > # SELECT * FROM pg_permissions WHERE grantee = 'test'; I wonder if these views should be defined on top of pg_shdepend instead of querying every single catalog. That would make for much shorter queries. -- Álvaro Herrera Valdivia, Chile
Re: [PATCH] pg_permissions
On 2021-Mar-23, Joel Jacobson wrote: > On Thu, Mar 11, 2021, at 08:00, Joel Jacobson wrote: > > 0004-pg_permissions-and-pg_ownerships.patch > > Having gotten some hands-on experience of these views for a while, > I notice I quite often want to check the ownerships/permissions > for some specific type of objects, or in some specific schema. > > The current patch returns pg_describe_object() as the "objdesc" column. > > Would it be a better idea to instead return the fields from > pg_identify_object()? > This would allow specifically filtering on "type", "schema", "name" or > "identity" > instead of having to apply a regex/LIKE on the object description. For programmatic use it is certainly better to use the object identity rather than the description. Particularly because the description gets translated, and the identity doesn't. -- Álvaro Herrera Valdivia, Chile
Re: [PATCH] pg_permissions
On Thu, Mar 11, 2021, at 08:00, Joel Jacobson wrote: > 0004-pg_permissions-and-pg_ownerships.patch Having gotten some hands-on experience of these views for a while, I notice I quite often want to check the ownerships/permissions for some specific type of objects, or in some specific schema. The current patch returns pg_describe_object() as the "objdesc" column. Would it be a better idea to instead return the fields from pg_identify_object()? This would allow specifically filtering on "type", "schema", "name" or "identity" instead of having to apply a regex/LIKE on the object description. /Joel
Re: [PATCH] pg_permissions
New version attached. Changes: * Added documentation in catalogs.sgml * Dropped "objsubid" from pg_ownerships since columns have no owner, only tables Do we prefer "pg_permissions" or "pg_privileges"? I can see "privileges" occur 2325 times in the sources, while "permissions" occur only 1097 times. Personally, I would prefer "pg_permissions" since it seems more common in general. "database permissions" gives 195 000 000 results on Google, while "database privileges" only gives 46 800 000 Google results. If we would have consistently used only "privileges" so far, I would vote for "pg_privileges", but since there is already a mix, I think "pg_permissions" would be nicer, it's easier to get right typing also. /Joel 0004-pg_permissions-and-pg_ownerships.patch Description: Binary data
Re: [PATCH] pg_permissions
On Tue, Mar 9, 2021, at 04:01, Chapman Flack wrote: > On Sat, Mar 06, 2021 at 08:03:17PM +0100, Joel Jacobson wrote: > >regclass | obj_desc | grantor | grantee | > privilege_type | is_grantable > > > --+-+-+-++-- > > 1. Is there a reason not to make 'grantor' and 'grantee' of type regrole? >In other words, s/rolname/oid::regrole/ throughout the view definition. >It looks the same visually, but should be easier to build on in a larger >query. > >Hmm, ok, a grantee of 'public' can't be expressed as a regrole. This >seems an annoying little corner.[1] It can be represented by 0::regrole, >but that displays as '-'. Hmm again, you can even '-'::regrole and get 0. > > > 2. Also to facilitate use in a larger query, how about columns for the >objid and objsubid, in addition to the human-friendly obj_desc? >And I'm not sure about using pg_attribute as the regclass for >attributes; it's nice to look at, but could also plant the wrong idea >that attributes have pg_attribute as their classid, when it's really >pg_class with an objsubid. Anyway, there's the human-friendly obj_desc >to tell you it's a column. Thanks for coming up with these two good ideas. I was wrong, they are great. Both have now been implemented. New patch attached. Example usage: CREATE ROLE test_user; CREATE ROLE test_group; CREATE ROLE test_owner; CREATE SCHEMA test AUTHORIZATION test_owner; GRANT ALL ON SCHEMA test TO test_group; GRANT test_group TO test_user; SELECT * FROM pg_permissions WHERE grantor = 'test_owner'::regrole; classid| objid | objsubid | objdesc | grantor | grantee | privilege_type | is_grantable --+---+--+-++++-- pg_namespace | 16390 |0 | schema test | test_owner | test_owner | USAGE | f pg_namespace | 16390 |0 | schema test | test_owner | test_owner | CREATE | f pg_namespace | 16390 |0 | schema test | test_owner | test_group | USAGE | f pg_namespace | 16390 |0 | schema test | test_owner | test_group | CREATE | f (4 rows) SET ROLE TO test_user; CREATE TABLE test.a (); RESET ROLE; SELECT * FROM pg_ownerships WHERE owner = 'test_owner'::regrole; classid| objid | objsubid | objdesc | owner --+---+--+-+ pg_namespace | 16390 |0 | schema test | test_owner (1 row) ALTER TABLE test.a OWNER TO test_owner; SELECT * FROM pg_ownerships WHERE owner = 'test_owner'::regrole; classid| objid | objsubid | objdesc | owner --+---+--+-+ pg_class | 16391 |0 | table a | test_owner pg_namespace | 16390 |0 | schema test | test_owner pg_type | 16393 |0 | type a | test_owner pg_type | 16392 |0 | type a[]| test_owner (4 rows) GRANT INSERT ON test.a TO test_group; SELECT * FROM pg_permissions WHERE grantee = 'test_group'::regrole; classid| objid | objsubid | objdesc | grantor | grantee | privilege_type | is_grantable --+---+--+-++++-- pg_class | 16391 |0 | table a | test_owner | test_group | INSERT | f pg_namespace | 16390 |0 | schema test | test_owner | test_group | USAGE | f pg_namespace | 16390 |0 | schema test | test_owner | test_group | CREATE | f (3 rows) /Joel 0003-pg_permissions-and-pg_ownerships.patch Description: Binary data
Re: [PATCH] pg_permissions
On 03/09/21 11:11, Joel Jacobson wrote: > On Tue, Mar 9, 2021, at 07:34, Joel Jacobson wrote: >> On Tue, Mar 9, 2021, at 04:01, Chapman Flack wrote: >>> 1. Is there a reason not to make 'grantor' and 'grantee' of type regrole? > > Having digested your idea, I actually agree with you. > > Since we have the regrole-type, I agree we should use it, > even though we need to cast, no biggie. This does highlight [topicshift] one sort of inconvenience I've observed before in other settings: how fussy it may be to write WHERE grantee = 'bob' when there is no user 'bob'. A simple cast 'bob'::regrole raises undefined_object (in class "Syntax Error or Access Rule Violation") rather than just returning no rows because no grantee is bob. It's a more general issue: I first noticed it when I had proudly implemented my first PostgreSQL type foo that would only accept valid foos as values, and the next thing that happened was my colleague in frontend development wrote mean Python comments about me because he couldn't simply search for a foo in a table without either first duplicating the validation of the value or trapping the error if the user had entered a non-foo to search for. We could solve that, of course, by implementing = and <> (foo,text) to simply return false (resp. true) if the text arg isn't castable to foo. But the naïve way of writing such an operator repeats the castability test for every row compared. If I were to build such an operator now, I might explore whether a planner support function could be used to check the castability once, and replace the whole comparison with constant false if that fails. And this strikes me as a situation that might be faced often enough to wonder if some kind of meta-support-function would be worth supplying that could do that for any type foo. [/topicshift] Regards, -Chap
Re: [PATCH] pg_permissions
On Tue, Mar 9, 2021, at 07:34, Joel Jacobson wrote: > On Tue, Mar 9, 2021, at 04:01, Chapman Flack wrote: >> 1. Is there a reason not to make 'grantor' and 'grantee' of type regrole? Having digested your idea, I actually agree with you. Since we have the regrole-type, I agree we should use it, even though we need to cast, no biggie. I realized my arguments were silly since I already exposed the class as regclass, which has the same problem. I'll send a new patch soon. /Joel
Re: [PATCH] pg_permissions
On Tue, Mar 9, 2021, at 04:01, Chapman Flack wrote: > On Sat, Mar 06, 2021 at 08:03:17PM +0100, Joel Jacobson wrote: > >regclass | obj_desc | grantor | grantee | > privilege_type | is_grantable > > > --+-+-+-++-- > > 1. Is there a reason not to make 'grantor' and 'grantee' of type regrole? I considered it, but this view is tailored for human-use, to be used by experienced as well as beginner users. >In other words, s/rolname/oid::regrole/ throughout the view definition. >It looks the same visually, but should be easier to build on in a larger >query. If using regrole, the users would need to know they would need to cast it to text, to search for values, e.g.: SELECT * FROM pg_permissions WHERE grantee = 'foobar'; ERROR: invalid input syntax for type oid: "foobar" LINE 1: SELECT * FROM pg_permissions WHERE grantee = 'foobar'; SELECT * FROM pg_permissions WHERE grantee LIKE 'foo%'; ERROR: operator does not exist: regrole ~~ unknown LINE 1: SELECT * FROM pg_permissions WHERE grantee LIKE 'foo%'; > 2. Also to facilitate use in a larger query, how about columns for the >objid and objsubid, in addition to the human-friendly obj_desc? I think it's good to prevent users from abusing this view, by not including oids and other columns needed for proper integration in larger queries/systems. Otherwise there is a risk users will be sloppy and just join pg_permissions, when they really should be joining some specific catalog. Also, lots of extra columns not needed by humans just makes the view less readable, since you would more often need to \x when the width of the output does't fit. Personally, I'm on a 15" MacBook Pro and I usually have two 117x24 terminals next to each other, in which both pg_permissions and pg_ownerships output usually fits fine. >And I'm not sure about using pg_attribute as the regclass for >attributes; it's nice to look at, but could also plant the wrong idea >that attributes have pg_attribute as their classid, when it's really >pg_class with an objsubid. Anyway, there's the human-friendly obj_desc >to tell you it's a column. While pg_class is the "origin class", I think we convey more meaningful information, by using the regclass for the table which stores the aclitem[] column, in your example, pg_attribute. This makes it more obvious to the user the permission is on some column, rather than on the table. In the case where you try to drop a user and don't understand why you can't, and then look in pg_permissions what could be the reason, it's more helpful to show pg_attribute than pg_class, since you hopefully then understand you should revoke permissions for some column, and not the table. You get this information in obj_desc as well, but I think regclass complements it nicely. And it's also more precise, the permission *is* really on pg_attribute, it just happens to be that pg_attribute has a multi-key primary key, where one of the keys is referencing pg_class.oid. > But I think it would be useful for this view to handle the part of the story > that involves acldefault() when the stored aclitem[] is null. I've long > wanted a view that actually shows you all of the permissions that apply > to something, even the ones you're supposed to Just Know, and indeed > I wrote such a thing for $work. > Then you could even query the view for an answer to the question "what > are all the permissions 'public' (or '-') can exercise here?" Seems useful, but maybe that's a different view/function? Could it be integrated into these views without increasing complexity? /Joel
Re: [PATCH] pg_permissions
On Sat, Mar 06, 2021 at 08:03:17PM +0100, Joel Jacobson wrote: >regclass | obj_desc | grantor | grantee | privilege_type | is_grantable > --+-+-+-++-- 1. Is there a reason not to make 'grantor' and 'grantee' of type regrole? In other words, s/rolname/oid::regrole/ throughout the view definition. It looks the same visually, but should be easier to build on in a larger query. Hmm, ok, a grantee of 'public' can't be expressed as a regrole. This seems an annoying little corner.[1] It can be represented by 0::regrole, but that displays as '-'. Hmm again, you can even '-'::regrole and get 0. 2. Also to facilitate use in a larger query, how about columns for the objid and objsubid, in addition to the human-friendly obj_desc? And I'm not sure about using pg_attribute as the regclass for attributes; it's nice to look at, but could also plant the wrong idea that attributes have pg_attribute as their classid, when it's really pg_class with an objsubid. Anyway, there's the human-friendly obj_desc to tell you it's a column. On 03/08/21 12:14, Joel Jacobson wrote: > On Mon, Mar 8, 2021, at 15:35, Joe Conway wrote: >> While this is interesting and probably useful for troubleshooting, it does >> not >> provide the complete picture if what you care about is something like "what >> stuff can joel do in my database". > > Good point, I agree. > > I think that's a different more complicated use-case though. I could agree that the role membership and inherit/noinherit part is a more complicated problem that could be solved by a larger query built over this view (facilitated by giving grantor and grantee regrole type) and some recursive-CTEness with the roles. But I think it would be useful for this view to handle the part of the story that involves acldefault() when the stored aclitem[] is null. I've long wanted a view that actually shows you all of the permissions that apply to something, even the ones you're supposed to Just Know, and indeed I wrote such a thing for $work. Then you could even query the view for an answer to the question "what are all the permissions 'public' (or '-') can exercise here?" On 03/06/21 19:08, Joel Jacobson wrote: > SELECT * FROM ownerships WHERE rolname = 'joel' LIMIT 5; > regclass | obj_desc | rolname > --+---+- Here again, I'd repeat the suggestions to present the owner as a regrole (and in this case there is no need to deal with 'public'), and to include the objid as well as the human-friendly obj_desc. Regards, -Chap
Re: [PATCH] pg_permissions
On Mon, Mar 8, 2021, at 15:35, Joe Conway wrote: > While this is interesting and probably useful for troubleshooting, it does not > provide the complete picture if what you care about is something like "what > stuff can joel do in my database". Good point, I agree. I think that's a different more complicated use-case though. Personally, I use these views to resolve errors like this: $ dropuser test dropuser: error: removal of role "test" failed: ERROR: role "test" cannot be dropped because some objects depend on it DETAIL: 1 object in database joel Hmmm. I wonder which 1 object that could be? $ psql # SELECT * FROM pg_ownerships WHERE rolname = 'test'; regclass | obj_desc | rolname --+--+- pg_class | table t | test pg_type | type t | test pg_type | type t[] | test (3 rows) It could also be due to permissions, so normally I would check both pg_ownership and pg_permissions at the same time, since otherwise I could possibly get the same error again: $ dropuser test dropuser: error: removal of role "test" failed: ERROR: role "test" cannot be dropped because some objects depend on it DETAIL: 1 object in database joel # SELECT * FROM pg_permissions WHERE grantee = 'test'; regclass | obj_desc | grantor | grantee | privilege_type | is_grantable --+--+-+-++-- pg_class | table t | joel| test| INSERT | f (1 row) Now, this situation is probably easiest to quickly resolve using REASSIGN OWNED BY ... TO ... but I think that command is scary, I would rather prefer to resolve it manually to not blindly cause problems. /Joel
Re: [PATCH] pg_permissions
On 3/6/21 2:03 PM, Joel Jacobson wrote: > ...but to answer the question... > > - What permissions are there for a specific role in the database? > > you need to manually query all relevant pg_catalog or > information_schema.*_privileges views, > which is a O(n) mental effort, while the first question is mentally O(1). > > I think this can be improved by providing humans a single pg_permissions > system view > which can be queried to answer the second question. This should save a lot of > keyboard punches. While this is interesting and probably useful for troubleshooting, it does not provide the complete picture if what you care about is something like "what stuff can joel do in my database". The reasons for this include default grants to PUBLIC and role membership, and even that is convoluted by INHERIT/NOINHERIT role attributes. I won't try to describe all the implications here, but a while back I wrote a fairly comprehensive blog[1] about it. FWIW in the blog I reference an extension that I wrote to facilitate object and role privilege inspection[2]. I have toyed with the idea of morphing that into a feature I can submit for pg15, but I don't want to spend effort on the morphing unless there is both sufficient interest and lack of conceptual objections to the feature. I'd love to hear from both sides of that scale. Joe [1] http://blog.crunchydata.com/blog/postgresql-defaults-and-impact-on-security-part-1 [2] https://github.com/CrunchyData/crunchy_check_access -- Crunchy Data - http://crunchydata.com PostgreSQL Support for Secure Enterprises Consulting, Training, & Open Source Development
Re: [PATCH] pg_permissions
On Mon, Mar 8, 2021, at 07:28, Joel Jacobson wrote: >Attached is a new patch with both pg_permissions and pg_ownerships in the same >patch, >based on HEAD (8a812e5106c5db50039336288d376a188844e2cc). > >Attachments: >0001-pg_permissions-and-pg_ownerships.patch I forgot to update src/test/regress/expected/rules.out. New patch attached. /Joel 0002-pg_permissions-and-pg_ownerships.patch Description: Binary data
Re: [PATCH] pg_permissions
On Mon, Mar 8, 2021, at 02:09, David Fetter wrote:
> +1 for both this and the ownerships view.
>
> Best,
> David.
I'm glad you like it.
I've put some more effort into this patch, and developed a method to
mechanically verify its correctness.
Attached is a new patch with both pg_permissions and pg_ownerships in the same
patch,
based on HEAD (8a812e5106c5db50039336288d376a188844e2cc).
I've also added five catalogs to pg_ownerships that were discovered to be
missing in the previous version:
pg_catalog.pg_database
pg_catalog.pg_default_acl
pg_catalog.pg_largeobject_metadata
pg_catalog.pg_publication
pg_catalog.pg_subscription
Here is how I've verified correctness of complete coverage:
All catalogs with permissions have an aclitem[] column.
There are totally 13 such catalogs in HEAD:
SELECT COUNT(DISTINCT table_name) FROM information_schema.columns WHERE
table_schema = 'pg_catalog' AND udt_name = '_aclitem';
count
---
13
(1 row)
Expect the same number of rows in the patch:
$ grep "(aclexplode(aa." 0001-pg_permissions-and-pg_ownerships.patch | wc -l
13
Using the new awesome pg_get_catalog_foreign_keys() function in v14,
we can now query what catalogs are referencing pg_authid.oid,
of which all named .*owner are known by convention to
indicate ownership. Let's see what other columns there are
referencing pg_authid.oid that could possibly also indicate ownership:
SELECT
regexp_replace(fkcols[1],'.*owner$','.*owner') AS fkcol,
COUNT(*)
FROM pg_get_catalog_foreign_keys()
WHERE pktable = 'pg_authid'::regclass
AND pkcols[1] = 'oid'
AND cardinality(fkcols) = 1
GROUP BY 1
ORDER BY 2 DESC;
fkcol| count
+---
.*owner|21
datdba | 1
defaclrole | 1
grantor| 1
member | 1
polroles | 1
roleid | 1
setrole| 1
umuser | 1
(9 rows)
If we exclude the .*owner and also look at fktable we see:
SELECT *
FROM pg_get_catalog_foreign_keys()
WHERE pktable = 'pg_authid'::regclass
AND pkcols[1] = 'oid'
AND cardinality(fkcols) = 1
AND fkcols[1] !~ '.*owner$'
fktable |fkcols| pktable | pkcols | is_array | is_opt
+--+---++--+
pg_database| {datdba} | pg_authid | {oid} | f| f
pg_db_role_setting | {setrole}| pg_authid | {oid} | f| t
pg_auth_members| {roleid} | pg_authid | {oid} | f| f
pg_auth_members| {member} | pg_authid | {oid} | f| f
pg_auth_members| {grantor}| pg_authid | {oid} | f| f
pg_user_mapping| {umuser} | pg_authid | {oid} | f| t
pg_policy | {polroles} | pg_authid | {oid} | t| t
pg_default_acl | {defaclrole} | pg_authid | {oid} | f| f
(8 rows)
By reading the documentation for these catalogs,
I've come to the conclusion these columns also indicate ownership:
pg_database.datdba
pg_default_acl.defaclrole
pg_policy.polroles
In total, we should expect 21+3=24 catalogs.
Let's see if this matches the patch:
$ grep "pg_authid.rolname" 0001-pg_permissions-and-pg_ownerships.patch | wc -l
24
All good.
I note it's not very often new catalogs are added,
so hopefully we can have a routine to update these views
when new catalogs with ownership- or permission columns are added.
However, should we ever get out of sync, we can use the method above to sort
things out.
/Joel
0001-pg_permissions-and-pg_ownerships.patch
Description: Binary data
Re: [PATCH] pg_permissions
On Sat, Mar 06, 2021 at 08:03:17PM +0100, Joel Jacobson wrote: > Hi, > > It's easy to answer the question... > >- What permissions are there on this specific object? > > ...but to answer the question... > >- What permissions are there for a specific role in the database? > > you need to manually query all relevant pg_catalog or > information_schema.*_privileges views, > which is a O(n) mental effort, while the first question is mentally O(1). > > I think this can be improved by providing humans a single pg_permissions > system view > which can be queried to answer the second question. This should save a lot of > keyboard punches. > > Demo: > > SELECT * FROM pg_permissions WHERE 'joel' IN (grantor,grantee); >regclass | obj_desc | grantor | grantee | > privilege_type | is_grantable > --+-+-+-++-- > pg_namespace | schema foo | joel| joel| USAGE > | f > pg_namespace | schema foo | joel| joel| CREATE > | f > pg_class | table foo.bar | joel| joel| INSERT > | f > pg_class | table foo.bar | joel| joel| SELECT > | f > pg_class | table foo.bar | joel| joel| UPDATE > | f > pg_class | table foo.bar | joel| joel| DELETE > | f > pg_class | table foo.bar | joel| joel| TRUNCATE > | f > pg_class | table foo.bar | joel| joel| REFERENCES > | f > pg_class | table foo.bar | joel| joel| TRIGGER > | f > pg_attribute | column baz of table foo.bar | joel| joel| SELECT > | f > pg_attribute | column baz of table foo.bar | joel| joel| UPDATE > | f > (11 rows) > > All catalogs with _aclitem columns have been included in the view. > > I think a similar one for ownerships would be nice too. > But I'll let you digest this one first to see if the concept is fruitful. +1 for both this and the ownerships view. Best, David. -- David Fetter http://fetter.org/ Phone: +1 415 235 3778 Remember to vote! Consider donating to Postgres: http://www.postgresql.org/about/donate
