Re: [HACKERS] Binary in/out for aclitem

2011-02-24 Thread Merlin Moncure
On Wed, Feb 23, 2011 at 3:30 PM, Tom Lane t...@sss.pgh.pa.us wrote:
 =?utf-8?q?Rados=C5=82aw_Smogura?= rsmog...@softperience.eu writes:
 Here is extended version, has version field (N_ACL_RIGHTS*2) and reserved
 mask, as well definition is more general then def of PGSQL. In any way it
 require that rights mades bit array.

 You're going in quite the wrong direction here.  The consensus as I
 understood it was that we should just use the text representation in
 binary mode too, rather than inventing a separate representation that's
 going to put a whole new set of constraints on what can happen to the
 internal representation.  The proposal you have here has no redeeming
 social value whatever, because nobody cares about the I/O efficiency
 for aclitem (and even if anyone did, you've made no case that this would
 actually be more efficient to use on the client side).

+1 on this.  binary wire format is a win generally when one of the two
properties is true:

1) the receiving application is putting it into a binary structure
that is similar to what the backend sends, and conversion is
non-trivial (timestamps, geo types, etc)
2) text format needs lots of escaping (bytea, arrays etc)

Let's take the numeric type for example...if we were debating the
binary wire format for that type, I would be arguing for the backend
to send a string for the binary wire format unless someone could
present a solid case that the postgres format dropped right into a
popular numeric library in C, etc (AFAIK, it doesn't).  Almost
everyone that gets a numeric will directly translate it to a string or
a hardware binary representation which the backend can't send.

Even if you could make the case for aclitem on performance grounds,
you still have to get past tom's objection (which I agree with) that
the performance benefit outweighs having to deal with making and
(especially) maintaining the binary wire format.  It should be
becoming obvious to everyone the binary formats are becoming
increasingly popular, and sooner or later backwards compatibility
issues and other unresolved issues pertaining to them have to be dealt
with.  Point being, let's not make that more difficult than it has to
be.

merlin

-- 
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] Binary in/out for aclitem

2011-02-24 Thread rsmogura

On Thu, 24 Feb 2011 08:38:35 -0600, Merlin Moncure wrote:

On Wed, Feb 23, 2011 at 3:30 PM, Tom Lane t...@sss.pgh.pa.us wrote:

=?utf-8?q?Rados=C5=82aw_Smogura?= rsmog...@softperience.eu writes:
Here is extended version, has version field (N_ACL_RIGHTS*2) and 
reserved
mask, as well definition is more general then def of PGSQL. In any 
way it

require that rights mades bit array.


You're going in quite the wrong direction here.  The consensus as I
understood it was that we should just use the text representation in
binary mode too, rather than inventing a separate representation 
that's
going to put a whole new set of constraints on what can happen to 
the
internal representation.  The proposal you have here has no 
redeeming

social value whatever, because nobody cares about the I/O efficiency
for aclitem (and even if anyone did, you've made no case that this 
would

actually be more efficient to use on the client side).


+1 on this.  binary wire format is a win generally when one of the 
two

properties is true:

1) the receiving application is putting it into a binary structure
that is similar to what the backend sends, and conversion is
non-trivial (timestamps, geo types, etc)
2) text format needs lots of escaping (bytea, arrays etc)



Let's take the numeric type for example...if we were debating the
binary wire format for that type, I would be arguing for the backend
to send a string for the binary wire format unless someone could
present a solid case that the postgres format dropped right into a
popular numeric library in C, etc (AFAIK, it doesn't).  Almost
everyone that gets a numeric will directly translate it to a string 
or

a hardware binary representation which the backend can't send.

Even if you could make the case for aclitem on performance grounds,
you still have to get past tom's objection (which I agree with) that
the performance benefit outweighs having to deal with making and
(especially) maintaining the binary wire format.  It should be
becoming obvious to everyone the binary formats are becoming
increasingly popular, and sooner or later backwards compatibility
issues and other unresolved issues pertaining to them have to be 
dealt

with.  Point being, let's not make that more difficult than it has to
be.

merlin


Thanks, but actually I didn't realized final direction, pass to text 
or create something really extensive, I didn't treat aclitem IO as 
live or dead case, just all. I always treat performance really serious, 
but I'm not psychopathic to check aclitem IO!!!


Btw, In my opinion binary format will be popular not for speed, but for 
that it is internal strict, and pass in many situations more useful 
informations (e.g. types for structs, arrays), it is just easier to 
maintain on driver side. But it is still unpopular maybe due to missing 
methods :), and few others.


Regards,
Radek


--
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] Binary in/out for aclitem

2011-02-24 Thread Radosław Smogura
Tom Lane t...@sss.pgh.pa.us Wednesday 23 February 2011 22:30:04
 =?utf-8?q?Rados=C5=82aw_Smogura?= rsmog...@softperience.eu writes:
  Here is extended version, has version field (N_ACL_RIGHTS*2) and reserved
  mask, as well definition is more general then def of PGSQL. In any way it
  require that rights mades bit array.
 
 You're going in quite the wrong direction here.  The consensus as I
 understood it was that we should just use the text representation in
 binary mode too, rather than inventing a separate representation that's
 going to put a whole new set of constraints on what can happen to the
 internal representation.  The proposal you have here has no redeeming
 social value whatever, because nobody cares about the I/O efficiency
 for aclitem (and even if anyone did, you've made no case that this would
 actually be more efficient to use on the client side).
 
   regards, tom lane

Look at it. Pass call to in/out.

Regards,
Radek
diff --git a/.gitignore b/.gitignore
index 1be11e8..0d594f9 100644
--- a/.gitignore
+++ b/.gitignore
@@ -17,3 +17,5 @@ objfiles.txt
 /GNUmakefile
 /config.log
 /config.status
+/nbproject/private/
+/nbproject
diff --git a/src/backend/utils/adt/acl.c b/src/backend/utils/adt/acl.c
index 691ba3b..fa151cd 100644
--- a/src/backend/utils/adt/acl.c
+++ b/src/backend/utils/adt/acl.c
@@ -33,6 +33,7 @@
 #include utils/lsyscache.h
 #include utils/memutils.h
 #include utils/syscache.h
+#include libpq/pqformat.h
 
 
 typedef struct
@@ -77,7 +78,8 @@ static const char *getid(const char *s, char *n);
 static void putid(char *p, const char *s);
 static Acl *allocacl(int n);
 static void check_acl(const Acl *acl);
-static const char *aclparse(const char *s, AclItem *aip);
+static const char *aclparse(const char *s, AclItem *aip, bool binary);
+static Datum aclitem_common_in_recv(const char* s, bool binary);
 static bool aclitem_match(const AclItem *a1, const AclItem *a2);
 static int	aclitemComparator(const void *arg1, const void *arg2);
 static void check_circularity(const Acl *old_acl, const AclItem *mod_aip,
@@ -222,6 +224,8 @@ putid(char *p, const char *s)
  *
  *		This routine is called by the parser as well as aclitemin(), hence
  *		the added generality.
+ * 
+ * @param binary if we parse for binary mode or text mode
  *
  * RETURNS:
  *		the string position in 's' immediately following the ACL
@@ -230,7 +234,7 @@ putid(char *p, const char *s)
  *		  UID/GID, id type identifier and mode type values.
  */
 static const char *
-aclparse(const char *s, AclItem *aip)
+aclparse(const char *s, AclItem *aip, bool binary)
 {
 	AclMode		privs,
 goption,
@@ -249,20 +253,20 @@ aclparse(const char *s, AclItem *aip)
 		/* we just read a keyword, not a name */
 		if (strcmp(name, group) != 0  strcmp(name, user) != 0)
 			ereport(ERROR,
-	(errcode(ERRCODE_INVALID_TEXT_REPRESENTATION),
+	(errcode((binary ? ERRCODE_INVALID_BINARY_REPRESENTATION : ERRCODE_INVALID_TEXT_REPRESENTATION)),
 	 errmsg(unrecognized key word: \%s\, name),
 	 errhint(ACL key word must be \group\ or \user\.)));
 		s = getid(s, name);		/* move s to the name beyond the keyword */
 		if (name[0] == '\0')
 			ereport(ERROR,
-	(errcode(ERRCODE_INVALID_TEXT_REPRESENTATION),
+	(errcode((binary ? ERRCODE_INVALID_BINARY_REPRESENTATION : ERRCODE_INVALID_TEXT_REPRESENTATION)),
 	 errmsg(missing name),
 	 errhint(A name must follow the \group\ or \user\ key word.)));
 	}
 
 	if (*s != '=')
 		ereport(ERROR,
-(errcode(ERRCODE_INVALID_TEXT_REPRESENTATION),
+(errcode((binary ? ERRCODE_INVALID_BINARY_REPRESENTATION : ERRCODE_INVALID_TEXT_REPRESENTATION)),
  errmsg(missing \=\ sign)));
 
 	privs = goption = ACL_NO_RIGHTS;
@@ -315,7 +319,7 @@ aclparse(const char *s, AclItem *aip)
 break;
 			default:
 ereport(ERROR,
-		(errcode(ERRCODE_INVALID_TEXT_REPRESENTATION),
+		(errcode((binary ? ERRCODE_INVALID_BINARY_REPRESENTATION : ERRCODE_INVALID_TEXT_REPRESENTATION)),
 	  errmsg(invalid mode character: must be one of \%s\,
 			 ACL_ALL_RIGHTS_STR)));
 		}
@@ -337,7 +341,7 @@ aclparse(const char *s, AclItem *aip)
 		s = getid(s + 1, name2);
 		if (name2[0] == '\0')
 			ereport(ERROR,
-	(errcode(ERRCODE_INVALID_TEXT_REPRESENTATION),
+	(errcode((binary ? ERRCODE_INVALID_BINARY_REPRESENTATION : ERRCODE_INVALID_TEXT_REPRESENTATION)),
 	 errmsg(a name must follow the \/\ sign)));
 		aip-ai_grantor = get_role_oid(name2, false);
 	}
@@ -548,6 +552,22 @@ check_acl(const Acl *acl)
  errmsg(ACL arrays must not contain null values)));
 }
 
+static
+Datum aclitem_common_in_recv(const char* s, bool binary)
+{
+AclItem*aip;
+
+aip = (AclItem *) palloc(sizeof(AclItem));
+s = aclparse(s, aip, binary);
+while (isspace((unsigned char) *s))
+++s;
+if (*s)
+ereport(ERROR,
+(errcode((binary ? ERRCODE_INVALID_BINARY_REPRESENTATION : ERRCODE_INVALID_TEXT_REPRESENTATION)),
+   

Re: [HACKERS] Binary in/out for aclitem

2011-02-23 Thread rsmogura

On Tue, 22 Feb 2011 20:20:39 -0500, Tom Lane wrote:

Robert Haas robertmh...@gmail.com writes:

On Tue, Feb 22, 2011 at 5:24 PM, Tom Lane t...@sss.pgh.pa.us wrote:
It'd be more future-proof than this patch, but I'm still 
unconvinced

about the use-case.


Do we want to intentionally make binary format a second-class 
citizen?


Well, it's not exactly a first-class citizen; compare for instance 
the
amount of verbiage in the docs about text I/O formats versus the 
amount
about binary formats.  But my question isn't about that; it's about 
why

aclitem should be considered a first-class citizen.  It makes me
uncomfortable that client apps are looking at it at all, because any
that do are bound to get broken in the future, even assuming that 
they
get the right answers today.  I wonder how many such clients are up 
to
speed for per-column privileges and non-constant default privileges 
for

instance.  And sepgsql is going to cut them off at the knees.

regards, tom lane


Technically, at eye glance, I didn't seen in sepgsql modifications to 
acl.h. So, I think, aclitem will be unaffected. In any way sepgsql needs 
some way to present access rights to administrator it may use own model, 
or aclitem, too.


JDBC, and other applications may use aclitem to get just information 
about who has what access. I think psql does this in same manner as 
JDBC, by calling select from pg_class. But if user, through psql, JDBC 
or other driver. will invoke select * from pg_class it will fail with 
no binary output, because it is plain user query.


Currently proposed binary output has space for 4 more privs. Am I 
right?


One thing I realized, I do not pass flag if grant target is group or 
user.


Regards,
Radek


--
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] Binary in/out for aclitem

2011-02-23 Thread Tom Lane
rsmogura rsmog...@softperience.eu writes:
  On Tue, 22 Feb 2011 20:20:39 -0500, Tom Lane wrote:
 ...  But my question isn't about that; it's about 
 why aclitem should be considered a first-class citizen.  It makes me
 uncomfortable that client apps are looking at it at all, because any
 that do are bound to get broken in the future, even assuming that 
 they get the right answers today.  I wonder how many such clients are up 
 to speed for per-column privileges and non-constant default privileges 
 for instance.  And sepgsql is going to cut them off at the knees.

  Technically, at eye glance, I didn't seen in sepgsql modifications to 
  acl.h. So, I think, aclitem will be unaffected. In any way sepgsql needs 
  some way to present access rights to administrator it may use own model, 
  or aclitem, too.

You're missing the point, which is that the current internal
representation of aclitem could change drastically to support future
feature improvements in the area of privileges.  It has already changed
significantly in the past (we didn't use to have WITH GRANT OPTION).
If we had to add a field, for instance, a binary representation would
simply be broken, as clients would have difficulty telling how to
interpret it as soon as there was more than one possible format.
Text representations are typically a bit more extensible.

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


Re: [HACKERS] Binary in/out for aclitem

2011-02-23 Thread Radosław Smogura
Tom Lane t...@sss.pgh.pa.us Wednesday 23 February 2011 16:19:27
 rsmogura rsmog...@softperience.eu writes:
   On Tue, 22 Feb 2011 20:20:39 -0500, Tom Lane wrote:
  ...  But my question isn't about that; it's about
  why aclitem should be considered a first-class citizen.  It makes me
  uncomfortable that client apps are looking at it at all, because any
  that do are bound to get broken in the future, even assuming that
  they get the right answers today.  I wonder how many such clients are up
  to speed for per-column privileges and non-constant default privileges
  for instance.  And sepgsql is going to cut them off at the knees.
  
   Technically, at eye glance, I didn't seen in sepgsql modifications to
   acl.h. So, I think, aclitem will be unaffected. In any way sepgsql needs
   some way to present access rights to administrator it may use own model,
   or aclitem, too.
 
 You're missing the point, which is that the current internal
 representation of aclitem could change drastically to support future
 feature improvements in the area of privileges.  It has already changed
 significantly in the past (we didn't use to have WITH GRANT OPTION).
 If we had to add a field, for instance, a binary representation would
 simply be broken, as clients would have difficulty telling how to
 interpret it as soon as there was more than one possible format.
 Text representations are typically a bit more extensible.
 
   regards, tom lane
I removed from patch this (think like currently not needed, but it is enaught 
to put in doc)

Each privilige has idividual number P from 1 to n. and it is represented by 
setted P-th bit. First n-th bits (in network bit order) represents normal 
priv, next n-th bits represents grant option of privs. This chain is encoded 
as n*2 bit number rounded up to full 8 bits, with minimal length 32 bit.

I was thinking about adding number of all privs to each ACL item, removed as 
this could be deducted from PG version, where 1st 7-bit represents version, 
last 8-th bit will represent if grant part has been added.

---
In any way binary output should be available, if we have binary mode. I know 
that text is more extensible, we may in contrast to above packed version, 
describes acl privs as byte array elements from represented setted priv (same 
as text).

Fallback solution is to just recall aclin/aclout with StringInfo.

Regards,
Radek.

-- 
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] Binary in/out for aclitem

2011-02-23 Thread Radosław Smogura
Tom Lane t...@sss.pgh.pa.us Wednesday 23 February 2011 16:19:27
 rsmogura rsmog...@softperience.eu writes:
   On Tue, 22 Feb 2011 20:20:39 -0500, Tom Lane wrote:
  ...  But my question isn't about that; it's about
  why aclitem should be considered a first-class citizen.  It makes me
  uncomfortable that client apps are looking at it at all, because any
  that do are bound to get broken in the future, even assuming that
  they get the right answers today.  I wonder how many such clients are up
  to speed for per-column privileges and non-constant default privileges
  for instance.  And sepgsql is going to cut them off at the knees.
  
   Technically, at eye glance, I didn't seen in sepgsql modifications to
   acl.h. So, I think, aclitem will be unaffected. In any way sepgsql needs
   some way to present access rights to administrator it may use own model,
   or aclitem, too.
 
 You're missing the point, which is that the current internal
 representation of aclitem could change drastically to support future
 feature improvements in the area of privileges.  It has already changed
 significantly in the past (we didn't use to have WITH GRANT OPTION).
 If we had to add a field, for instance, a binary representation would
 simply be broken, as clients would have difficulty telling how to
 interpret it as soon as there was more than one possible format.
 Text representations are typically a bit more extensible.
 
   regards, tom lane

Actully, You litlle messed in my head. So in prev post we don't need to send 
information if grant option has been set, currently in text mode no grant 
options means ACL_NO_RIGHTS, and in binary same may be achived be settig there 
0. 

But version field may be usefull to validate this and future calls, and 
provide backward compatibility (if newer client will send less bits then rest 
of bits will be set to 0).

I think about splitting privs chain to two numbers, it may be easier to 
implement this and parse if number of privs will extend 32...

In addition I may add support for possible, future representation, where given 
privilige may be yes, no, undefined (like in Windows).

Regrads,
Radek

-- 
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] Binary in/out for aclitem

2011-02-23 Thread Radosław Smogura
Tom Lane t...@sss.pgh.pa.us Wednesday 23 February 2011 16:19:27
 rsmogura rsmog...@softperience.eu writes:
   On Tue, 22 Feb 2011 20:20:39 -0500, Tom Lane wrote:
  ...  But my question isn't about that; it's about
  why aclitem should be considered a first-class citizen.  It makes me
  uncomfortable that client apps are looking at it at all, because any
  that do are bound to get broken in the future, even assuming that
  they get the right answers today.  I wonder how many such clients are up
  to speed for per-column privileges and non-constant default privileges
  for instance.  And sepgsql is going to cut them off at the knees.
  
   Technically, at eye glance, I didn't seen in sepgsql modifications to
   acl.h. So, I think, aclitem will be unaffected. In any way sepgsql needs
   some way to present access rights to administrator it may use own model,
   or aclitem, too.
 
 You're missing the point, which is that the current internal
 representation of aclitem could change drastically to support future
 feature improvements in the area of privileges.  It has already changed
 significantly in the past (we didn't use to have WITH GRANT OPTION).
 If we had to add a field, for instance, a binary representation would
 simply be broken, as clients would have difficulty telling how to
 interpret it as soon as there was more than one possible format.
 Text representations are typically a bit more extensible.
 
   regards, tom lane

Here is extended version, has version field (N_ACL_RIGHTS*2) and reserved 
mask, as well definition is more general then def of PGSQL. In any way it 
require that rights mades bit array.

Still I tested only aclitemsend.

Btw, Is it possible and needed to add group byte, indicating that grantee is 
group or user?

Regards,
Radek
diff --git a/.gitignore b/.gitignore
index 1be11e8..0d594f9 100644
--- a/.gitignore
+++ b/.gitignore
@@ -17,3 +17,5 @@ objfiles.txt
 /GNUmakefile
 /config.log
 /config.status
+/nbproject/private/
+/nbproject
diff --git a/src/backend/utils/adt/acl.c b/src/backend/utils/adt/acl.c
index 691ba3b..c25c0fd 100644
--- a/src/backend/utils/adt/acl.c
+++ b/src/backend/utils/adt/acl.c
@@ -33,6 +33,7 @@
 #include utils/lsyscache.h
 #include utils/memutils.h
 #include utils/syscache.h
+#include libpq/pqformat.h
 
 
 typedef struct
@@ -78,6 +79,10 @@ static void putid(char *p, const char *s);
 static Acl *allocacl(int n);
 static void check_acl(const Acl *acl);
 static const char *aclparse(const char *s, AclItem *aip);
+
+/** Assigns default grantor and send warning. */
+static void aclitem_assign_default_grantor(AclItem *aip);
+
 static bool aclitem_match(const AclItem *a1, const AclItem *a2);
 static int	aclitemComparator(const void *arg1, const void *arg2);
 static void check_circularity(const Acl *old_acl, const AclItem *mod_aip,
@@ -209,6 +214,14 @@ putid(char *p, const char *s)
 	*p = '\0';
 }
 
+/** Assigns default grantor and send warning. */
+void aclitem_assign_default_grantor(AclItem *aip) {
+aip-ai_grantor = BOOTSTRAP_SUPERUSERID;
+ereport(WARNING,
+(errcode(ERRCODE_INVALID_GRANTOR),
+ errmsg(defaulting grantor to user ID %u,
+BOOTSTRAP_SUPERUSERID)));
+}
 /*
  * aclparse
  *		Consumes and parses an ACL specification of the form:
@@ -343,11 +356,7 @@ aclparse(const char *s, AclItem *aip)
 	}
 	else
 	{
-		aip-ai_grantor = BOOTSTRAP_SUPERUSERID;
-		ereport(WARNING,
-(errcode(ERRCODE_INVALID_GRANTOR),
- errmsg(defaulting grantor to user ID %u,
-		BOOTSTRAP_SUPERUSERID)));
+aclitem_assign_default_grantor(aip);
 	}
 
 	ACLITEM_SET_PRIVS_GOPTIONS(*aip, privs, goption);
@@ -643,6 +652,163 @@ aclitemout(PG_FUNCTION_ARGS)
 
 	PG_RETURN_CSTRING(out);
 }
+/** Do binary read of aclitem. Input format is same as {@link aclitem_recv}, but
+ * special algorithm is used to determine grantee's and grantor's OID. The reason
+ * is to keep backward information compatiblity with text mode - typical
+ * client (which gets instructions from user)
+ * may be much more interested in sending grantee and grantors name then
+ * OID. Detailed rule is as follow:br/
+ * If message has no name and names' length then
+ * use passed OIDs (message may be truncated, we accept this, 
+ * but both, two last fields must be not present).br/
+ * If grantee's name len or grantor's name len is {@code -1} then use respecitve
+ * OIDs.br/
+ * If name length is not {@code -1} then find OID for given part, and
+ * ensure that respective OID is {@code 0} or is equal to found OID./br
+ * If grantor's OID is {@code 0} and grantor's name lenght is {@code -1} or 
+ * truncated then assign superuser as grantor.
+ */
+Datum
+aclitemrecv(PG_FUNCTION_ARGS) {
+StringInfo	buf = (StringInfo) PG_GETARG_POINTER(0);
+AclItem*aip;
+intgRawLen;
+char   *gVal = NULL;
+int4gValLen;
+OidgOid;
+int2   numberOfAcls;
+int4   mask;

Re: [HACKERS] Binary in/out for aclitem

2011-02-23 Thread Alvaro Herrera
Excerpts from Radosław Smogura's message of mié feb 23 15:18:22 -0300 2011:

 Btw, Is it possible and needed to add group byte, indicating that grantee is 
 group or user?

There are no groups or users, only roles.

-- 
Álvaro Herrera alvhe...@commandprompt.com
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

-- 
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] Binary in/out for aclitem

2011-02-23 Thread Tom Lane
Alvaro Herrera alvhe...@commandprompt.com writes:
 Excerpts from Radosław Smogura's message of mié feb 23 15:18:22 -0300 2011:
 Btw, Is it possible and needed to add group byte, indicating that grantee is 
 group or user?

 There are no groups or users, only roles.

Even if there were, this is not part of the value of an aclitem.

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


Re: [HACKERS] Binary in/out for aclitem

2011-02-23 Thread Tom Lane
=?utf-8?q?Rados=C5=82aw_Smogura?= rsmog...@softperience.eu writes:
 Here is extended version, has version field (N_ACL_RIGHTS*2) and reserved 
 mask, as well definition is more general then def of PGSQL. In any way it 
 require that rights mades bit array.

You're going in quite the wrong direction here.  The consensus as I
understood it was that we should just use the text representation in
binary mode too, rather than inventing a separate representation that's
going to put a whole new set of constraints on what can happen to the
internal representation.  The proposal you have here has no redeeming
social value whatever, because nobody cares about the I/O efficiency
for aclitem (and even if anyone did, you've made no case that this would
actually be more efficient to use on the client side).

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


Re: [HACKERS] Binary in/out for aclitem

2011-02-22 Thread Tom Lane
=?utf-8?q?Rados=C5=82aw_Smogura?= rsmog...@softperience.eu writes:
 Actaully one more POD left it's aclitem :). In Java for e.g. it is used to 
 obtain column priviliges, I assume some folks may want to use it too.

I think this one has got far less use-case than the other, and I don't
want to expose the internal representation of ACLITEM to the world.
So, nope, not excited about it.

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


Re: [HACKERS] Binary in/out for aclitem

2011-02-22 Thread Andrew Dunstan



On 02/22/2011 05:04 PM, Tom Lane wrote:

=?utf-8?q?Rados=C5=82aw_Smogura?=rsmog...@softperience.eu  writes:

Actaully one more POD left it's aclitem :). In Java for e.g. it is used to
obtain column priviliges, I assume some folks may want to use it too.

I think this one has got far less use-case than the other, and I don't
want to expose the internal representation of ACLITEM to the world.
So, nope, not excited about it.



The sendv for enums sends the label, and ISTR there are some others that 
send the text representation also. Would that be better?


cheers

andrew

--
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] Binary in/out for aclitem

2011-02-22 Thread Tom Lane
Andrew Dunstan and...@dunslane.net writes:
 On 02/22/2011 05:04 PM, Tom Lane wrote:
 I think this one has got far less use-case than the other, and I don't
 want to expose the internal representation of ACLITEM to the world.

 The sendv for enums sends the label, and ISTR there are some others that 
 send the text representation also. Would that be better?

It'd be more future-proof than this patch, but I'm still unconvinced
about the use-case.

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


Re: [HACKERS] Binary in/out for aclitem

2011-02-22 Thread Robert Haas
On Tue, Feb 22, 2011 at 5:24 PM, Tom Lane t...@sss.pgh.pa.us wrote:
 Andrew Dunstan and...@dunslane.net writes:
 On 02/22/2011 05:04 PM, Tom Lane wrote:
 I think this one has got far less use-case than the other, and I don't
 want to expose the internal representation of ACLITEM to the world.

 The sendv for enums sends the label, and ISTR there are some others that
 send the text representation also. Would that be better?

 It'd be more future-proof than this patch, but I'm still unconvinced
 about the use-case.

Do we want to intentionally make binary format a second-class citizen?

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

-- 
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] Binary in/out for aclitem

2011-02-22 Thread Tom Lane
Robert Haas robertmh...@gmail.com writes:
 On Tue, Feb 22, 2011 at 5:24 PM, Tom Lane t...@sss.pgh.pa.us wrote:
 It'd be more future-proof than this patch, but I'm still unconvinced
 about the use-case.

 Do we want to intentionally make binary format a second-class citizen?

Well, it's not exactly a first-class citizen; compare for instance the
amount of verbiage in the docs about text I/O formats versus the amount
about binary formats.  But my question isn't about that; it's about why
aclitem should be considered a first-class citizen.  It makes me
uncomfortable that client apps are looking at it at all, because any
that do are bound to get broken in the future, even assuming that they
get the right answers today.  I wonder how many such clients are up to
speed for per-column privileges and non-constant default privileges for
instance.  And sepgsql is going to cut them off at the knees.

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


Re: [HACKERS] Binary in/out for aclitem

2011-02-22 Thread Robert Haas
[ removing Radoslaw from the CC list, as his email is bouncing every time ]

On Tue, Feb 22, 2011 at 8:20 PM, Tom Lane t...@sss.pgh.pa.us wrote:
 Robert Haas robertmh...@gmail.com writes:
 On Tue, Feb 22, 2011 at 5:24 PM, Tom Lane t...@sss.pgh.pa.us wrote:
 It'd be more future-proof than this patch, but I'm still unconvinced
 about the use-case.

 Do we want to intentionally make binary format a second-class citizen?

 Well, it's not exactly a first-class citizen; compare for instance the
 amount of verbiage in the docs about text I/O formats versus the amount
 about binary formats.  But my question isn't about that; it's about why
 aclitem should be considered a first-class citizen.  It makes me
 uncomfortable that client apps are looking at it at all, because any
 that do are bound to get broken in the future, even assuming that they
 get the right answers today.  I wonder how many such clients are up to
 speed for per-column privileges and non-constant default privileges for
 instance.  And sepgsql is going to cut them off at the knees.

Well, unfortunately, there's an awful lot of information that can only
be obtained in a reasonable way by introspection of the system
catalogs.  If you want to know whether user A can select from table B,
there's really no sensible way of obtaining that without parsing the
aclitem entries in some fashion, and unfortunately that's just the tip
of the iceberg.  One of the first applications I wrote for PG included
a tool to upgrade the production schema to match the dev schema, which
promptly got broken when (I'm dating myself here[1]) PG added support
for dropping columns (7.3) and recording the grantor on aclitems
(7.4).  I'm not going to claim that there aren't better ways of trying
to solve the problems that I was trying to solve that day, but at the
time it seemed like the best solution, and if I had a dollar for every
other person who is written a similar application, I am pretty sure I
could afford at least a pizza, if not dinner at Fogo de Chao.

Now, if you were to propose adding a well-designed set of DCL commands
to expose this kind of information to clients in a more structured
way, I would be the first to applaud.  LIST TABLES?  SHOW GRANTS TO?
Sign me up!  (I got a request for the latter just today.)  But until
then, if you need this information, you don't have much choice but to
pull it out of the system catalogs; and if JDBC would rather use
binary format to talk to the server, I don't particularly see any
reason to say no.  If we prefer to expose the text format rather
than anything else, that's OK with me, but I do think it would make
sense to expose something.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

[1] Insert obligatory joke about how no one else would.

-- 
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] Binary in/out for aclitem

2011-02-22 Thread Tom Lane
Robert Haas robertmh...@gmail.com writes:
 On Tue, Feb 22, 2011 at 8:20 PM, Tom Lane t...@sss.pgh.pa.us wrote:
 ... But my question isn't about that; it's about why
 aclitem should be considered a first-class citizen.  It makes me
 uncomfortable that client apps are looking at it at all, because any
 that do are bound to get broken in the future, even assuming that they
 get the right answers today.  I wonder how many such clients are up to
 speed for per-column privileges and non-constant default privileges for
 instance.  And sepgsql is going to cut them off at the knees.

 Well, unfortunately, there's an awful lot of information that can only
 be obtained in a reasonable way by introspection of the system
 catalogs.  If you want to know whether user A can select from table B,
 there's really no sensible way of obtaining that without parsing the
 aclitem entries in some fashion, and unfortunately that's just the tip
 of the iceberg.

Um, that question is precisely why we invented the has_foo_privilege
class of functions.  I would *much* rather see users applying those
functions than looking at ACLs directly --- and if there's some
reasonable use-case that those don't cover, let's talk about that.

 Now, if you were to propose adding a well-designed set of DCL commands
 to expose this kind of information to clients in a more structured
 way, I would be the first to applaud.  LIST TABLES?  SHOW GRANTS TO?
 Sign me up!  (I got a request for the latter just today.)  But until
 then, if you need this information, you don't have much choice but to
 pull it out of the system catalogs; and if JDBC would rather use
 binary format to talk to the server, I don't particularly see any
 reason to say no.  If we prefer to expose the text format rather
 than anything else, that's OK with me, but I do think it would make
 sense to expose something.

Well, to go back to the binary-format issue, if we're going to insist
that all standard types have binary I/O support then we should actually
do that, not accept piecemeal patches that move us incrementally in that
direction without establishing a policy.  To my mind, establishing a
policy would include adding a type_sanity regression test query that
shows there are no missing binary I/O functions.  As of HEAD, we have

postgres=# select typname,typtype from pg_type where typreceive = 0 or typsend 
= 0;
 typname  | typtype 
--+-
 smgr | b
 aclitem  | b
 gtsvector| b
 any  | p
 trigger  | p
 language_handler | p
 internal | p
 opaque   | p
 anyelement   | p
 anynonarray  | p
 anyenum  | p
 fdw_handler  | p
(12 rows)

Possibly we could exclude pseudotypes from the policy, on the grounds
there are never really values of those types anyway.  But other than
that, we have:

smgr: let's just get rid of that useless vestigial type.

aclitem: as per this thread, using the text representation as
the binary representation seems reasonable, or at least it doesn't
make anything any worse.

gtsvector: this is strictly an internal type, so it probably
doesn't need actual I/O support, but we could put in a couple of
dummy functions that just throw ERRCODE_FEATURE_NOT_SUPPORTED.

Maybe the right plan would be to give all the pseudotypes error-throwing
binary I/O functions too.  Then, if anyone lobbies for not throwing an
error (as per what we just did with void), at least it doesn't take
a catversion bump to fix it.

If someone wanted to propose doing all that, I could get behind it.
But I'm not excited about debating this one datatype at a time.

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