[PATCH] Largeobject access controls

KaiGai Kohei <kaigai@ak.jp.nec.com> wrote:

The pg_largeobject system catalog is reworked to manage its metadata.

CATALOG(pg_largeobject,2613)
{
Oid loowner; /* OID of the owner */
Oid lochunk; /* OID of the data chunks */
aclitem loacl[1]; /* access permissions */
} FormData_pg_largeobject;

Actual data chunks are stored in the toast relation of pg_largeobject,
and its chunk_id is stored in the pg_largeobject.lochunk.

A bit acrobatic, but insteresting idea.

I have some random comments:

* Do you measure performance of the new LO implementation?
AFAIK, Users expect performance benefits to LO; TOASTed
bytea slows down when the size of data is 100KB or greater
even if they don't use random seeks.

* We might take care of DROP ROLE and REASSIGN/DROP OWNED.
Or, we might have large object without owner.
It might require full-scanning of pg_largeobject table,
but we can accept it because the size of pg_largeobject
will be smaller; we have actual data out of the table.

* Don't we also need "ALTER LARGE OBJECT <oid> OWNER TO <user>"
for consistency?

Regards,
---
ITAGAKI Takahiro
NTT Open Source Software Center

Itagaki Takahiro wrote:

KaiGai Kohei <kaigai@ak.jp.nec.com> wrote:

The pg_largeobject system catalog is reworked to manage its metadata.

CATALOG(pg_largeobject,2613)
{
Oid loowner; /* OID of the owner */
Oid lochunk; /* OID of the data chunks */
aclitem loacl[1]; /* access permissions */
} FormData_pg_largeobject;

Actual data chunks are stored in the toast relation of pg_largeobject,
and its chunk_id is stored in the pg_largeobject.lochunk.

A bit acrobatic, but insteresting idea.

I have some random comments:

* Do you measure performance of the new LO implementation?
AFAIK, Users expect performance benefits to LO; TOASTed
bytea slows down when the size of data is 100KB or greater
even if they don't use random seeks.

Not yet. Can you recommend commonly-used workload?

* We might take care of DROP ROLE and REASSIGN/DROP OWNED.
Or, we might have large object without owner.
It might require full-scanning of pg_largeobject table,
but we can accept it because the size of pg_largeobject
will be smaller; we have actual data out of the table.

I think it should be implemented using the dependency mechanism.
It requires full-scanning on the pg_shdepend tables, but it has
been accepted.

* Don't we also need "ALTER LARGE OBJECT <oid> OWNER TO <user>"
for consistency?

Agreed. It will be also necessary to implement REASSIGN OWNED.

Thanks,
--
OSS Platform Development Division, NEC
KaiGai Kohei <kaigai@ak.jp.nec.com>

KaiGai Kohei <kaigai@ak.jp.nec.com> writes:

The attached patch provides access control features on largeobject.
This patch adds the ownership and two permissions (SELECT and UPDATE) on
largeobjects. The two permissions controls reader and writer accesses to
the largeobejcts.

What about DELETE permissions? Should we track that separately from
UPDATE?

The CREATE USER/ROLE statement got a new option: LARGEOBJECT/NOLARGEOBJECT.
It enables to controls whether the user can create a largeobject, or not.

I don't think this is necessary or appropriate.

The pg_largeobject system catalog is reworked to manage its metadata.
Actual data chunks are stored in the toast relation of pg_largeobject,

This seems like a very confusing design, and one that (a) breaks
existing code to no purpose, (b) will greatly complicate in-place
upgrade. Instead of abusing a toast relation to do something
nonstandard, keep pg_largeobject as it is now and add a new, separate
catalog that carries ownership and permissions info for each LO OID.

regards, tom lane

Tom Lane wrote:

KaiGai Kohei <kaigai@ak.jp.nec.com> writes:

The attached patch provides access control features on largeobject.
This patch adds the ownership and two permissions (SELECT and UPDATE) on
largeobjects. The two permissions controls reader and writer accesses to
the largeobejcts.

What about DELETE permissions? Should we track that separately from
UPDATE?

PostgreSQL checks ownership of the database object when user tries to
drop it. This patch also add pg_largeobject_ownercheck() on lo_unlink().

The CREATE USER/ROLE statement got a new option: LARGEOBJECT/NOLARGEOBJECT.
It enables to controls whether the user can create a largeobject, or not.

I don't think this is necessary or appropriate.

What should control privilege to create a new largeobject?
Or, it implicitly allows everyone to create a new one?

The pg_largeobject system catalog is reworked to manage its metadata.
Actual data chunks are stored in the toast relation of pg_largeobject,

This seems like a very confusing design, and one that (a) breaks
existing code to no purpose, (b) will greatly complicate in-place
upgrade. Instead of abusing a toast relation to do something
nonstandard, keep pg_largeobject as it is now and add a new, separate
catalog that carries ownership and permissions info for each LO OID.

It was the original design just before the first commit fest. :-)
I don't have any reason to oppose to it.

Thanks,
--
KaiGai Kohei <kaigai@kaigai.gr.jp>

KaiGai Kohei <kaigai@kaigai.gr.jp> writes:

Tom Lane wrote:

What about DELETE permissions? Should we track that separately from
UPDATE?

PostgreSQL checks ownership of the database object when user tries to
drop it. This patch also add pg_largeobject_ownercheck() on lo_unlink().

Oh, okay, that will do fine.

The CREATE USER/ROLE statement got a new option: LARGEOBJECT/NOLARGEOBJECT.
It enables to controls whether the user can create a largeobject, or not.

I don't think this is necessary or appropriate.

What should control privilege to create a new largeobject?
Or, it implicitly allows everyone to create a new one?

We have not had any requests to keep people from creating LOs, so I
think we can just implicitly allow everyone. If we were going to try
to manage it, I don't think a role attribute is a very good solution.
It's not grantable or inheritable, it can't be managed per-database,
etc. So I'd leave this out until there's some popular demand.

regards, tom lane

The CREATE USER/ROLE statement got a new option: LARGEOBJECT/NOLARGEOBJECT.
It enables to controls whether the user can create a largeobject, or not.

I don't think this is necessary or appropriate.

What should control privilege to create a new largeobject?
Or, it implicitly allows everyone to create a new one?

We have not had any requests to keep people from creating LOs, so I
think we can just implicitly allow everyone. If we were going to try
to manage it, I don't think a role attribute is a very good solution.
It's not grantable or inheritable, it can't be managed per-database,
etc. So I'd leave this out until there's some popular demand.

OK, I'll keep the current behavior (it allows everyone to create it).

BTW, currently, the default ACL of largeobject allows anything for owner
and nothing for world. Do you have any comment for the default behavior?

Thanks,
--
KaiGai Kohei <kaigai@kaigai.gr.jp>

KaiGai Kohei <kaigai@kaigai.gr.jp> writes:

BTW, currently, the default ACL of largeobject allows anything for owner
and nothing for world. Do you have any comment for the default behavior?

Mph. I think the backlash will be too great. You have to leave the
default behavior the same as it is now, ie, world access.

regards, tom lane

KaiGai Kohei wrote:

BTW, currently, the default ACL of largeobject allows anything for owner
and nothing for world. Do you have any comment for the default behavior?

Backwards compatibility would say that the world should be able to at
least read the object.

--
Alvaro Herrera http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

The attached patch is the revised version of largeobject access controls.

It reverts pg_largeobject system catalog, and adds new pg_largeobject_meta
system catalog to store the owner identifier and its ACLs.

The definition of pg_largeobject_meta:

#define LargeObjectMetaRelationId 2336

CATALOG(pg_largeobject_meta,2336)
{
Oid lomowner; /* OID of the largeobject owner */
aclitem lomacl[1]; /* access permissions */
} FormData_pg_largeobject_meta;

The pg_largeobject system catalog is still used to store data chunks of
largeobjects, and its pg_largeobject.loid is associated with OID of the
pg_largeobject_meta system catalog.

* It also supports case handling in DROP ROLE and REASSIGN/DROP OWNED
using existing dependency mechanism.
* A new "ALTER LARGE OBJECT <oid> OWNER TO <user>" statement was added.
* Permission checks on creation of largeobjects are dropped. It implicitly
allows everyone to create a new largeobject.
(CREATE USER LARGEOBJECT/NOLARGEOBJECT is also dropped.)
* The default ACL allows public to read/write new largeobjects as long as
owner does not revoke permissions. (MEMO: It might be configurable
using GUC whether the default allows public to read/write, or not.)

[Performance measurement]
We measured the time to execute \lo_import with two large files (the one
is well compressible, the other is not so) and \lo_export them.
In the result, it seems to me there are no significant regression here.

* Environment
CPU: Pentium4 3.20GHz
Mem: 512MB
Kernel: 2.6.30-6.fc12.i586
PostgreSQL configuration: all parameters are in default.

* Base PostgreSQL
- Import/Export an uncompressible file
[kaigai@saba ~]$ time -p psql postgres -c '\lo_import 512MB_Rnd'
lo_import 16386
real 132.33
user 1.01
sys 5.06
[kaigai@saba ~]$ time -p psql postgres -c '\lo_export 16386 /dev/null'
lo_export
real 77.57
user 0.79
sys 3.76

- Import/Export well compressible file
[kaigai@saba ~]$ time -p psql postgres -c '\lo_import 512MB_Zero'
lo_import 16387
real 45.84
user 0.91
sys 5.38
[kaigai@saba ~]$ time -p psql postgres -c '\lo_export 16387 /dev/null'
lo_export
real 13.51
user 0.62
sys 2.98

* with Largeobject access control patch
- Import/Export an uncompressible file
[kaigai@saba ~]$ time -p psql postgres -c '\lo_import 512MB_Rnd'
lo_import 16384
real 132.49
user 1.13
sys 5.10
[kaigai@saba ~]$ time -p psql postgres -c '\lo_export 16384 /dev/null'
lo_export
real 76.14
user 0.81
sys 3.63

- Import/Export well compressible file
[kaigai@saba ~]$ time -p psql postgres -c '\lo_import 512MB_Zero'
lo_import 16385
real 44.21
user 0.91
sys 5.51
[kaigai@saba ~]$ time -p psql postgres -c '\lo_export 16385 /dev/null'
lo_export
real 14.27
user 0.66
sys 3.11

Thanks,

[kaigai@saba blob]$ diffstat sepgsql-02-blob-8.5devel-r2272.patch.gz
doc/src/sgml/ref/allfiles.sgml | 1
doc/src/sgml/ref/alter_large_object.sgml | 75 ++++++++
doc/src/sgml/ref/grant.sgml | 8
doc/src/sgml/ref/revoke.sgml | 6
doc/src/sgml/reference.sgml | 1
src/backend/catalog/Makefile | 6
src/backend/catalog/aclchk.c | 247 ++++++++++++++++++++++++++
src/backend/catalog/dependency.c | 14 +
src/backend/catalog/pg_largeobject.c | 270 +++++++++!!!!!!!!!!!!!!!!!!!
src/backend/catalog/pg_shdepend.c | 8
src/backend/commands/alter.c | 5
src/backend/commands/comment.c | 14 !
src/backend/commands/tablecmds.c | 1
src/backend/libpq/be-fsstubs.c | 49 ++--
src/backend/parser/gram.y | 20 ++
src/backend/storage/large_object/inv_api.c | 115 +++-----!!!!
src/backend/tcop/utility.c | 3
src/backend/utils/adt/acl.c | 5
src/backend/utils/cache/syscache.c | 13 +
src/include/catalog/dependency.h | 1
src/include/catalog/indexing.h | 3
src/include/catalog/pg_largeobject_meta.h | 66 +++++++
src/include/nodes/parsenodes.h | 1
src/include/utils/acl.h | 6
src/include/utils/syscache.h | 1
src/test/regress/expected/privileges.out | 162 +++++++++++++++++
src/test/regress/expected/sanity_check.out | 3
src/test/regress/sql/privileges.sql | 65 ++++++
28 files changed, 859 insertions(+), 73 deletions(-), 237 modifications(!)

--
OSS Platform Development Division, NEC
KaiGai Kohei <kaigai@ak.jp.nec.com>

Tom Lane wrote:

KaiGai Kohei <kaigai@kaigai.gr.jp> writes:

BTW, currently, the default ACL of largeobject allows anything for owner
and nothing for world. Do you have any comment for the default behavior?

Mph. I think the backlash will be too great. You have to leave the
default behavior the same as it is now, ie, world access.

BTW as a default it is pretty bad. Should we have a GUC var to set the
default LO permissions?

--
Alvaro Herrera http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

Alvaro Herrera wrote:

Tom Lane wrote:

KaiGai Kohei <kaigai@kaigai.gr.jp> writes:

BTW, currently, the default ACL of largeobject allows anything for owner
and nothing for world. Do you have any comment for the default behavior?

Mph. I think the backlash will be too great. You have to leave the
default behavior the same as it is now, ie, world access.

BTW as a default it is pretty bad. Should we have a GUC var to set the
default LO permissions?

It seems to me a reasonable idea in direction.
However, it might be better to add a GUC variable to turn on/off LO
permission feature, not only default permissions.
It allows us to control whether the privilege mechanism should perform
in backward compatible, or not.
--
OSS Platform Development Division, NEC
KaiGai Kohei <kaigai@ak.jp.nec.com>

KaiGai Kohei wrote:

Alvaro Herrera wrote:

Tom Lane wrote:

KaiGai Kohei <kaigai@kaigai.gr.jp> writes:

BTW, currently, the default ACL of largeobject allows anything for owner
and nothing for world. Do you have any comment for the default behavior?

Mph. I think the backlash will be too great. You have to leave the
default behavior the same as it is now, ie, world access.

BTW as a default it is pretty bad. Should we have a GUC var to set the
default LO permissions?

It seems to me a reasonable idea in direction.
However, it might be better to add a GUC variable to turn on/off LO
permission feature, not only default permissions.
It allows us to control whether the privilege mechanism should perform
in backward compatible, or not.

Now we have two options:

1. A GUC variable to set the default largeobject permissions.

SET largeobject_default_acl = [ ro | rw | none ]
- ro : read-only
- rw : read-writable
- none : nothing

It can control the default acl which is applied when NULL is set on
the pg_largeobject_meta.lomacl. However, lo_unlink() checks ownership
on the largeobject, so it is not enough compatible with v8.4.x or prior.

2. A GUC veriable to turn on/off largeobject permissions.

SET largeobject_compat_dac = [ on | off ]

When the variable is turned on, largeobject dac permission check is
not applied as the v8.4.x or prior version did. So, the variable is
named "compat" which means compatible behavior.
It also does not check ownership on lo_unlink().

My preference is the second approach.

What's your opinion?

Thanks,
--
OSS Platform Development Division, NEC
KaiGai Kohei <kaigai@ak.jp.nec.com>

2009/9/3 KaiGai Kohei <kaigai@ak.jp.nec.com>:

KaiGai Kohei wrote:

Alvaro Herrera wrote:

Tom Lane wrote:

KaiGai Kohei <kaigai@kaigai.gr.jp> writes:

BTW, currently, the default ACL of largeobject allows anything for owner
and nothing for world. Do you have any comment for the default behavior?

Mph.  I think the backlash will be too great.  You have to leave the
default behavior the same as it is now, ie, world access.

BTW as a default it is pretty bad.  Should we have a GUC var to set the
default LO permissions?

It seems to me a reasonable idea in direction.
However, it might be better to add a GUC variable to turn on/off LO
permission feature, not only default permissions.
It allows us to control whether the privilege mechanism should perform
in backward compatible, or not.

Now we have two options:

1. A GUC variable to set the default largeobject permissions.

 SET largeobject_default_acl = [ ro | rw | none ]
   - ro   : read-only
   - rw   : read-writable
   - none : nothing

 It can control the default acl which is applied when NULL is set on
 the pg_largeobject_meta.lomacl. However, lo_unlink() checks ownership
 on the largeobject, so it is not enough compatible with v8.4.x or prior.

2. A GUC veriable to turn on/off largeobject permissions.

 SET largeobject_compat_dac = [ on | off ]

 When the variable is turned on, largeobject dac permission check is
 not applied as the v8.4.x or prior version did. So, the variable is
 named "compat" which means compatible behavior.
 It also does not check ownership on lo_unlink().

My preference is the second approach.

What's your opinion?

I prefer the first. There's little harm in letting users set the
default permissions for themselves, but a GUC that controls
system-wide behavior will have to be something only superusers can
money with, and that seems like it will reduce usability.

Why couldn't lo_unlink() just check write privilege?

...Robert

Robert Haas wrote:

2009/9/3 KaiGai Kohei <kaigai@ak.jp.nec.com>:

KaiGai Kohei wrote:

Alvaro Herrera wrote:

Tom Lane wrote:

KaiGai Kohei <kaigai@kaigai.gr.jp> writes:

BTW, currently, the default ACL of largeobject allows anything for owner
and nothing for world. Do you have any comment for the default behavior?

Mph. I think the backlash will be too great. You have to leave the
default behavior the same as it is now, ie, world access.

BTW as a default it is pretty bad. Should we have a GUC var to set the
default LO permissions?

It seems to me a reasonable idea in direction.
However, it might be better to add a GUC variable to turn on/off LO
permission feature, not only default permissions.
It allows us to control whether the privilege mechanism should perform
in backward compatible, or not.

Now we have two options:

1. A GUC variable to set the default largeobject permissions.

SET largeobject_default_acl = [ ro | rw | none ]
- ro : read-only
- rw : read-writable
- none : nothing

It can control the default acl which is applied when NULL is set on
the pg_largeobject_meta.lomacl. However, lo_unlink() checks ownership
on the largeobject, so it is not enough compatible with v8.4.x or prior.

2. A GUC veriable to turn on/off largeobject permissions.

SET largeobject_compat_dac = [ on | off ]

When the variable is turned on, largeobject dac permission check is
not applied as the v8.4.x or prior version did. So, the variable is
named "compat" which means compatible behavior.
It also does not check ownership on lo_unlink().

My preference is the second approach.

What's your opinion?

I prefer the first. There's little harm in letting users set the
default permissions for themselves, but a GUC that controls
system-wide behavior will have to be something only superusers can
money with, and that seems like it will reduce usability.

I don't intend to allow session users to set up their default acl.
Both operation should be always system-wide.

If a normal user can change the default acl, it is also equivalent
he can grant all permissions to public on all the largeobject with
its acl being NULL.
Note that PostgreSQL does not set up a certain ACLs on its creation
time, so NULL is assigned. The default ACL means an alternarive set
of permissions, when it is NULL.

Why couldn't lo_unlink() just check write privilege?

Because it is inconsistent behavior.
PostgreSQL checks its ownership on dropping a certain database objects,
such as tabls, procedures and so on.
It seems to me quite strange, if only largeobject checks writer permission
to drop itself.

Thanks,
--
OSS Platform Development Division, NEC
KaiGai Kohei <kaigai@ak.jp.nec.com>

The attached patch is an update of largeobject access controls.

It adds a new guc variable to turn on/off compatible behavior in
largeobejct access controls.

largeobject_compat_dac = [on | off] (default: off)

If the variable is turned on, all the new access control features
on largeobjects are bypassed, as if v8.4.x or prior did.
(Note that lo_import()/lo_export() checks client's superuser privilege
independent from the guc setting, because it has been checked prior to
the v8.4.x.)

--------------------------------
[kaigai@saba blob]$ psql postgres
psql (8.5devel)
Type "help" for help.
postgres=# SET SESSION AUTHORIZATION ymj;
SET
postgres=> SELECT loread(lo_open(1234, x'40000'::int), 100);
ERROR: permission denied for largeobject 1234

postgres=> \c -
psql (8.5devel)
You are now connected to database "postgres".
postgres=# SET largeobject_compat_dac TO on; -- set compatible largeobject
SET
postgres=# SET SESSION AUTHORIZATION ymj;
SET
postgres=> SELECT loread(lo_open(1234, x'40000'::int), 100);
loread
--------
\x
(1 row)

KaiGai Kohei wrote:

Robert Haas wrote:

2009/9/3 KaiGai Kohei <kaigai@ak.jp.nec.com>:

KaiGai Kohei wrote:

Alvaro Herrera wrote:

Tom Lane wrote:

KaiGai Kohei <kaigai@kaigai.gr.jp> writes:

BTW, currently, the default ACL of largeobject allows anything for owner
and nothing for world. Do you have any comment for the default behavior?

Mph. I think the backlash will be too great. You have to leave the
default behavior the same as it is now, ie, world access.

BTW as a default it is pretty bad. Should we have a GUC var to set the
default LO permissions?

It seems to me a reasonable idea in direction.
However, it might be better to add a GUC variable to turn on/off LO
permission feature, not only default permissions.
It allows us to control whether the privilege mechanism should perform
in backward compatible, or not.

Now we have two options:

1. A GUC variable to set the default largeobject permissions.

SET largeobject_default_acl = [ ro | rw | none ]
- ro : read-only
- rw : read-writable
- none : nothing

It can control the default acl which is applied when NULL is set on
the pg_largeobject_meta.lomacl. However, lo_unlink() checks ownership
on the largeobject, so it is not enough compatible with v8.4.x or prior.

2. A GUC veriable to turn on/off largeobject permissions.

SET largeobject_compat_dac = [ on | off ]

When the variable is turned on, largeobject dac permission check is
not applied as the v8.4.x or prior version did. So, the variable is
named "compat" which means compatible behavior.
It also does not check ownership on lo_unlink().

My preference is the second approach.

What's your opinion?

I prefer the first. There's little harm in letting users set the
default permissions for themselves, but a GUC that controls
system-wide behavior will have to be something only superusers can
money with, and that seems like it will reduce usability.

I don't intend to allow session users to set up their default acl.
Both operation should be always system-wide.

If a normal user can change the default acl, it is also equivalent
he can grant all permissions to public on all the largeobject with
its acl being NULL.
Note that PostgreSQL does not set up a certain ACLs on its creation
time, so NULL is assigned. The default ACL means an alternarive set
of permissions, when it is NULL.

Why couldn't lo_unlink() just check write privilege?

Because it is inconsistent behavior.
PostgreSQL checks its ownership on dropping a certain database objects,
such as tabls, procedures and so on.
It seems to me quite strange, if only largeobject checks writer permission
to drop itself.

Thanks,

--
OSS Platform Development Division, NEC
KaiGai Kohei <kaigai@ak.jp.nec.com>

Jamie,

* Robert Haas (robertmhaas@gmail.com) wrote:

Jaime Casanova <jcasanov@systemguards.com.ec>
- Largeobject access controls

How is the review for this coming? Do you have any thoughts regarding
the new GUC?

Thanks,

Stephen

On Fri, Sep 18, 2009 at 8:29 PM, Stephen Frost <sfrost@snowman.net> wrote:

Jamie,

How is the review for this coming?  Do you have any thoughts regarding
the new GUC?

Hi, sorry... these have been hard days... i'm just starting reviewing

--
Atentamente,
Jaime Casanova
Soporte y capacitación de PostgreSQL
Asesoría y desarrollo de sistemas
Guayaquil - Ecuador
Cel. +59387171157

2009/9/6 KaiGai Kohei <kaigai@ak.jp.nec.com>:

The attached patch is an update of largeobject access controls.

it applies fine (just 3 succeded hunks), compiles and passes
regression tests...

ALTER LARGE OBJECT is working, but now that we can change the owner of
a LO we should be able to see who the actual owner is... i mean we
should add an owner column in \dl for psql (maybe \dl+) and maybe an
lo_owner() function.

the GRANTs (and REVOKEs) work just fine too...
Another question is what we want that only the LO owner can delete it
(via lo_unlink)?

another one, is it possible for us to have a CREATE LARGE OBJECT statement?
the reason for this is:
1) it is a little ugly to use the OID in ALTER/GRANT/REVOKE
statements, in a create statement we can assign a name to the LO
2) it could be more consistent with other ALTER/GRANT/REVOKE that acts
over objects created with CREATE while large objects are created via
lo_import() which makes obvious that are just records in meta-data
table (hope this is understandable)

It adds a new guc variable to turn on/off compatible behavior in
largeobejct access controls.

 largeobject_compat_dac = [on | off] (default: off)

If the variable is turned on, all the new access control features
on largeobjects are bypassed, as if v8.4.x or prior did.

the GUC works as intended
but i don't like the name, it is not very meaningful for those of us
that doesn't know what DAC or MAC are...
another point, you really have to put the GUC in the postgresql.conf
file if you hope people know about it ;)
it is not documented either

About the code...
- I don't like the name pg_largeobject_meta why not pg_largeobject_acl
(put here any other name you like)? or there was a reason for that
choose?

--
Atentamente,
Jaime Casanova
Soporte y capacitación de PostgreSQL
Asesoría y desarrollo de sistemas
Guayaquil - Ecuador
Cel. +59387171157

Jaime, Thanks for your reviewing.

Jaime Casanova wrote:

2009/9/6 KaiGai Kohei <kaigai@ak.jp.nec.com>:

The attached patch is an update of largeobject access controls.

it applies fine (just 3 succeded hunks), compiles and passes
regression tests...

ALTER LARGE OBJECT is working, but now that we can change the owner of
a LO we should be able to see who the actual owner is... i mean we
should add an owner column in \dl for psql (maybe \dl+) and maybe an
lo_owner() function.

I would like to buy your idea at the revised patch.

the GRANTs (and REVOKEs) work just fine too...
Another question is what we want that only the LO owner can delete it
(via lo_unlink)?

It is the standard manner in PostgreSQL. The native database privilege mechanism
checks ownership of the database object when a user tries to drop the object.
I don't think we have good reason that only largeobejct has its own principle.

Please note that it is different from ACL_DELETE permission, because it does not
control privilege to drop the table itself. It allows a certain user to delete
tuples within the table.

another one, is it possible for us to have a CREATE LARGE OBJECT statement?
the reason for this is:
1) it is a little ugly to use the OID in ALTER/GRANT/REVOKE
statements, in a create statement we can assign a name to the LO
2) it could be more consistent with other ALTER/GRANT/REVOKE that acts
over objects created with CREATE while large objects are created via
lo_import() which makes obvious that are just records in meta-data
table (hope this is understandable)

It is not difficult to implement the named-largeobejct.

However, the matter is whether it is really wanted feature to decorate
a largeobject, or not. At least, access controls on largeobjects is a
todo item, but symbolic identifier on largeobject is not the one.

BTW, we already have COMMENT ON LARGE OBJECT <loid> statement now.
How should it be handled, if a largeobject has its symbolic identifier?

It adds a new guc variable to turn on/off compatible behavior in
largeobejct access controls.

largeobject_compat_dac = [on | off] (default: off)

If the variable is turned on, all the new access control features
on largeobjects are bypassed, as if v8.4.x or prior did.

the GUC works as intended
but i don't like the name, it is not very meaningful for those of us
that doesn't know what DAC or MAC are...

Do you think the "largeobject_compat_acl" is a meaningful name, instead?

another point, you really have to put the GUC in the postgresql.conf
file if you hope people know about it ;)
it is not documented either

I'll add a description about the GUC at the document.
Is it also necessary to add postgresql.conf.sample??

About the code...
- I don't like the name pg_largeobject_meta why not pg_largeobject_acl
(put here any other name you like)? or there was a reason for that
choose?

My preference is a pair of pg_largeobject and pg_largeobject_data (which
has an identical structure to the current pg_largeobject).

However, it seems to me the pg_largeobject_acl is an incorrect name,
because it also contains the owner identifier which is a part of metadata,
but not an acl.

Thanks,
--
KaiGai Kohei <kaigai@kaigai.gr.jp>