Thread: Re: [PATCH] pg_permissions

Re: [PATCH] pg_permissions

From

Alvaro Herrera

Date:

23 March 2021, 23:39:37

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

From

"Joel Jacobson"

Date:

25 March 2021, 12:48:55

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.

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;

----------+--------+----------+-------+----------------+--------+---------+-----------------

(3 rows)

-- pg_shdepend variant:

select * from pg_ownerships where owner = 'baz'::regrole;

---------+--------+----------+-------+------+--------+--------+---------------

1247 | 407860 | 0 | baz | type | public | foobar | public.foobar

(1 row)

I'll update the patch.

/Joel

Re: [PATCH] pg_permissions

From

Alvaro Herrera

Date:

25 March 2021, 18:16:37

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 Herrera                            39°49'30"S 73°17'W
"E pur si muove" (Galileo Galilei)

Re: [PATCH] pg_permissions

From

"Joel Jacobson"

Date:

25 March 2021, 19:46:21

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;

--------------+-------+----------+--------+--------+------+----------+------------+------------+----------------+--------------

(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;

---------+-------+----------+--------+--------+------+----------+------------

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;

--------------+-------+----------+--------+--------+------+----------+------------+------------+----------------+--------------

(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

Attachment

0005-pg_permissions-and-pg_ownerships.patch

Re: [PATCH] pg_permissions

From

Tom Lane

Date:

25 March 2021, 19:51:55

"Joel Jacobson" <joel@compiler.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.

            regards, tom lane

Re: [PATCH] pg_permissions

From

"Joel Jacobson"

Date:

26 March 2021, 09:53:04

On Thu, Mar 25, 2021, at 17:51, Tom Lane wrote:

"Joel Jacobson" <joel@compiler.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

From

"Joel Jacobson"

Date:

26 March 2021, 12:38:15

On Fri, Mar 26, 2021, at 07:53, Joel Jacobson wrote:

On Thu, Mar 25, 2021, at 17:51, Tom Lane wrote:
"Joel Jacobson" <joel@compiler.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