Re: [HACKERS] Simplify calls of pg_class_aclcheck when multiple modes are used

[Michael Paquier]

On Thu, Oct 23, 2014 at 10:45 AM, Peter Eisentraut pete...@gmx.net wrote:

 Committed your patch and tests.

Thanks!
-- 
Michael

Re: [HACKERS] Simplify calls of pg_class_aclcheck when multiple modes are used

[Peter Eisentraut]

On 10/21/14 6:19 PM, Michael Paquier wrote:
 On Wed, Oct 22, 2014 at 5:03 AM, Peter Eisentraut pete...@gmx.net
 mailto:pete...@gmx.net wrote:
 
 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-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Simplify calls of pg_class_aclcheck when multiple modes are used

[Peter Eisentraut]

On 8/27/14 8:02 AM, Michael Paquier wrote:
 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?

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/5446b819.1020...@gmx.net

I'll wait for the resolution of that and then commit this.

diff --git a/src/test/regress/expected/sequence.out b/src/test/regress/expected/sequence.out
index a27b5fd..8783ca6 100644
--- a/src/test/regress/expected/sequence.out
+++ b/src/test/regress/expected/sequence.out
@@ -367,6 +367,41 @@ DROP SEQUENCE seq2;
 SELECT lastval();
 ERROR:  lastval is not yet defined in this session
 CREATE USER seq_user;
+-- privileges tests
+-- nextval
+BEGIN;
+SET LOCAL SESSION AUTHORIZATION seq_user;
+CREATE SEQUENCE seq3;
+REVOKE ALL ON seq3 FROM seq_user;
+GRANT SELECT ON seq3 TO seq_user;
+SELECT nextval('seq3');
+ERROR:  permission denied for sequence seq3
+ROLLBACK;
+BEGIN;
+SET LOCAL SESSION AUTHORIZATION seq_user;
+CREATE SEQUENCE seq3;
+REVOKE ALL ON seq3 FROM seq_user;
+GRANT UPDATE ON seq3 TO seq_user;
+SELECT nextval('seq3');
+ nextval 
+-
+   1
+(1 row)
+
+ROLLBACK;
+BEGIN;
+SET LOCAL SESSION AUTHORIZATION seq_user;
+CREATE SEQUENCE seq3;
+REVOKE ALL ON seq3 FROM seq_user;
+GRANT USAGE ON seq3 TO seq_user;
+SELECT nextval('seq3');
+ nextval 
+-
+   1
+(1 row)
+
+ROLLBACK;
+-- currval
 BEGIN;
 SET LOCAL SESSION AUTHORIZATION seq_user;
 CREATE SEQUENCE seq3;
@@ -377,9 +412,97 @@ SELECT nextval('seq3');
 (1 row)
 
 REVOKE ALL ON seq3 FROM seq_user;
+GRANT SELECT ON seq3 TO seq_user;
+SELECT currval('seq3');
+ currval 
+-
+   1
+(1 row)
+
+ROLLBACK;
+BEGIN;
+SET LOCAL SESSION AUTHORIZATION seq_user;
+CREATE SEQUENCE seq3;
+SELECT nextval('seq3');
+ nextval 
+-
+   1
+(1 row)
+
+REVOKE ALL ON seq3 FROM seq_user;
+GRANT UPDATE ON seq3 TO seq_user;
+SELECT currval('seq3');
+ERROR:  permission denied for sequence seq3
+ROLLBACK;
+BEGIN;
+SET LOCAL SESSION AUTHORIZATION seq_user;
+CREATE SEQUENCE seq3;
+SELECT nextval('seq3');
+ nextval 
+-
+   1
+(1 row)
+
+REVOKE ALL ON seq3 FROM seq_user;
+GRANT USAGE ON seq3 TO seq_user;
+SELECT currval('seq3');
+ currval 
+-
+   1
+(1 row)
+
+ROLLBACK;
+-- lastval
+BEGIN;
+SET LOCAL SESSION AUTHORIZATION seq_user;
+CREATE SEQUENCE seq3;
+SELECT nextval('seq3');
+ nextval 
+-
+   1
+(1 row)
+
+REVOKE ALL ON seq3 FROM seq_user;
+GRANT SELECT ON seq3 TO seq_user;
+SELECT lastval();
+ lastval 
+-
+   1
+(1 row)
+
+ROLLBACK;
+BEGIN;
+SET LOCAL SESSION AUTHORIZATION seq_user;
+CREATE SEQUENCE seq3;
+SELECT nextval('seq3');
+ nextval 
+-
+   1
+(1 row)
+
+REVOKE ALL ON seq3 FROM seq_user;
+GRANT UPDATE ON seq3 TO seq_user;
 SELECT lastval();
 ERROR:  permission denied for sequence seq3
 ROLLBACK;
+BEGIN;
+SET LOCAL SESSION AUTHORIZATION seq_user;
+CREATE SEQUENCE seq3;
+SELECT nextval('seq3');
+ nextval 
+-
+   1
+(1 row)
+
+REVOKE ALL ON seq3 FROM seq_user;
+GRANT USAGE ON seq3 TO seq_user;
+SELECT lastval();
+ lastval 
+-
+   1
+(1 row)
+
+ROLLBACK;
 -- Sequences should get wiped out as well:
 DROP TABLE serialTest, serialTest2;
 -- Make sure sequences are gone:
diff --git a/src/test/regress/sql/sequence.sql b/src/test/regress/sql/sequence.sql
index 8d3b700..0dd653d 100644
--- a/src/test/regress/sql/sequence.sql
+++ b/src/test/regress/sql/sequence.sql
@@ -168,11 +168,86 @@ CREATE SEQUENCE seq2;
 
 CREATE USER seq_user;
 
+-- privileges tests
+
+-- nextval
+BEGIN;
+SET LOCAL SESSION AUTHORIZATION seq_user;
+CREATE SEQUENCE seq3;
+REVOKE ALL ON seq3 FROM seq_user;
+GRANT SELECT ON seq3 TO seq_user;
+SELECT nextval('seq3');
+ROLLBACK;
+
+BEGIN;
+SET LOCAL SESSION AUTHORIZATION seq_user;
+CREATE SEQUENCE seq3;
+REVOKE ALL ON seq3 FROM seq_user;
+GRANT UPDATE ON seq3 TO seq_user;
+SELECT nextval('seq3');
+ROLLBACK;
+
+BEGIN;
+SET LOCAL SESSION AUTHORIZATION seq_user;
+CREATE SEQUENCE seq3;
+REVOKE ALL ON seq3 FROM seq_user;
+GRANT USAGE ON seq3 TO seq_user;
+SELECT nextval('seq3');
+ROLLBACK;
+
+-- currval
+BEGIN;
+SET LOCAL SESSION AUTHORIZATION seq_user;
+CREATE SEQUENCE seq3;
+SELECT nextval('seq3');
+REVOKE ALL ON seq3 FROM seq_user;
+GRANT SELECT ON seq3 TO seq_user;
+SELECT currval('seq3');
+ROLLBACK;
+
+BEGIN;
+SET LOCAL SESSION AUTHORIZATION seq_user;
+CREATE SEQUENCE seq3;
+SELECT nextval('seq3');
+REVOKE ALL ON seq3 FROM seq_user;
+GRANT UPDATE ON seq3 TO seq_user;
+SELECT currval('seq3');
+ROLLBACK;
+
+BEGIN;
+SET LOCAL SESSION AUTHORIZATION seq_user;
+CREATE 

Re: [HACKERS] Simplify calls of pg_class_aclcheck when multiple modes are used

[Michael Paquier]

On Wed, Oct 22, 2014 at 5:03 AM, Peter Eisentraut pete...@gmx.net wrote:

 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

Re: [HACKERS] Simplify calls of pg_class_aclcheck when multiple modes are used

[Fabrízio de Royes Mello]

Em terça-feira, 21 de outubro de 2014, Michael Paquier 
michael.paqu...@gmail.com escreveu:

 On Wed, Oct 22, 2014 at 5:03 AM, Peter Eisentraut pete...@gmx.net
 javascript:_e(%7B%7D,'cvml','pete...@gmx.net'); wrote:

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

Re: [HACKERS] Simplify calls of pg_class_aclcheck when multiple modes are used

[Fabrízio de Royes Mello]

On Wed, Aug 27, 2014 at 9:02 AM, Michael Paquier michael.paqu...@gmail.com
wrote:

 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.


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

[HACKERS] Simplify calls of pg_class_aclcheck when multiple modes are used

[Michael Paquier]

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
From e6b23e537d223e4bdb3abada2d761e630c8b27c0 Mon Sep 17 00:00:00 2001
From: Michael Paquier mich...@otacoo.com
Date: Wed, 27 Aug 2014 20:45:31 +0900
Subject: [PATCH] Minimize calls of pg_class_aclcheck to minimum necessary

In a couple of code paths, pg_class_aclcheck is called in succession with
multiple different modes set. This patch combines those modes to have a
single call of this function and reduce a bit process overhead for
permission checking.
---
 src/backend/commands/sequence.c | 12 ++--
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/src/backend/commands/sequence.c b/src/backend/commands/sequence.c
index 3b89dd0..6d5f65b 100644
--- a/src/backend/commands/sequence.c
+++ b/src/backend/commands/sequence.c
@@ -546,8 +546,8 @@ nextval_internal(Oid relid)
 	/* open and AccessShareLock sequence */
 	init_sequence(relid, elm, seqrel);
 
-	if (pg_class_aclcheck(elm-relid, GetUserId(), ACL_USAGE) != ACLCHECK_OK 
-		pg_class_aclcheck(elm-relid, GetUserId(), ACL_UPDATE) != ACLCHECK_OK)
+	if (pg_class_aclcheck(elm-relid, GetUserId(),
+		  ACL_USAGE | ACL_UPDATE) != ACLCHECK_OK)
 		ereport(ERROR,
 (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
  errmsg(permission denied for sequence %s,
@@ -759,8 +759,8 @@ currval_oid(PG_FUNCTION_ARGS)
 	/* open and AccessShareLock sequence */
 	init_sequence(relid, elm, seqrel);
 
-	if (pg_class_aclcheck(elm-relid, GetUserId(), ACL_SELECT) != ACLCHECK_OK 
-		pg_class_aclcheck(elm-relid, GetUserId(), ACL_USAGE) != ACLCHECK_OK)
+	if (pg_class_aclcheck(elm-relid, GetUserId(),
+		  ACL_SELECT | ACL_USAGE) != ACLCHECK_OK)
 		ereport(ERROR,
 (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
  errmsg(permission denied for sequence %s,
@@ -801,8 +801,8 @@ lastval(PG_FUNCTION_ARGS)
 	/* nextval() must have already been called for this sequence */
 	Assert(last_used_seq-last_valid);
 
-	if (pg_class_aclcheck(last_used_seq-relid, GetUserId(), ACL_SELECT) != ACLCHECK_OK 
-		pg_class_aclcheck(last_used_seq-relid, GetUserId(), ACL_USAGE) != ACLCHECK_OK)
+	if (pg_class_aclcheck(last_used_seq-relid, GetUserId(),
+		  ACL_SELECT | ACL_USAGE) != ACLCHECK_OK)
 		ereport(ERROR,
 (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
  errmsg(permission denied for sequence %s,
-- 
2.1.0


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers