Message-ID: <49E809F6.8070008@ak.jp.nec.com>
Date: Fri, 17 Apr 2009 13:47:50 +0900
From: KaiGai Kohei <kaigai@ak.jp.nec.com>
User-Agent: Thunderbird 2.0.0.6 (Windows/20070728)
MIME-Version: 1.0
To: pgsql-hackers@postgresql.org
Subject: [PATCH] unalias of ACL_SELECT_FOR_UPDATE
Content-Type: multipart/mixed; boundary="------------070900010300020607070305"

This is a multi-part message in MIME format.
--------------070900010300020607070305
Content-Type: text/plain; charset=ISO-2022-JP
Content-Transfer-Encoding: 7bit

Currently, the ACL_SELECT_FOR_UPDATE privilege is defined as an alias
of ACL_UPDATE as follows:

  at src/include/nodes/parsenodes.h:
           :
  /* Currently, SELECT ... FOR UPDATE/FOR SHARE requires UPDATE privileges */
  #define ACL_SELECT_FOR_UPDATE   ACL_UPDATE
           :

It is unconfortable for us because SE-PostgreSQL have two individual
permissions for updates (db_table:{update}) and explicit table locks
(db_table:{lock}), but it unables to discriminate whether the given
relation is actually used for UPDATE or SELECT FOR UPDATE.
So, I would like to define the bit as (1<<12) which is not in use.

I think we have two options to solve the matter.

The one is to check ACL_SELECT_FOR_UPDATE by UPDATE privilege, as
the attached patch doing. It implicitly expands the users privilege
when he has ACL_UPDATE, so it enables GRANT UPDATE to cover both of
ACL_UPDATE and ACL_SELECT_FOR_UPDATE as the current implementation
doing.

The other is to add a new privilege for explicit table locks, such
as something like LOCK privilege. It is a straightforward approach,
but we need to have a user visible changes.

Which is more preferable design?

Thanks,
-- 
OSS Platform Development Division, NEC
KaiGai Kohei <kaigai@ak.jp.nec.com>

--------------070900010300020607070305
Content-Type: text/x-patch;
 name="pgsql-v8.5-select_for_update.patch"
Content-Transfer-Encoding: 7bit
Content-Disposition: inline;
 filename="pgsql-v8.5-select_for_update.patch"

*** base/src/include/nodes/parsenodes.h	2009-04-09 00:13:21.000000000 +0900
--- base_ex/src/include/nodes/parsenodes.h	2009-04-09 00:41:07.000000000 +0900
*************** typedef uint32 AclMode;			/* a bitmask o
*** 71,80 ****
  #define ACL_CREATE		(1<<9)	/* for namespaces and databases */
  #define ACL_CREATE_TEMP (1<<10) /* for databases */
  #define ACL_CONNECT		(1<<11) /* for databases */
- #define N_ACL_RIGHTS	12		/* 1 plus the last 1<<x */
- #define ACL_NO_RIGHTS	0
  /* Currently, SELECT ... FOR UPDATE/FOR SHARE requires UPDATE privileges */
! #define ACL_SELECT_FOR_UPDATE	ACL_UPDATE
  
  
  /*****************************************************************************
--- 71,80 ----
  #define ACL_CREATE		(1<<9)	/* for namespaces and databases */
  #define ACL_CREATE_TEMP (1<<10) /* for databases */
  #define ACL_CONNECT		(1<<11) /* for databases */
  /* Currently, SELECT ... FOR UPDATE/FOR SHARE requires UPDATE privileges */
! #define ACL_SELECT_FOR_UPDATE	(1<<12)
! #define N_ACL_RIGHTS	13		/* 1 plus the last 1<<x */
! #define ACL_NO_RIGHTS	0
  
  
  /*****************************************************************************
*** base/src/backend/utils/adt/acl.c	2009-02-09 13:08:34.000000000 +0900
--- base_ex/src/backend/utils/adt/acl.c	2009-04-06 11:49:32.000000000 +0900
*************** aclmask(const Acl *acl, Oid roleid, Oid 
*** 1141,1151 ****
  	for (i = 0; i < num; i++)
  	{
  		AclItem    *aidata = &aidat[i];
  
  		if (aidata->ai_grantee == ACL_ID_PUBLIC ||
  			aidata->ai_grantee == roleid)
  		{
! 			result |= aidata->ai_privs & mask;
  			if ((how == ACLMASK_ALL) ? (result == mask) : (result != 0))
  				return result;
  		}
--- 1141,1156 ----
  	for (i = 0; i < num; i++)
  	{
  		AclItem    *aidata = &aidat[i];
+ 		AclMode		aiprivs = aidata->ai_privs;
+ 
+ 		/* fixup ACL_SELECT_FOR_UPDATE */
+ 		if (aiprivs & ACL_UPDATE)
+ 			aiprivs |= ACL_SELECT_FOR_UPDATE;
  
  		if (aidata->ai_grantee == ACL_ID_PUBLIC ||
  			aidata->ai_grantee == roleid)
  		{
! 			result |= aiprivs & mask;
  			if ((how == ACLMASK_ALL) ? (result == mask) : (result != 0))
  				return result;
  		}
*************** aclmask(const Acl *acl, Oid roleid, Oid 
*** 1162,1167 ****
--- 1167,1177 ----
  	for (i = 0; i < num; i++)
  	{
  		AclItem    *aidata = &aidat[i];
+ 		AclMode		aiprivs = aidata->ai_privs;
+ 
+ 		/* fixup ACL_SELECT_FOR_UPDATE */
+ 		if (aiprivs & ACL_UPDATE)
+ 			aiprivs |= ACL_SELECT_FOR_UPDATE;
  
  		if (aidata->ai_grantee == ACL_ID_PUBLIC ||
  			aidata->ai_grantee == roleid)
*************** aclmask(const Acl *acl, Oid roleid, Oid 
*** 1170,1176 ****
  		if ((aidata->ai_privs & remaining) &&
  			has_privs_of_role(roleid, aidata->ai_grantee))
  		{
! 			result |= aidata->ai_privs & mask;
  			if ((how == ACLMASK_ALL) ? (result == mask) : (result != 0))
  				return result;
  			remaining = mask & ~result;
--- 1180,1186 ----
  		if ((aidata->ai_privs & remaining) &&
  			has_privs_of_role(roleid, aidata->ai_grantee))
  		{
! 			result |= aiprivs & mask;
  			if ((how == ACLMASK_ALL) ? (result == mask) : (result != 0))
  				return result;
  			remaining = mask & ~result;

--------------070900010300020607070305--