proposal: regrole type?

Started by Pavel Stehuleabout 13 years ago9 messages
#1Pavel Stehule
pavel.stehule@gmail.com

Hello

Can we implement REGROLE type, that simplify role name <-> oid transformations?

Regards

Pavel

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

#2Pavel Golub
pavel@microolap.com
In reply to: Pavel Stehule (#1)
Re: proposal: regrole type?

Hello, Pavel.

You wrote:

PS> Hello

PS> Can we implement REGROLE type, that simplify role name <-> oid transformations?

+1 from me. My old wish.

PS> Regards

PS> Pavel

--
With best wishes,
Pavel mailto:pavel@gf.microolap.com

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

#3Tom Lane
tgl@sss.pgh.pa.us
In reply to: Pavel Stehule (#1)
Re: proposal: regrole type?

Pavel Stehule <pavel.stehule@gmail.com> writes:

Can we implement REGROLE type, that simplify role name <-> oid transformations?

Why? It's not any more complicated than it is for the other object
types that lack REGxxx pseudotypes. Generally speaking, we've only
bothered with pseudotypes for the cases where lookup is not trivial,
eg because there are search path considerations.

regards, tom lane

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

#4Pavel Stehule
pavel.stehule@gmail.com
In reply to: Pavel Golub (#2)
Re: proposal: regrole type?

Hello

2012/12/25 Pavel Golub <pavel@microolap.com>:

Hello, Pavel.

You wrote:

PS> Hello

PS> Can we implement REGROLE type, that simplify role name <-> oid transformations?

+1 from me. My old wish.

I started implementation. I found a two points, that should be solved before.

we operate over roles with (without) fictive role "public". So we need
two datatypes :( I have no idea about second name :( - there should be
difference if type enables or disallow "public".

second issue is value of ACL_ID_PUBLIC, that is zero - and there is
not difference from InvalidOid - what should be acceptable via "-"
symbol.

Any ideas?

Regards

Pavel

PS> Regards

PS> Pavel

--
With best wishes,
Pavel mailto:pavel@gf.microolap.com

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

#5Pavel Stehule
pavel.stehule@gmail.com
In reply to: Tom Lane (#3)
Re: proposal: regrole type?

2012/12/25 Tom Lane <tgl@sss.pgh.pa.us>:

Pavel Stehule <pavel.stehule@gmail.com> writes:

Can we implement REGROLE type, that simplify role name <-> oid transformations?

Why? It's not any more complicated than it is for the other object
types that lack REGxxx pseudotypes. Generally speaking, we've only
bothered with pseudotypes for the cases where lookup is not trivial,
eg because there are search path considerations.

my first motivation was a cosiness, but a second view shows so this
type(s) can be useful:

* It is relative natural - and can simplify and speed up some kind of
queries, because it directly access to cache.

* We can reduce to half lot of functions \df has_* (84 functions)

pg_catalog | pg_has_role | boolean | name, name, text | normal
pg_catalog | pg_has_role | boolean | name, oid, text | normal
pg_catalog | pg_has_role | boolean | name, text | normal
pg_catalog | pg_has_role | boolean | oid, name, text | normal
pg_catalog | pg_has_role | boolean | oid, oid, text | normal
pg_catalog | pg_has_role | boolean | oid, text | normal

these function should be replaced with more semantics descriptive headers

pg_has_role(regrole, regrole, text)
pg_has_role(regrole, text)
pg_has_role(text)

so we can drop (42 functions from catalog)

* this new datatype can be used in custom functions with implicit
validity checking - and if I can sort a importance of some internal
objects - then the roles are relative on high position.

Best regards

Pavel

p.s. A implementation should be little bit harder than I expected due
specific role "public", but I am thinking so it can has a some value.

regards, tom lane

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

#6Tom Lane
tgl@sss.pgh.pa.us
In reply to: Pavel Stehule (#5)
Re: proposal: regrole type?

Pavel Stehule <pavel.stehule@gmail.com> writes:

* We can reduce to half lot of functions \df has_* (84 functions)

Not without breaking existing queries. A function taking regrole might
look like it substitutes for one taking a text-string user name as long
as you only pass literal constants to it, but as soon as you pass
non-constants you'll find out different. (Unless your plan is to also
create an implicit cast from text to regrole, which strikes me as a
seriously bad idea.)

The reason we've not been more aggressive about using the OID-alias
pseudotypes is exactly that they're not a cure-all. Otherwise we would
already have about a dozen more of them. I don't think it's really
worth it: the notational savings is pretty marginal and the impact on
application namespace should not be ignored. (Keep in mind that any new
system type causes problems for similarly-named user tables.)

regards, tom lane

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

#7Pavel Stehule
pavel.stehule@gmail.com
In reply to: Tom Lane (#6)
Re: proposal: regrole type?

2012/12/25 Tom Lane <tgl@sss.pgh.pa.us>:

Pavel Stehule <pavel.stehule@gmail.com> writes:

* We can reduce to half lot of functions \df has_* (84 functions)

Not without breaking existing queries. A function taking regrole might
look like it substitutes for one taking a text-string user name as long
as you only pass literal constants to it, but as soon as you pass
non-constants you'll find out different. (Unless your plan is to also
create an implicit cast from text to regrole, which strikes me as a
seriously bad idea.)

understand

The reason we've not been more aggressive about using the OID-alias
pseudotypes is exactly that they're not a cure-all.

yes, I agree. But this type can has sense without propagation to
current functionality - and current functions can be marked as
obsolete in future. I believe so it can clean little bit this are -
mainly can solve task, where can be used pseudo role "public" and it
is more accurate and semantic design and can be used in new functions
and system views.

Otherwise we would

already have about a dozen more of them. I don't think it's really
worth it: the notational savings is pretty marginal and the impact on
application namespace should not be ignored. (Keep in mind that any new
system type causes problems for similarly-named user tables.)

9.3 has no problem

postgres=# create table regtype(a int);
CREATE TABLE

postgres=# create table regclass(a int);
CREATE TABLE

Regards

Pavel

regards, tom lane

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

#8Pavel Stehule
pavel.stehule@gmail.com
In reply to: Pavel Stehule (#4)
Re: proposal: regrole type?

2012/12/25 Pavel Stehule <pavel.stehule@gmail.com>:

Hello

2012/12/25 Pavel Golub <pavel@microolap.com>:

Hello, Pavel.

You wrote:

PS> Hello

PS> Can we implement REGROLE type, that simplify role name <-> oid transformations?

+1 from me. My old wish.

I started implementation. I found a two points, that should be solved before.

we operate over roles with (without) fictive role "public". So we need
two datatypes :( I have no idea about second name :( - there should be
difference if type enables or disallow "public".

second issue is value of ACL_ID_PUBLIC, that is zero - and there is
not difference from InvalidOid - what should be acceptable via "-"
symbol.

Any ideas?

one idea

regrole - defined only for "real" roles - support InvalidOid - doesn't
support "public"

regaclrole - defined for any roles, that can be used for ACL (with
"public"), doesn't support InvalidOid

Regards

Pavel

Regards

Pavel

PS> Regards

PS> Pavel

--
With best wishes,
Pavel mailto:pavel@gf.microolap.com

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

#9Pavel Stehule
pavel.stehule@gmail.com
In reply to: Pavel Stehule (#7)
1 attachment(s)
Re: proposal: regrole type?

2012/12/26 Pavel Stehule <pavel.stehule@gmail.com>:

2012/12/25 Tom Lane <tgl@sss.pgh.pa.us>:

Pavel Stehule <pavel.stehule@gmail.com> writes:

* We can reduce to half lot of functions \df has_* (84 functions)

Not without breaking existing queries. A function taking regrole might
look like it substitutes for one taking a text-string user name as long
as you only pass literal constants to it, but as soon as you pass
non-constants you'll find out different. (Unless your plan is to also
create an implicit cast from text to regrole, which strikes me as a
seriously bad idea.)

I was little bit surprised so regproc, regprocedure is not used on
SQL level in our builtin functions - and I use both types often in our
custom queries.

So it can be similar with regrole and regaclrole - it can be addressed
for more orthogonal work with roles

I am sending patch, but I will not assign to commitfest now.

Regards

Pavel

Attachments:

regrole.patchapplication/octet-stream; name=regrole.patchDownload
*** a/src/backend/utils/adt/regproc.c
--- b/src/backend/utils/adt/regproc.c
***************
*** 26,31 ****
--- 26,32 ----
  #include "access/htup_details.h"
  #include "catalog/indexing.h"
  #include "catalog/namespace.h"
+ #include "catalog/pg_authid.h"
  #include "catalog/pg_class.h"
  #include "catalog/pg_operator.h"
  #include "catalog/pg_proc.h"
***************
*** 35,40 ****
--- 36,42 ----
  #include "lib/stringinfo.h"
  #include "miscadmin.h"
  #include "parser/parse_type.h"
+ #include "utils/acl.h"
  #include "utils/builtins.h"
  #include "utils/fmgroids.h"
  #include "utils/lsyscache.h"
***************
*** 1495,1497 **** parseNameAndArgTypes(const char *string, bool allowNone, List **names,
--- 1497,1674 ----
  
  	pfree(rawname);
  }
+ 
+ /*
+  * returns true, when string is numeric
+  */
+ static bool
+ is_numeric_oid(char *str_oid, Oid *oid)
+ {
+ 	if (str_oid[0] >= '0' &&
+ 		str_oid[0] <= '9' &&
+ 		strspn(str_oid, "0123456789") == strlen(str_oid))
+ 	{
+ 		*oid = DatumGetObjectId(DirectFunctionCall1(oidin,
+ 									  CStringGetDatum(str_oid)));
+ 		return true;
+ 	}
+ 	else
+ 		return false;
+ }
+ 
+ /*
+  * returns printed role name or oid, when role doesn't exists
+  */
+ static char *
+ format_roleid(Oid roleid)
+ {
+ 	HeapTuple	tuple;
+ 	char		*result;
+ 
+ 	tuple = SearchSysCache1(AUTHOID, ObjectIdGetDatum(roleid));
+ 	if (HeapTupleIsValid(tuple))
+ 	{
+ 		result = pstrdup(NameStr(((Form_pg_authid) GETSTRUCT(tuple))->rolname));
+ 		ReleaseSysCache(tuple);
+ 	}
+ 	else
+ 	{
+ 		/* If OID doesn't match any pg_ts_dict row, return it numerically */
+ 		result = (char *) palloc(NAMEDATALEN);
+ 		snprintf(result, NAMEDATALEN, "%u", roleid);
+ 	}
+ 
+ 	return result;
+ }
+ 
+ /*
+  * regprolein		- converts "rolename" to role OID
+  *
+  * We also accept a numeric OID, for symmetry with the output routine.
+  *
+  * '-' signifies unknown (OID 0).  In all other cases, the input must
+  * match an existing pg_role entry.
+  */
+ Datum
+ regrolein(PG_FUNCTION_ARGS)
+ {
+ 	char	   *rolname_or_oid = PG_GETARG_CSTRING(0);
+ 	Oid result = InvalidOid;
+ 
+ 	/* '-' ? */
+ 	if (strcmp(rolname_or_oid, "-") == 0)
+ 		PG_RETURN_OID(InvalidOid);
+ 
+ 	/* Numeric OID? */
+ 	if (is_numeric_oid(rolname_or_oid, &result))
+ 		PG_RETURN_OID(result);
+ 
+ 	/* Now we know so input string should be relname */
+ 	result = get_role_oid(rolname_or_oid, false);
+ 
+ 	PG_RETURN_OID(result);
+ }
+ 
+ /*
+  * regroleout		- converts role OID to "rolname"
+  */
+ Datum
+ regroleout(PG_FUNCTION_ARGS)
+ {
+ 	RegProcedure roleid = PG_GETARG_OID(0);
+ 	char	   *result;
+ 
+ 	if (roleid == InvalidOid)
+ 		result = pstrdup("-");
+ 	else
+ 		result = format_roleid(roleid);
+ 
+ 	PG_RETURN_CSTRING(result);
+ }
+ 
+ /*
+  *		regprolerecv			- converts external binary format to regrole
+  */
+ Datum
+ regrolerecv(PG_FUNCTION_ARGS)
+ {
+ 	/* Exactly the same as oidrecv, so share code */
+ 	return oidrecv(fcinfo);
+ }
+ 
+ /*
+  *		regprolesend			- converts regprole to binary format
+  */
+ Datum
+ regrolesend(PG_FUNCTION_ARGS)
+ {
+ 	/* Exactly the same as oidsend, so share code */
+ 	return oidsend(fcinfo);
+ }
+ 
+ /*
+  * regaclrolein		- converts "rolename" to role OID
+  *
+  * We also accept a numeric OID, for symmetry with the output routine.
+  *
+  * It doesn't support '-' as InvalidOid, because "public" role has zero oid.
+  */
+ Datum
+ regaclrolein(PG_FUNCTION_ARGS)
+ {
+ 	char	   *rolname_or_oid = PG_GETARG_CSTRING(0);
+ 	Oid result;
+ 
+ 	/* Numeric OID? */
+ 	if (is_numeric_oid(rolname_or_oid, &result))
+ 		PG_RETURN_OID(result);
+ 
+ 	if (strcmp(rolname_or_oid, "public") == 0)
+ 		/*
+ 		 * Note: there is a issue because ACL_ID_PUBLIC is 0,
+ 		 * and there is not difference between InvalidOid. Can we join
+ 		 * "public" role with InvalidOid role id, resp symbol "-"?
+ 		 */
+ 		result = ACL_ID_PUBLIC;
+ 	else
+ 		result = get_role_oid(rolname_or_oid, false);
+ 
+ 	PG_RETURN_OID(result);
+ }
+ 
+ /*
+  * regaclroleout		- converts role OID to "rolname"
+  */
+ Datum
+ regaclroleout(PG_FUNCTION_ARGS)
+ {
+ 	RegProcedure roleid = PG_GETARG_OID(0);
+ 	char	   *result;
+ 
+ 	if (roleid == ACL_ID_PUBLIC)
+ 		result = pstrdup("public");
+ 	else
+ 		result = format_roleid(roleid);
+ 
+ 	PG_RETURN_CSTRING(result);
+ }
+ 
+ /*
+  *		regprolerecv			- converts external binary format to regrole
+  */
+ Datum
+ regaclrolerecv(PG_FUNCTION_ARGS)
+ {
+ 	/* Exactly the same as oidrecv, so share code */
+ 	return oidrecv(fcinfo);
+ }
+ 
+ /*
+  *		regprolesend			- converts regprole to binary format
+  */
+ Datum
+ regaclrolesend(PG_FUNCTION_ARGS)
+ {
+ 	/* Exactly the same as oidsend, so share code */
+ 	return oidsend(fcinfo);
+ }
*** a/src/include/catalog/pg_cast.h
--- b/src/include/catalog/pg_cast.h
***************
*** 187,192 **** DATA(insert (	21 2205  313 i f ));
--- 187,193 ----
  DATA(insert (	23 2205    0 i b ));
  DATA(insert ( 2205	 20 1288 a f ));
  DATA(insert ( 2205	 23    0 a b ));
+ 
  DATA(insert (	26 2206    0 i b ));
  DATA(insert ( 2206	 26    0 i b ));
  DATA(insert (	20 2206 1287 i f ));
***************
*** 194,199 **** DATA(insert (	21 2206  313 i f ));
--- 195,214 ----
  DATA(insert (	23 2206    0 i b ));
  DATA(insert ( 2206	 20 1288 a f ));
  DATA(insert ( 2206	 23    0 a b ));
+ DATA(insert (	26 3475    0 i b ));
+ DATA(insert ( 3475	 26    0 i b ));
+ DATA(insert (	20 3475 1287 i f ));
+ DATA(insert (	21 3475  313 i f ));
+ DATA(insert (	23 3475    0 i b ));
+ DATA(insert ( 3475	 20 1288 a f ));
+ DATA(insert ( 3475	 23    0 a b ));
+ DATA(insert (	26 3477    0 i b ));
+ DATA(insert ( 3477	 26    0 i b ));
+ DATA(insert (	20 3477 1287 i f ));
+ DATA(insert (	21 3477  313 i f ));
+ DATA(insert (	23 3477    0 i b ));
+ DATA(insert ( 3477	 20 1288 a f ));
+ DATA(insert ( 3477	 23    0 a b ));
  DATA(insert (	26 3734    0 i b ));
  DATA(insert ( 3734	 26    0 i b ));
  DATA(insert (	20 3734 1287 i f ));
*** a/src/include/catalog/pg_proc.h
--- b/src/include/catalog/pg_proc.h
***************
*** 4123,4128 **** DESCR("I/O");
--- 4123,4146 ----
  DATA(insert OID = 2963 (  uuid_hash		   PGNSP PGUID 12 1 0 0 0 f f f f t f i 1 0 23 "2950" _null_ _null_ _null_ _null_ uuid_hash _null_ _null_ _null_ ));
  DESCR("hash");
  
+ /* regrole related function */
+ DATA(insert OID = 3479 (  regrolein		PGNSP PGUID 12 1 0 0 0 f f f f t f s 1 0 3475 "2275" _null_ _null_ _null_ _null_ regrolein _null_ _null_ _null_ ));
+ DESCR("I/O");
+ DATA(insert OID = 3480 (  regroleout		PGNSP PGUID 12 1 0 0 0 f f f f t f s 1 0 2275 "3475" _null_ _null_ _null_ _null_ regroleout _null_ _null_ _null_ ));
+ DESCR("I/O");
+ DATA(insert OID = 3481 (  regrolerecv		PGNSP PGUID 12 1 0 0 0 f f f f t f i 1 0 3475 "2281" _null_ _null_ _null_ _null_ regrolerecv _null_ _null_ _null_ ));
+ DESCR("I/O");
+ DATA(insert OID = 3482 (  regrolesend		PGNSP PGUID 12 1 0 0 0 f f f f t f i 1 0 17 "3475" _null_ _null_ _null_ _null_ regrolesend _null_ _null_ _null_ ));
+ DESCR("I/O");
+ DATA(insert OID = 3483 (  regaclrolein		PGNSP PGUID 12 1 0 0 0 f f f f t f s 1 0 3477 "2275" _null_ _null_ _null_ _null_ regaclrolein _null_ _null_ _null_ ));
+ DESCR("I/O");
+ DATA(insert OID = 3484 (  regaclroleout		PGNSP PGUID 12 1 0 0 0 f f f f t f s 1 0 2275 "3477" _null_ _null_ _null_ _null_ regaclroleout _null_ _null_ _null_ ));
+ DESCR("I/O");
+ DATA(insert OID = 3485 (  regaclrolerecv		PGNSP PGUID 12 1 0 0 0 f f f f t f i 1 0 3477 "2281" _null_ _null_ _null_ _null_ regaclrolerecv _null_ _null_ _null_ ));
+ DESCR("I/O");
+ DATA(insert OID = 3486 (  regaclrolesend		PGNSP PGUID 12 1 0 0 0 f f f f t f i 1 0 17 "3477" _null_ _null_ _null_ _null_ regaclrolesend _null_ _null_ _null_ ));
+ DESCR("I/O");
+ 
  /* enum related procs */
  DATA(insert OID = 3504 (  anyenum_in	PGNSP PGUID 12 1 0 0 0 f f f f t f i 1 0 3500 "2275" _null_ _null_ _null_ _null_ anyenum_in _null_ _null_ _null_ ));
  DESCR("I/O");
*** a/src/include/catalog/pg_type.h
--- b/src/include/catalog/pg_type.h
***************
*** 576,581 **** DESCR("UUID datatype");
--- 576,593 ----
  #define UUIDOID 2950
  DATA(insert OID = 2951 ( _uuid			PGNSP PGUID -1 f b A f t \054 0 2950 0 array_in array_out array_recv array_send - - array_typanalyze i x f 0 -1 0 0 _null_ _null_ _null_ ));
  
+ /* regrole */
+ DATA(insert OID = 3475 ( regrole	   PGNSP PGUID	4 t b N f t \054 0	 0 3476 regrolein regroleout regrolerecv regrolesend - - - i p f 0 -1 0 0 _null_ _null_ _null_ ));
+ DESCR("registered role");
+ #define REGROLEOID		3475
+ DATA(insert OID = 3476 ( _regrole PGNSP PGUID -1 f b A f t \054 0 3475 0 array_in array_out array_recv array_send - - array_typanalyze i x f 0 -1 0 0 _null_ _null_ _null_ ));
+ #define REGROLEARRAYOID 3476
+ DATA(insert OID = 3477 ( regaclrole	   PGNSP PGUID	4 t b N f t \054 0	 0 3478 regaclrolein regaclroleout regaclrolerecv regaclrolesend - - - i p f 0 -1 0 0 _null_ _null_ _null_ ));
+ DESCR("registered role");
+ #define REGACLROLEOID		3477
+ DATA(insert OID = 3478 ( _regaclrole PGNSP PGUID -1 f b A f t \054 0 3477 0 array_in array_out array_recv array_send - - array_typanalyze i x f 0 -1 0 0 _null_ _null_ _null_ ));
+ #define REGACLROLEARRAYOID 3478
+ 
  /* text search */
  DATA(insert OID = 3614 ( tsvector		PGNSP PGUID -1 f b U f t \054 0 0 3643 tsvectorin tsvectorout tsvectorrecv tsvectorsend - - ts_typanalyze i x f 0 -1 0 0 _null_ _null_ _null_ ));
  DESCR("text representation for text search");
***************
*** 624,629 **** DATA(insert OID = 3926 ( int8range		PGNSP PGUID  -1 f r R f t \054 0 0 3927 rang
--- 636,643 ----
  DESCR("range of bigints");
  DATA(insert OID = 3927 ( _int8range		PGNSP PGUID  -1 f b A f t \054 0 3926 0 array_in array_out array_recv array_send - - array_typanalyze d x f 0 -1 0 0 _null_ _null_ _null_ ));
  
+ 
+ 
  /*
   * pseudo-types
   *
*** a/src/include/utils/builtins.h
--- b/src/include/utils/builtins.h
***************
*** 608,613 **** extern Datum regdictionaryin(PG_FUNCTION_ARGS);
--- 608,621 ----
  extern Datum regdictionaryout(PG_FUNCTION_ARGS);
  extern Datum regdictionaryrecv(PG_FUNCTION_ARGS);
  extern Datum regdictionarysend(PG_FUNCTION_ARGS);
+ extern Datum regrolein(PG_FUNCTION_ARGS);
+ extern Datum regroleout(PG_FUNCTION_ARGS);
+ extern Datum regrolerecv(PG_FUNCTION_ARGS);
+ extern Datum regrolesend(PG_FUNCTION_ARGS);
+ extern Datum regaclrolein(PG_FUNCTION_ARGS);
+ extern Datum regaclroleout(PG_FUNCTION_ARGS);
+ extern Datum regaclrolerecv(PG_FUNCTION_ARGS);
+ extern Datum regaclrolesend(PG_FUNCTION_ARGS);
  extern Datum text_regclass(PG_FUNCTION_ARGS);
  extern List *stringToQualifiedNameList(const char *string);
  extern char *format_procedure(Oid procedure_oid);