Discussion:
Simplify calls of pg_class_aclcheck when multiple modes are used
Michael Paquier
2014-08-27 12:02:40 UTC
Permalink
Hi all,

In a couple of code paths we do the following to check permissions on an
object:
if (pg_class_aclcheck(relid, userid, ACL_USAGE) != ACLCHECK_OK &&
pg_class_aclcheck(relid, userid, ACL_UPDATE) != ACLCHECK_OK)
ereport(ERROR, blah);

Wouldn't it be better to simplify that with a single call of
pg_class_aclcheck, gathering together the modes that need to be checked? In
the case above, the call to pg_class_aclcheck would become like that:
if (pg_class_aclcheck(relid, userid,
ACL_USAGE | ACL_UPDATE) != ACLCHECK_OK)
ereport(ERROR, blah);

That's not a critical thing, but it would save some cycles. Patch is
attached.
Regards,
--
Michael
Fabrízio de Royes Mello
2014-10-06 17:58:11 UTC
Permalink
Post by Michael Paquier
Hi all,
In a couple of code paths we do the following to check permissions on an
if (pg_class_aclcheck(relid, userid, ACL_USAGE) != ACLCHECK_OK &&
pg_class_aclcheck(relid, userid, ACL_UPDATE) != ACLCHECK_OK)
ereport(ERROR, blah);
Wouldn't it be better to simplify that with a single call of
pg_class_aclcheck, gathering together the modes that need to be checked? In
Post by Michael Paquier
if (pg_class_aclcheck(relid, userid,
ACL_USAGE | ACL_UPDATE) != ACLCHECK_OK)
ereport(ERROR, blah);
That's not a critical thing, but it would save some cycles. Patch is
attached.
I did a little review:
- applied to master without errors
- no compiler warnings
- no regressions

It's a minor change and as Michael already said would save some cycles.

Marked as "Ready for commiter".

Regards,

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
Post by Michael Paquier
Timbira: http://www.timbira.com.br
Blog: http://fabriziomello.github.io
Linkedin: http://br.linkedin.com/in/fabriziomello
Twitter: http://twitter.com/fabriziomello
Github: http://github.com/fabriziomello
Peter Eisentraut
2014-10-21 20:03:09 UTC
Permalink
Post by Michael Paquier
In a couple of code paths we do the following to check permissions on an
if (pg_class_aclcheck(relid, userid, ACL_USAGE) != ACLCHECK_OK &&
pg_class_aclcheck(relid, userid, ACL_UPDATE) != ACLCHECK_OK)
ereport(ERROR, blah);
Wouldn't it be better to simplify that with a single call of
pg_class_aclcheck, gathering together the modes that need to be checked?
Yes, it's probably just an oversight.

While looking at this, I wrote a few tests cases for sequence
privileges, because that was not covered at all. That patch is attached.

That led me to discover this issue:
http://www.postgresql.org/message-id/***@gmx.net

I'll wait for the resolution of that and then commit this.
Michael Paquier
2014-10-21 22:19:01 UTC
Permalink
Post by Peter Eisentraut
While looking at this, I wrote a few tests cases for sequence
privileges, because that was not covered at all. That patch is attached.
+1 for those tests.
--
Michael
Fabrízio de Royes Mello
2014-10-21 22:29:13 UTC
Permalink
Em terça-feira, 21 de outubro de 2014, Michael Paquier <
Post by Michael Paquier
Post by Peter Eisentraut
While looking at this, I wrote a few tests cases for sequence
privileges, because that was not covered at all. That patch is attached.
+1 for those tests.
+1

Fabrízio Mello
--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
Post by Michael Paquier
Post by Peter Eisentraut
Timbira: http://www.timbira.com.br
Blog: http://fabriziomello.github.io
Linkedin: http://br.linkedin.com/in/fabriziomello
Twitter: http://twitter.com/fabriziomello
Github: http://github.com/fabriziomello
Peter Eisentraut
2014-10-23 01:45:52 UTC
Permalink
Post by Peter Eisentraut
While looking at this, I wrote a few tests cases for sequence
privileges, because that was not covered at all. That patch is attached.
+1 for those tests.
--
Michael
Committed your patch and tests.
--
Sent via pgsql-hackers mailing list (pgsql-***@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Michael Paquier
2014-10-23 14:48:01 UTC
Permalink
Post by Peter Eisentraut
Committed your patch and tests.
Thanks!
--
Michael
Loading...