pgsql-server/src backend/utils/adt/acl.c inclu ...

On 26-Apr-04, at 11:06 AM, Bruce Momjian wrote:

Please find a attached a small patch that adds accessor functions
for "aclitem" so that it is not an opaque datatype.

Shouldn't this patch include some documentation updates?

-Neil

Please find a attached a small patch that adds accessor functions
for "aclitem" so that it is not an opaque datatype.

Shouldn't this patch include some documentation updates?

Well I thought about that, but I wasn't sure were to put it.
There is no documentation at all about the aclitem data type.
I'll submit a few lines anyway.

--
Fabien Coelho - coelho@cri.ensmp.fr

Neil Conway wrote:

On 26-Apr-04, at 11:06 AM, Bruce Momjian wrote:

Please find a attached a small patch that adds accessor functions
for "aclitem" so that it is not an opaque datatype.

Shouldn't this patch include some documentation updates?

I talked to the author about this. Turns out none of our permission
functions have docs. I figure if folks want to use them, they will use
psql \df to find them.

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

Bruce Momjian <pgman@candle.pha.pa.us> writes:

Neil Conway wrote:

Shouldn't this patch include some documentation updates?

I talked to the author about this. Turns out none of our permission
functions have docs.

Really? What is Table 9.37 in
http://developer.postgresql.org/docs/postgres/functions-misc.html
Arguably these functions do not belong right there, but that's hardly a
reason to think that they do not need documentation.

Personally, though, I think that Peter's original objection was right
on. We shouldn't be exporting these functions at all; it is right to
treat aclitem as an opaque type. The problem with allowing computations
on aclitems to occur in client-side code is that we will be locking
ourselves into the present representation of access rights, which is
pretty durn foolish considering that we *know* we need to make changes
in that area pretty soon to move closer to SQL compliance (the whole
users/groups/roles business). The correct approach is not to export
low-level access and put functionality in the client, but to put the
functionality on the server side where it's convenient to change it
at the same time we reimplement ACLs.

Ergo, my recommendation is to revert this change altogether. Fabien
should figure out the high-level description of what he wants to know
(at a level similar to has_table_privilege() and its ilk) and propose
server-side functions to implement that.

regards, tom lane

Dear Tom,

http://developer.postgresql.org/docs/postgres/functions-misc.html
Arguably these functions do not belong right there, but that's hardly a
reason to think that they do not need documentation.

Sure. I was planing to add something anyway.

Personally, though, I think that Peter's original objection was right
on. We shouldn't be exporting these functions at all; it is right to
treat aclitem as an opaque type.

I disagree strongly on this point.

As I stated previously, it should a general principle that all information
about internal configuration should be available from SQL, and better if
in a relationnal form. This is among the "10 rules" of what a relationnal
DB should do, as far as I can remember.

Otherwise, it means that you do not trust SQL and the relationnal DB
as a general tool. As a leading developper in a RDB system, I cannot
believe that;-)

The problem with allowing computations on aclitems to occur in
client-side code

I'm developping some "pg_advisor" stuff to check for many things in the
database. YOU decided that all that should not be on the server side.
Fine, I agree. Now if you want to keep things opaque in the server...

is that we will be locking ourselves into the present representation of
access rights, which is pretty durn foolish.

I perfectly agree with you on this point;-)

The "pg_hba.conf" code is pretty disappointing. I mean by that low
level, no real internal data structure

The aclitem stuff violates all rules I teach to my student about
sound design: it is a array (no NF1) the same field references keys
in different arrays, a null array means something implicitly, aso.

considering that we *know* we need to make changes in that area pretty
soon to move closer to SQL compliance (the whole users/groups/roles
business). The correct approach is not to export low-level access and
put functionality in the client, but to put the functionality on the
server side where it's convenient to change it at the same time we
reimplement ACLs.

Well, it would be no big stuff to adapt this.

Ergo, my recommendation is to revert this change altogether.

You're the boss.

Fabien should figure out the high-level description of what he wants to
know (at a level similar to has_table_privilege() and its ilk) and
propose server-side functions to implement that.

Sure, I did that already.

I built a plpgsql functions to return appropriate relations that I can
query. However this plpgsql needs to access your "opaque" type. I can load
the functions, but they seem to me that they belong to the backend.

"has_*_privileges()" is NOT relationnal as it hides queries, so it does
not really suit queries that want to deal with all possible users/groups
and all possible objects. Moreover, I need access to the raw information
to check for its consistency, not the derived functionnal stuff.

--
Fabien Coelho - coelho@cri.ensmp.fr

Tom Lane wrote:

Ergo, my recommendation is to revert this change altogether. Fabien
should figure out the high-level description of what he wants to know
(at a level similar to has_table_privilege() and its ilk) and propose
server-side functions to implement that.

I agree with that.

OK, patch reverted at request of Tom and Peter. Please propose a more
generalitzed soluion. Thanks.

---------------------------------------------------------------------------

Fabien COELHO wrote:

Dear Tom,

http://developer.postgresql.org/docs/postgres/functions-misc.html
Arguably these functions do not belong right there, but that's hardly a
reason to think that they do not need documentation.

Sure. I was planing to add something anyway.

Personally, though, I think that Peter's original objection was right
on. We shouldn't be exporting these functions at all; it is right to
treat aclitem as an opaque type.

I disagree strongly on this point.

As I stated previously, it should a general principle that all information
about internal configuration should be available from SQL, and better if
in a relationnal form. This is among the "10 rules" of what a relationnal
DB should do, as far as I can remember.

Otherwise, it means that you do not trust SQL and the relationnal DB
as a general tool. As a leading developper in a RDB system, I cannot
believe that;-)

The problem with allowing computations on aclitems to occur in
client-side code

I'm developping some "pg_advisor" stuff to check for many things in the
database. YOU decided that all that should not be on the server side.
Fine, I agree. Now if you want to keep things opaque in the server...

is that we will be locking ourselves into the present representation of
access rights, which is pretty durn foolish.

I perfectly agree with you on this point;-)

The "pg_hba.conf" code is pretty disappointing. I mean by that low
level, no real internal data structure

The aclitem stuff violates all rules I teach to my student about
sound design: it is a array (no NF1) the same field references keys
in different arrays, a null array means something implicitly, aso.

considering that we *know* we need to make changes in that area pretty
soon to move closer to SQL compliance (the whole users/groups/roles
business). The correct approach is not to export low-level access and
put functionality in the client, but to put the functionality on the
server side where it's convenient to change it at the same time we
reimplement ACLs.

Well, it would be no big stuff to adapt this.

Ergo, my recommendation is to revert this change altogether.

You're the boss.

Fabien should figure out the high-level description of what he wants to
know (at a level similar to has_table_privilege() and its ilk) and
propose server-side functions to implement that.

Sure, I did that already.

I built a plpgsql functions to return appropriate relations that I can
query. However this plpgsql needs to access your "opaque" type. I can load
the functions, but they seem to me that they belong to the backend.

"has_*_privileges()" is NOT relationnal as it hides queries, so it does
not really suit queries that want to deal with all possible users/groups
and all possible objects. Moreover, I need access to the raw information
to check for its consistency, not the derived functionnal stuff.

--
Fabien Coelho - coelho@cri.ensmp.fr

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

OK, patch reverted at request of Tom and Peter. Please propose a more
generalitzed soluion. Thanks.

Sigh.

You refuse a 10 lines patch to access a stupid opaque type in the backend.
You both refuse some things to be done in the back end, and also to add
what is needed to do it in userland. Moreover, I don't think this patch
did hurt anybody, as it was pretty invisible and was useful to me.

I'm tired. When I'll be too tired, you'll just lose a contributor. A very
small loss indeed, but I don't think it is a good policy for your project.

Have a nice day,

--
Fabien Coelho - coelho@cri.ensmp.fr

Dear committers, dear hackers,

Subject: Re: [COMMITTERS] pgsql-server/src backend/utils/adt/acl.c ...

Ergo, my recommendation is to revert this change altogether. Fabien
should figure out the high-level description of what he wants to know

Please find attached as somehow requested a plpgsql implementation for a
"high-level description" (by that, I understand "relationnal", not
"functionnal") of acl in postgres.

The pg_{database,class,namespace,language,proc}_acl views are just
intermediate for the construction of the description from current acl
implementation. I'm not sure the implementation is right about the default
settings, but the spirit is there.

The actual descriptions are pg_{public,group,user}_acl, and pg_granted_acl
and pg_acl are examples of how to use these "high level descriptions".

You may notice that "high level" means two different things. High level
functions from the back-end point of view (has_privileves stuff), and high
level relationnal (something you can query). I need the second stuff.

Also, I must admit that I don't find it really motivating to have to
reimplement all this in C and to have it rejected for some reason such as
"we may change things in this area in some hypothetical future time", as
it was the motivation for rejecting 10 lines of code for 5 aclitem
accessor functions.

A general comment about pg_catalog is that it looks like it was designed
by a C programmer and cast later as an afterthought to a relationnal view.
It makes it quite uneasy to manipulate these tables for any other purpose
that the one that was foreseen by the designer from its internal point of
view, especially as it is not normalized and as opaque types are used.

Anyway, thanks in advance for your comments about this description, and
suggestions about the probability of acceptance it could have (if
implemented properly in C) in the backend, so as to replace quite infamous
aclitem accessors.

Have a nice day,

--
Fabien Coelho - coelho@cri.ensmp.fr

Fabien COELHO wrote:

Please find attached as somehow requested a plpgsql implementation
for a "high-level description" (by that, I understand "relationnal",
not "functionnal") of acl in postgres.

That doesn't tell me anything. The functionality of the ACL system is
to answer questions like "does user X have privilege Y on object Z".
It seems that this question can be answered using existing facilities.
Additionally we have information schema views that list existing
privileges, and those views are defined using existing facilities. It
appears that your tables mostly duplicate that. Can you say in words
what information you are missing?

Dear Peter,

The functionality of the ACL system is to answer questions like "does
user X have privilege Y on object Z".

ONE OF the functionality, and THE functionality from the backend point
of view.

As I'm developing slowly a "pg_advisor" schema to check for various
consistency issues. For instance, "does all foreign key checks have
relevant indexes", things like that. I'm also considering writing views to
check for inconsistencies in the granted rights.

It seems that this question can be answered using existing facilities.

Sure. But this is a functional interface for answering *one* specific
question, what is fine from the backend standpoint. The functions
hide queries to the raw data.

I'm rather trying to find *all* possible answers to some questions, so
typically this would be done with one single large query. For instance,
"is there a granted right in the system where the grantor does not have
the relevant grant options right?". The has_* interface does not allow
to answer to this question, I need to go back to the raw data.

As I already said, querying an array of opaque type is difficult.

Additionally we have information schema views that list existing
privileges, and those views are defined using existing facilities. It
appears that your tables mostly duplicate that.

Well, that is indeed possible to a partial extent.

On one of the first discussion about the advisor stuff, it appeared that
it was more interesting to rely on pg_catalog than on information_schema,
so I'm looking only at pg_catalog at the time.

One argument was that specific informations are only available in
pg_catalog. For instance, I need to check for acl about groups, but group
is pg-specific and does not appear in information_schema.

I hope that my intent is clearer, have a nice day,

--
Fabien Coelho - coelho@cri.ensmp.fr

Fabien COELHO wrote:

OK, patch reverted at request of Tom and Peter. Please propose a more
generalitzed soluion. Thanks.

Sigh.

You refuse a 10 lines patch to access a stupid opaque type in the backend.
You both refuse some things to be done in the back end, and also to add
what is needed to do it in userland. Moreover, I don't think this patch
did hurt anybody, as it was pretty invisible and was useful to me.

The point where it will hurt is not now, but when or if we need to
change the internal implementation of the entire rights system. Because
at the time we rip out the whole ACL, you or somebody else will ask
absolutely justified for backward compatibility. The reason to keep
things as opaque is the freedom this gives to change implementation
details without asking if it could possibly break existing code.

I'm tired. When I'll be too tired, you'll just lose a contributor. A very
small loss indeed, but I don't think it is a good policy for your project.

I do understand the frustration, but I hope you understand the larger
scale of problems that would be created if your patch got accepted.

Jan

--
#======================================================================#
# It's easier to get forgiveness for being wrong than for being right. #
# Let's break this rule - forgive me. #
#================================================== JanWieck@Yahoo.com #

I agree with you that work is needed in the acl area.

I hope you understand the larger scale of problems that would be created
if your patch got accepted.

I would be ready to bet that I would be the only user for these functions.
So large looks like me alone;-)

Have a nice day,

--
Fabien Coelho - coelho@cri.ensmp.fr