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
Attachments:
aclitem_binary_20110222.patchtext/x-patch; charset=UTF-8; name=aclitem_binary_20110222.patchDownload
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 <b>only</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;
+ int gRawLen;
+ char *gVal = NULL;
+ int gValLen;
+ Oid gOid;
+
+ 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 (aip->ai_grantor == 0 && gVal == NULL)
+ aclitem_assign_default_grantor(aip);
+
+ PG_RETURN_ACLITEM_P(aip);
+}
+/**
+ * aclitem_send
+ * Allocates storage for, and fills in, a new null-delimited string
+ * containing a formatted ACL specification. See aclparse for details.
+ *
+ * The output format is as follows:
+ * grantee's oid, grantor's oid
+ * rights - 32 bit number, 1st 16 bit describes grantor rights
+ * grentee's name length
+ * grantee's name
+ * grantor's name length
+ * grantor's name
+ * "Number of rights" - describes how many rights PSQL has defined. Each right
+ * has internal number and is represented by bit 1 set on n-th internal positon.
+ * First 16 (currently we have 11) rights are packed in int, for more additional
+ * byte will be added, so you may think about rights like "Number of rights" bit
+ * long number (rounded up to multiplicity of 8 bits), but no shorter then 16 bits.
+ * <br/>
+ * Magic lenght for grantee's or grantor's name -1 means that following
+ * is unnamed, which will be for {@link ACL_ID_PUBLIC}.
+ * <br/>
+ * Last elements are used to pass "usefull" information to client, so it
+ * don't need to query for those names, actually client may be more
+ * interested in getting those names, then OIDs. It will keep backward
+ * compatibility with text mode, as well.
+ */
+Datum
+aclitemsend(PG_FUNCTION_ARGS)
+{
+ AclItem *aip = PG_GETARG_ACLITEM_P(0);
+ char *oidName;
+ HeapTuple htup;
+ StringInfoData buf;
+
+ pq_begintypsend(&buf);
+
+ pq_sendint(&buf, aip->ai_grantee, 4);
+ pq_sendint(&buf, aip->ai_grantor, 4);
+ pq_sendint(&buf, aip->ai_privs, 4);
+
+ if (N_ACL_RIGHTS > 16) {
+ /* We sends rights as long (16 bit), but if in future number
+ * of rights will increase we got problem.
+ */
+ }
+
+ /** Append names at the end. */
+ if (aip->ai_grantee != ACL_ID_PUBLIC) {
+ htup = SearchSysCache1(AUTHOID, ObjectIdGetDatum(aip->ai_grantee));
+ if (HeapTupleIsValid(htup)) {
+ oidName = NameStr(((Form_pg_authid) GETSTRUCT(htup))->rolname);
+ pq_sendcountedtext(&buf, oidName, strlen(oidName), false);
+ ReleaseSysCache(htup);
+ }else {
+ pq_sendint(&buf,-1, 4);
+ }
+ }else {
+ pq_sendint(&buf, -1, 4);
+ }
+
+ htup = SearchSysCache1(AUTHOID, ObjectIdGetDatum(aip->ai_grantor));
+ if (HeapTupleIsValid(htup)) {
+ oidName = NameStr(((Form_pg_authid) GETSTRUCT(htup))->rolname);
+ pq_sendcountedtext(&buf, oidName, strlen(oidName), false);
+ ReleaseSysCache(htup);
+ }
+ else {
+ pq_sendint(&buf, -1, 4);
+ }
+
+ PG_RETURN_BYTEA_P(pq_endtypsend(&buf));
+}
/*
* aclitem_match
diff --git a/src/include/catalog/pg_proc.h b/src/include/catalog/pg_proc.h
index 8c940bb..c56d8f8 100644
--- a/src/include/catalog/pg_proc.h
+++ b/src/include/catalog/pg_proc.h
@@ -4230,6 +4230,10 @@ DATA(insert OID = 3120 ( void_recv PGNSP PGUID 12 1 0 0 f f f t f i 1 0 22
DESCR("I/O");
DATA(insert OID = 3121 ( void_send PGNSP PGUID 12 1 0 0 f f f t f i 1 0 17 "2278" _null_ _null_ _null_ _null_ void_send _null_ _null_ _null_ ));
DESCR("I/O");
+DATA(insert OID = 3122 ( aclitemrecv PGNSP PGUID 12 1 0 0 f f f t f i 1 0 1033 "2281" _null_ _null_ _null_ _null_ aclitemrecv _null_ _null_ _null_ ));
+DESCR("I/O");
+DATA(insert OID = 3123 ( aclitemsend PGNSP PGUID 12 1 0 0 f f f t f i 1 0 17 "1033" _null_ _null_ _null_ _null_ aclitemsend _null_ _null_ _null_ ));
+DESCR("I/O");
/* System-view support functions with pretty-print option */
DATA(insert OID = 2504 ( pg_get_ruledef PGNSP PGUID 12 1 0 0 f f f t f s 2 0 25 "26 16" _null_ _null_ _null_ _null_ pg_get_ruledef_ext _null_ _null_ _null_ ));
diff --git a/src/include/catalog/pg_type.h b/src/include/catalog/pg_type.h
index 9baed6c..f89ed44 100644
--- a/src/include/catalog/pg_type.h
+++ b/src/include/catalog/pg_type.h
@@ -462,7 +462,7 @@ DATA(insert OID = 1023 ( _abstime PGNSP PGUID -1 f b A f t \054 0 702 0 array_
DATA(insert OID = 1024 ( _reltime PGNSP PGUID -1 f b A f t \054 0 703 0 array_in array_out array_recv array_send - - - i x f 0 -1 0 0 _null_ _null_ ));
DATA(insert OID = 1025 ( _tinterval PGNSP PGUID -1 f b A f t \054 0 704 0 array_in array_out array_recv array_send - - - i x f 0 -1 0 0 _null_ _null_ ));
DATA(insert OID = 1027 ( _polygon PGNSP PGUID -1 f b A f t \054 0 604 0 array_in array_out array_recv array_send - - - d x f 0 -1 0 0 _null_ _null_ ));
-DATA(insert OID = 1033 ( aclitem PGNSP PGUID 12 f b U f t \054 0 0 1034 aclitemin aclitemout - - - - - i p f 0 -1 0 0 _null_ _null_ ));
+DATA(insert OID = 1033 ( aclitem PGNSP PGUID 12 f b U f t \054 0 0 1034 aclitemin aclitemout aclitemrecv aclitemsend - - - i p f 0 -1 0 0 _null_ _null_ ));
DESCR("access control list");
#define ACLITEMOID 1033
DATA(insert OID = 1034 ( _aclitem PGNSP PGUID -1 f b A f t \054 0 1033 0 array_in array_out array_recv array_send - - - i x f 0 -1 0 0 _null_ _null_ ));
diff --git a/src/include/utils/acl.h b/src/include/utils/acl.h
index 1e9cf7f..1ca959d 100644
--- a/src/include/utils/acl.h
+++ b/src/include/utils/acl.h
@@ -239,6 +239,8 @@ extern void initialize_acl(void);
*/
extern Datum aclitemin(PG_FUNCTION_ARGS);
extern Datum aclitemout(PG_FUNCTION_ARGS);
+extern Datum aclitemrecv(PG_FUNCTION_ARGS);
+extern Datum aclitemsend(PG_FUNCTION_ARGS);
extern Datum aclinsert(PG_FUNCTION_ARGS);
extern Datum aclremove(PG_FUNCTION_ARGS);
extern Datum aclcontains(PG_FUNCTION_ARGS);
=?utf-8?q?Rados=C5=82aw_Smogura?= <rsmogura@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
On 02/22/2011 05:04 PM, Tom Lane wrote:
=?utf-8?q?Rados=C5=82aw_Smogura?=<rsmogura@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
Andrew Dunstan <andrew@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
On Tue, Feb 22, 2011 at 5:24 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Andrew Dunstan <andrew@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
Robert Haas <robertmhaas@gmail.com> writes:
On Tue, Feb 22, 2011 at 5:24 PM, Tom Lane <tgl@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
[ removing Radoslaw from the CC list, as his email is bouncing every time ]
On Tue, Feb 22, 2011 at 8:20 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Robert Haas <robertmhaas@gmail.com> writes:
On Tue, Feb 22, 2011 at 5:24 PM, Tom Lane <tgl@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]Insert obligatory joke about how no one else would.) 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.
Robert Haas <robertmhaas@gmail.com> writes:
On Tue, Feb 22, 2011 at 8:20 PM, Tom Lane <tgl@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
On Tue, Feb 22, 2011 at 9:26 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
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.
OK, fair point.
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.
Well, the obvious applications are "I'd like to know who has
permissions on this item" and "I'd like to know what this user has
permissions to do".
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.
That's hard to disagree with. +1 for accepting a patch that fixes all
the remaining cases, but not anything less than that.
smgr: let's just get rid of that useless vestigial type.
Heck yeah. Even if we someday want to go back to supporting more than
one smgr, the chances that we're going to want to do it this way are
just about zero.
This part probably merits its own commit, though.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
On Tue, 22 Feb 2011 20:20:39 -0500, Tom Lane wrote:
Robert Haas <robertmhaas@gmail.com> writes:
On Tue, Feb 22, 2011 at 5:24 PM, Tom Lane <tgl@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
rsmogura <rsmogura@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
Tom Lane <tgl@sss.pgh.pa.us> Wednesday 23 February 2011 16:19:27
rsmogura <rsmogura@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.
Tom Lane <tgl@sss.pgh.pa.us> Wednesday 23 February 2011 16:19:27
rsmogura <rsmogura@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
Tom Lane <tgl@sss.pgh.pa.us> Wednesday 23 February 2011 16:19:27
rsmogura <rsmogura@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
Attachments:
aclitem_binary_20110223.patchtext/x-patch; charset=UTF-8; name=aclitem_binary_20110223.patchDownload
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;
+ int gRawLen;
+ char *gVal = NULL;
+ int4 gValLen;
+ Oid gOid;
+ int2 numberOfAcls;
+ int4 mask;
+
+ aip = (AclItem *) palloc(sizeof(AclItem));
+ aip->ai_grantee = pq_getmsgint(buf, 4);
+ aip->ai_grantor = pq_getmsgint(buf, 4);
+ numberOfAcls = pq_getmsgint(buf, 2);
+ if (numberOfAcls != N_ACL_RIGHTS * 2) {
+ 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")));
+ aip->ai_privs = ACL_NO_RIGHTS;
+ PG_RETURN_ACLITEM_P(aip);
+ }
+
+ aip->ai_privs = pq_getmsgint(buf, 4);
+ mask = 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 (aip->ai_grantor == 0 && gVal == NULL)
+ aclitem_assign_default_grantor(aip);
+
+ PG_RETURN_ACLITEM_P(aip);
+}
+/**
+ * The output format is as follows:
+ * <ol>
+ * <li>grantee's oid</li>
+ * <li>grantor's oid</li>
+ * <li>number of rights (describes priviliges set version) (short)</li>
+ * <li>rights - N-bit number</li>
+ * <li>mask (reserved for future usage) - N-bit number</li>
+ * <li>grentee's name length</li>
+ * <li>grantee's name</li>
+ * <li>grantor's name length</li>
+ * <li>grantor's name</li>
+ * </ol>
+ * "Number of rights" - describes how many rights PSQL has defined. Each right
+ * has internal number <i>n</i> and is represented by bit 1 set on <i>n</i>-th
+ * internal positon. Currently we have 24 rigths (12 base and 12 grantor)<br/>
+ * N is lowest number such that {@code N%8 == 0 && N >= 32 && N >= "Number of rights"
+ * <br/>
+ * <b>Backend implementation notice</b>
+ * Currently, when I wrote this
+ * rights indexed from 1-12 represents "base" privilege, rights 16-29 represents
+ * privilige to grant base privilige. So, rights 13-15, 30-32 are reserved for
+ * future usage.
+ * <br/>
+ * Magic lenght for grantee's or grantor's name -1 means that respecitive
+ * is unnamed, which will be, for example, for {@link ACL_ID_PUBLIC}.
+ * <br/>
+ * Last elements are used to pass "usefull" information to client, so it
+ * don't need to query for those names, actually client may be more
+ * interested in getting those names, then OIDs. It will keep backward
+ * compatibility with text mode, as well.
+ */
+Datum
+aclitemsend(PG_FUNCTION_ARGS)
+{
+ AclItem *aip = PG_GETARG_ACLITEM_P(0);
+ char *oidName;
+ HeapTuple htup;
+ StringInfoData buf;
+ //Here we use simplification of documentation, because of we have < 16 priviliges
+ pq_begintypsend(&buf);
+ pq_sendint(&buf, aip->ai_grantee, 4);
+ pq_sendint(&buf, aip->ai_grantor, 4);
+ //Version
+ pq_sendint(&buf, N_ACL_RIGHTS*2, 2);
+ pq_sendint(&buf, aip->ai_privs, 4);
+ //Mask currently all 1
+ pq_sendint(&buf, 0xFFffFFff, 4);
+ if (N_ACL_RIGHTS > 16) {
+ /* We sends rights as long (16 bit), but if in future number
+ * of rights will increase we got problem.
+ */
+ }
+
+ /** Append names at the end. */
+ if (aip->ai_grantee != ACL_ID_PUBLIC) {
+ htup = SearchSysCache1(AUTHOID, ObjectIdGetDatum(aip->ai_grantee));
+ if (HeapTupleIsValid(htup)) {
+ oidName = NameStr(((Form_pg_authid) GETSTRUCT(htup))->rolname);
+ pq_sendcountedtext(&buf, oidName, strlen(oidName), false);
+ ReleaseSysCache(htup);
+ }else {
+ pq_sendint(&buf,-1, 4);
+ }
+ }else {
+ pq_sendint(&buf, -1, 4);
+ }
+
+ htup = SearchSysCache1(AUTHOID, ObjectIdGetDatum(aip->ai_grantor));
+ if (HeapTupleIsValid(htup)) {
+ oidName = NameStr(((Form_pg_authid) GETSTRUCT(htup))->rolname);
+ pq_sendcountedtext(&buf, oidName, strlen(oidName), false);
+ ReleaseSysCache(htup);
+ }
+ else {
+ pq_sendint(&buf, -1, 4);
+ }
+
+ PG_RETURN_BYTEA_P(pq_endtypsend(&buf));
+}
/*
* aclitem_match
diff --git a/src/include/catalog/pg_proc.h b/src/include/catalog/pg_proc.h
index 8c940bb..c56d8f8 100644
--- a/src/include/catalog/pg_proc.h
+++ b/src/include/catalog/pg_proc.h
@@ -4230,6 +4230,10 @@ DATA(insert OID = 3120 ( void_recv PGNSP PGUID 12 1 0 0 f f f t f i 1 0 22
DESCR("I/O");
DATA(insert OID = 3121 ( void_send PGNSP PGUID 12 1 0 0 f f f t f i 1 0 17 "2278" _null_ _null_ _null_ _null_ void_send _null_ _null_ _null_ ));
DESCR("I/O");
+DATA(insert OID = 3122 ( aclitemrecv PGNSP PGUID 12 1 0 0 f f f t f i 1 0 1033 "2281" _null_ _null_ _null_ _null_ aclitemrecv _null_ _null_ _null_ ));
+DESCR("I/O");
+DATA(insert OID = 3123 ( aclitemsend PGNSP PGUID 12 1 0 0 f f f t f i 1 0 17 "1033" _null_ _null_ _null_ _null_ aclitemsend _null_ _null_ _null_ ));
+DESCR("I/O");
/* System-view support functions with pretty-print option */
DATA(insert OID = 2504 ( pg_get_ruledef PGNSP PGUID 12 1 0 0 f f f t f s 2 0 25 "26 16" _null_ _null_ _null_ _null_ pg_get_ruledef_ext _null_ _null_ _null_ ));
diff --git a/src/include/catalog/pg_type.h b/src/include/catalog/pg_type.h
index 9baed6c..f89ed44 100644
--- a/src/include/catalog/pg_type.h
+++ b/src/include/catalog/pg_type.h
@@ -462,7 +462,7 @@ DATA(insert OID = 1023 ( _abstime PGNSP PGUID -1 f b A f t \054 0 702 0 array_
DATA(insert OID = 1024 ( _reltime PGNSP PGUID -1 f b A f t \054 0 703 0 array_in array_out array_recv array_send - - - i x f 0 -1 0 0 _null_ _null_ ));
DATA(insert OID = 1025 ( _tinterval PGNSP PGUID -1 f b A f t \054 0 704 0 array_in array_out array_recv array_send - - - i x f 0 -1 0 0 _null_ _null_ ));
DATA(insert OID = 1027 ( _polygon PGNSP PGUID -1 f b A f t \054 0 604 0 array_in array_out array_recv array_send - - - d x f 0 -1 0 0 _null_ _null_ ));
-DATA(insert OID = 1033 ( aclitem PGNSP PGUID 12 f b U f t \054 0 0 1034 aclitemin aclitemout - - - - - i p f 0 -1 0 0 _null_ _null_ ));
+DATA(insert OID = 1033 ( aclitem PGNSP PGUID 12 f b U f t \054 0 0 1034 aclitemin aclitemout aclitemrecv aclitemsend - - - i p f 0 -1 0 0 _null_ _null_ ));
DESCR("access control list");
#define ACLITEMOID 1033
DATA(insert OID = 1034 ( _aclitem PGNSP PGUID -1 f b A f t \054 0 1033 0 array_in array_out array_recv array_send - - - i x f 0 -1 0 0 _null_ _null_ ));
diff --git a/src/include/utils/acl.h b/src/include/utils/acl.h
index 1e9cf7f..1ca959d 100644
--- a/src/include/utils/acl.h
+++ b/src/include/utils/acl.h
@@ -239,6 +239,8 @@ extern void initialize_acl(void);
*/
extern Datum aclitemin(PG_FUNCTION_ARGS);
extern Datum aclitemout(PG_FUNCTION_ARGS);
+extern Datum aclitemrecv(PG_FUNCTION_ARGS);
+extern Datum aclitemsend(PG_FUNCTION_ARGS);
extern Datum aclinsert(PG_FUNCTION_ARGS);
extern Datum aclremove(PG_FUNCTION_ARGS);
extern Datum aclcontains(PG_FUNCTION_ARGS);
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 <alvherre@commandprompt.com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support
Alvaro Herrera <alvherre@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
=?utf-8?q?Rados=C5=82aw_Smogura?= <rsmogura@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
On Wed, Feb 23, 2011 at 3:30 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
=?utf-8?q?Rados=C5=82aw_Smogura?= <rsmogura@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
On Thu, 24 Feb 2011 08:38:35 -0600, Merlin Moncure wrote:
On Wed, Feb 23, 2011 at 3:30 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
=?utf-8?q?Rados=C5=82aw_Smogura?= <rsmogura@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
Tom Lane <tgl@sss.pgh.pa.us> Wednesday 23 February 2011 22:30:04
=?utf-8?q?Rados=C5=82aw_Smogura?= <rsmogura@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
Attachments:
aclitem_binary_20110224.patchtext/x-patch; charset=UTF-8; name=aclitem_binary_20110224.patchDownload
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)),
+ errmsg("extra garbage at the end of the ACL specification")));
+
+ PG_RETURN_ACLITEM_P(aip);
+}
/*
* aclitemin
* Allocates storage for, and fills in, a new AclItem given a string
@@ -559,19 +579,9 @@ check_acl(const Acl *acl)
Datum
aclitemin(PG_FUNCTION_ARGS)
{
- const char *s = PG_GETARG_CSTRING(0);
- AclItem *aip;
-
- aip = (AclItem *) palloc(sizeof(AclItem));
- s = aclparse(s, aip);
- while (isspace((unsigned char) *s))
- ++s;
- if (*s)
- ereport(ERROR,
- (errcode(ERRCODE_INVALID_TEXT_REPRESENTATION),
- errmsg("extra garbage at the end of the ACL specification")));
-
- PG_RETURN_ACLITEM_P(aip);
+ const char *s = PG_GETARG_CSTRING(0);
+
+ PG_RETURN_ACLITEM_P(aclitem_common_in_recv(s, false));
}
/*
@@ -644,6 +654,35 @@ aclitemout(PG_FUNCTION_ARGS)
PG_RETURN_CSTRING(out);
}
+/** Pass call to {@link aclitem_common_in_recv} */
+Datum
+aclitemrecv(PG_FUNCTION_ARGS) {
+ StringInfo buf = (StringInfo) PG_GETARG_POINTER(0);
+ char *str;
+ int nbytes;
+ AclItem *out;
+
+ str = pq_getmsgtext(buf, buf->len - buf->cursor, &nbytes);
+ out = (AclItem*) aclitem_common_in_recv(str, true);
+ pfree(str);
+
+ PG_RETURN_ACLITEM_P(out);
+}
+
+/** Pass call to {@link aclitemout}. */
+Datum
+aclitemsend(PG_FUNCTION_ARGS)
+{
+ StringInfoData buf;
+ const char *aclText;
+
+ pq_begintypsend(&buf);
+ aclText = (const char*) aclitemout(fcinfo);
+ pq_sendtext(&buf, aclText, strlen(aclText));
+
+ PG_RETURN_BYTEA_P(pq_endtypsend(&buf));
+}
+
/*
* aclitem_match
* Two AclItems are considered to match iff they have the same
diff --git a/src/include/catalog/pg_proc.h b/src/include/catalog/pg_proc.h
index 8c940bb..c56d8f8 100644
--- a/src/include/catalog/pg_proc.h
+++ b/src/include/catalog/pg_proc.h
@@ -4230,6 +4230,10 @@ DATA(insert OID = 3120 ( void_recv PGNSP PGUID 12 1 0 0 f f f t f i 1 0 22
DESCR("I/O");
DATA(insert OID = 3121 ( void_send PGNSP PGUID 12 1 0 0 f f f t f i 1 0 17 "2278" _null_ _null_ _null_ _null_ void_send _null_ _null_ _null_ ));
DESCR("I/O");
+DATA(insert OID = 3122 ( aclitemrecv PGNSP PGUID 12 1 0 0 f f f t f i 1 0 1033 "2281" _null_ _null_ _null_ _null_ aclitemrecv _null_ _null_ _null_ ));
+DESCR("I/O");
+DATA(insert OID = 3123 ( aclitemsend PGNSP PGUID 12 1 0 0 f f f t f i 1 0 17 "1033" _null_ _null_ _null_ _null_ aclitemsend _null_ _null_ _null_ ));
+DESCR("I/O");
/* System-view support functions with pretty-print option */
DATA(insert OID = 2504 ( pg_get_ruledef PGNSP PGUID 12 1 0 0 f f f t f s 2 0 25 "26 16" _null_ _null_ _null_ _null_ pg_get_ruledef_ext _null_ _null_ _null_ ));
diff --git a/src/include/catalog/pg_type.h b/src/include/catalog/pg_type.h
index 9baed6c..f89ed44 100644
--- a/src/include/catalog/pg_type.h
+++ b/src/include/catalog/pg_type.h
@@ -462,7 +462,7 @@ DATA(insert OID = 1023 ( _abstime PGNSP PGUID -1 f b A f t \054 0 702 0 array_
DATA(insert OID = 1024 ( _reltime PGNSP PGUID -1 f b A f t \054 0 703 0 array_in array_out array_recv array_send - - - i x f 0 -1 0 0 _null_ _null_ ));
DATA(insert OID = 1025 ( _tinterval PGNSP PGUID -1 f b A f t \054 0 704 0 array_in array_out array_recv array_send - - - i x f 0 -1 0 0 _null_ _null_ ));
DATA(insert OID = 1027 ( _polygon PGNSP PGUID -1 f b A f t \054 0 604 0 array_in array_out array_recv array_send - - - d x f 0 -1 0 0 _null_ _null_ ));
-DATA(insert OID = 1033 ( aclitem PGNSP PGUID 12 f b U f t \054 0 0 1034 aclitemin aclitemout - - - - - i p f 0 -1 0 0 _null_ _null_ ));
+DATA(insert OID = 1033 ( aclitem PGNSP PGUID 12 f b U f t \054 0 0 1034 aclitemin aclitemout aclitemrecv aclitemsend - - - i p f 0 -1 0 0 _null_ _null_ ));
DESCR("access control list");
#define ACLITEMOID 1033
DATA(insert OID = 1034 ( _aclitem PGNSP PGUID -1 f b A f t \054 0 1033 0 array_in array_out array_recv array_send - - - i x f 0 -1 0 0 _null_ _null_ ));
diff --git a/src/include/utils/acl.h b/src/include/utils/acl.h
index 1e9cf7f..1ca959d 100644
--- a/src/include/utils/acl.h
+++ b/src/include/utils/acl.h
@@ -239,6 +239,8 @@ extern void initialize_acl(void);
*/
extern Datum aclitemin(PG_FUNCTION_ARGS);
extern Datum aclitemout(PG_FUNCTION_ARGS);
+extern Datum aclitemrecv(PG_FUNCTION_ARGS);
+extern Datum aclitemsend(PG_FUNCTION_ARGS);
extern Datum aclinsert(PG_FUNCTION_ARGS);
extern Datum aclremove(PG_FUNCTION_ARGS);
extern Datum aclcontains(PG_FUNCTION_ARGS);