Re: [HACKERS] Binary in/out for aclitem
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
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
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
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
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
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
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
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
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
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
=?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
[HACKERS] Binary in/out for aclitem
Hi,
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 tested only recv :-(
Acually I don't know if idea of such format is OK, but my intention was to
send roles names, so driver don't need to ask for name (like for regproc) and
to keep together human-based approch (text) and binary approch.
Kind 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..33877cb 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,143 @@ 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 contains bonly/b oids and privs (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;
+intgValLen;
+OidgOid;
+
+aip = (AclItem *) palloc(sizeof(AclItem));
+aip-ai_grantee = pq_getmsgint(buf, 4);
+aip-ai_grantor = pq_getmsgint(buf, 4);
+aip-ai_privs = pq_getmsgint(buf, 4);
+
+/* Client has passed names. */
+if (buf-cursor buf-len) {
+/* Read grantee. */
+gRawLen = pq_getmsgint(buf, 4);
+if (gRawLen != -1) {
+gVal = pq_getmsgtext(buf, gRawLen, gValLen);
+gOid = get_role_oid(gVal, false);
+if (aip-ai_grantee != 0 aip-ai_grantee != gOid) {
+ereport(ERROR,
+(errcode(ERRCODE_INVALID_BINARY_REPRESENTATION),
+errmsg(grantee's OID is not 0 and passed grantee's name and grantee's OID doesn't match)));
+
+}
+}
+
+/* Read grantor. */
+gRawLen = pq_getmsgint(buf, 4);
+if (gRawLen != -1) {
+gVal = pq_getmsgtext(buf, gRawLen, gValLen);
+gOid = get_role_oid(gVal, false);
+if (aip-ai_grantor != 0 aip-ai_grantor != gOid) {
+ereport(ERROR,
+(errcode(ERRCODE_INVALID_BINARY_REPRESENTATION),
+errmsg(grantor's OID is not 0 and passed grantor's name and grantor's OID doesn't match)));
+}
+}else {
+gVal = NULL;
+}
+}
+
+if
Re: [HACKERS] Binary in/out for aclitem
=?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
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
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
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
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
[ 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
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
