Re: [PATCH] Largeobject access controls

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. Only owner can unlink the largeobject which is owned by.
It also add a new attribute on the database role to control whether he
can create a new largeobject, or not. Because largeobject is not stored
within a certain namespace, we cannot control its creation using CREATE
permission.

The CREATE USER/ROLE statement got a new option: LARGEOBJECT/NOLARGEOBJECT.
It enables to controls whether the user can create a largeobject, or not.
The default is LARGEOBJECT which means user can create them.
This attribute is stored within pg_authid.rollargeobject defined as bool.

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.
As I noted before, there are several difficulties to implement partially
writable varlena type, so it uses the its toast relation just as a storage
to store its data chunks.

The GRANT/REVOKE statement also support largeobject, as follows:

GRANT SELECT ON LARGE OBJECT 1234 TO kaigai;

It follows the matter when COMMENT ON statement specifies a large object.

Thanks,

======== (Example) ================================
postgres=# CREATE USER dog; -- user can create largeobjects in default
CREATE ROLE
postgres=# CREATE USER cat NOLARGEOBJECT;
CREATE ROLE
postgres=# \c - dog
psql (8.5devel)
You are now connected to database "postgres" as user "dog".
postgres=> SELECT lo_create(123);
lo_create
-----------
123
(1 row)

postgres=> SELECT lo_create(100);
lo_create
-----------
100
(1 row)

postgres=> GRANT SELECT ON LARGE OBJECT 123 TO cat;
GRANT
postgres=> \c - cat
psql (8.5devel)
You are now connected to database "postgres" as user "cat".
postgres=> SELECT lo_create(456);
ERROR: permission denied to create largeobject
postgres=> SELECT loread(lo_open(123, x'40000'::int), 100);
loread
--------
\x
(1 row)

postgres=> SELECT lowrite(lo_open(123, x'20000'::int), 'abcdefg');
ERROR: permission denied for largeobject 123
postgres=> SELECT lo_unlink(123);
ERROR: must be owner of largeobject 123
===================================================

[kaigai(at)saba ~]$ diffstat sepgsql-02-blob-8.5devel-r2264.patch.gz
doc/src/sgml/ref/create_role.sgml | 13 +
doc/src/sgml/ref/create_user.sgml | 1
doc/src/sgml/ref/grant.sgml | 8
doc/src/sgml/ref/revoke.sgml | 6
src/backend/catalog/aclchk.c | 246 ++++++++++++++++++++
src/backend/catalog/dependency.c | 14 +
src/backend/catalog/pg_largeobject.c | 139 +!!!!!!!!!!
src/backend/catalog/pg_shdepend.c | 4
src/backend/commands/comment.c | 10
src/backend/commands/tablecmds.c | 1
src/backend/commands/user.c | 32 ++
src/backend/libpq/be-fsstubs.c | 141 ++++++++++-
src/backend/parser/gram.y | 26 +!
src/backend/storage/large_object/inv_api.c | 344 ++++-------!!!!!!!!!!!!!!!!
src/backend/utils/adt/acl.c | 4
src/backend/utils/cache/syscache.c | 13 +
src/include/catalog/dependency.h | 1
src/include/catalog/indexing.h | 4
src/include/catalog/pg_authid.h | 14 !
src/include/catalog/pg_largeobject.h | 17 !
src/include/catalog/toasting.h | 10
src/include/nodes/parsenodes.h | 1
src/include/parser/kwlist.h | 2
src/include/utils/acl.h | 6
src/include/utils/syscache.h | 1
src/test/regress/expected/privileges.out | 202 +++++++++++++++++
src/test/regress/input/largeobject.source | 7
src/test/regress/output/largeobject.source | 10
src/test/regress/sql/privileges.sql | 75 ++++++
29 files changed, 857 insertions(+), 107 deletions(-), 388 modifications(!)

--
OSS Platform Development Division, NEC
KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>

KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)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(at)ak(dot)jp(dot)nec(dot)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(at)ak(dot)jp(dot)nec(dot)com>

KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)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(at)ak(dot)jp(dot)nec(dot)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(at)kaigai(dot)gr(dot)jp>

KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)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(at)kaigai(dot)gr(dot)jp>

KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)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(at)saba ~]$ time -p psql postgres -c '\lo_import 512MB_Rnd'
lo_import 16386
real 132.33
user 1.01
sys 5.06
[kaigai(at)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(at)saba ~]$ time -p psql postgres -c '\lo_import 512MB_Zero'
lo_import 16387
real 45.84
user 0.91
sys 5.38
[kaigai(at)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(at)saba ~]$ time -p psql postgres -c '\lo_import 512MB_Rnd'
lo_import 16384
real 132.49
user 1.13
sys 5.10
[kaigai(at)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(at)saba ~]$ time -p psql postgres -c '\lo_import 512MB_Zero'
lo_import 16385
real 44.21
user 0.91
sys 5.51
[kaigai(at)saba ~]$ time -p psql postgres -c '\lo_export 16385 /dev/null'
lo_export
real 14.27
user 0.66
sys 3.11

Thanks,

[kaigai(at)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(at)ak(dot)jp(dot)nec(dot)com>

Tom Lane wrote:
> KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)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(at)kaigai(dot)gr(dot)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(at)ak(dot)jp(dot)nec(dot)com>

KaiGai Kohei wrote:
> Alvaro Herrera wrote:
>> Tom Lane wrote:
>>> KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)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(at)ak(dot)jp(dot)nec(dot)com>

2009/9/3 KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>:
> KaiGai Kohei wrote:
>> Alvaro Herrera wrote:
>>> Tom Lane wrote:
>>>> KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)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(at)ak(dot)jp(dot)nec(dot)com>:
>> KaiGai Kohei wrote:
>>> Alvaro Herrera wrote:
>>>> Tom Lane wrote:
>>>>> KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)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(at)ak(dot)jp(dot)nec(dot)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(at)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(at)ak(dot)jp(dot)nec(dot)com>:
>>> KaiGai Kohei wrote:
>>>> Alvaro Herrera wrote:
>>>>> Tom Lane wrote:
>>>>>> KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)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(at)ak(dot)jp(dot)nec(dot)com>

Jamie,

* Robert Haas (robertmhaas(at)gmail(dot)com) wrote:
> Jaime Casanova <jcasanov(at)systemguards(dot)com(dot)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(at)snowman(dot)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(at)ak(dot)jp(dot)nec(dot)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(at)ak(dot)jp(dot)nec(dot)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(at)kaigai(dot)gr(dot)jp>

On Tue, Sep 22, 2009 at 7:23 AM, KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
>
>> 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.

yeah! i don't think this will be implemented soon nor that you had to
do it... just want to mention it for later discussion because it seems
like natural evolution of the feature

>
>>> 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?
>

maybe something like "largeobject_security_controls"?

>> 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??
>

i think so, it's a compatibility issue so it must be easily findable
(don't know if that word actually exists, though :)

>> 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.
>

have anyone better ideas about the name? if not, then go with
pg_largeobject_meta

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

On Tue, Sep 22, 2009 at 1:29 PM, Jaime Casanova
<jcasanov(at)systemguards(dot)com(dot)ec> wrote:
> have anyone better ideas about the name? if not, then go with
> pg_largeobject_meta

I don't think there's anything wrong with calling it metadata. That
seems to leave the door open to future things we might want to do,
without restricting it to security-related settings or whatever. But
I don't see any reason to abbreviate it either - I'd go with
pg_largeobject_metadata rather than just pg_largeobject_meta.

...Robert

On Tue, Sep 22, 2009 at 12:53 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Tue, Sep 22, 2009 at 1:29 PM, Jaime Casanova
> <jcasanov(at)systemguards(dot)com(dot)ec> wrote:
>> have anyone better ideas about the name? if not, then go with
>> pg_largeobject_meta
>
> I don't think there's anything wrong with calling it metadata.  That
> seems to leave the door open to future things we might want to do,
> without restricting it to security-related settings or whatever.  But
> I don't see any reason to abbreviate it either - I'd go with
> pg_largeobject_metadata rather than just pg_largeobject_meta.
>

ok! metadata is self explanatory meta is not...
at least not for someone that his mother tongue is spanish and think
of "meta" like in "goal" ;)

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

Jaime,

KaiGai Kohei wrote:
| > 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.

Now we don't have xxx_owner() function for other database objects,
such as tables, procedures and so on.
I agree to enhance \dl command for psql, however, it seems to me
that using SELECT from system catalogs are normal manner in pgsql,
instead of lo_owner() function.

Jaime Casanova wrote:
>> Do you think the "largeobject_compat_acl" is a meaningful name, instead?
>
> maybe something like "largeobject_security_controls"?

It is important to contain a term of "compat" which means compatible,
because this GUC does not disables all the security checks.
The v8.4.x checks superuser privilege on using lo_import()/lo_export().
It is also checked in this patch, even if the GUC is turned on.

The purpose of the GUC is to provide compatible behavior, not to provide
a stuff to turn on/off all the security features in largeobjects.

So, I still prefer the "largeobject_compat_acl".

Now, I'm revising the patch as follows:
- pg_largeobject_meta is renamed to pg_largeobject_metadata
- The GUC of largeobject_compat_dac is renamed to largeobject_compat_acl
- psql supports \dl to show owner of the largeobject
- add documentation for the GUC, and add it to the postgresql.conf.sample

Any comments?

Thanks,
--
OSS Platform Development Division, NEC
KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>

2009/9/23 KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>:
> Now, I'm revising the patch as follows:
> - pg_largeobject_meta is renamed to pg_largeobject_metadata
> - The GUC of largeobject_compat_dac is renamed to largeobject_compat_acl
> - psql supports \dl to show owner of the largeobject
> - add documentation for the GUC, and add it to the postgresql.conf.sample

I still don't like the idea of having a GUC that turns off a
substantial part of the security system.

Am I the only one?

...Robert

Robert Haas wrote:
> 2009/9/23 KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>:
>> Now, I'm revising the patch as follows:
>> - pg_largeobject_meta is renamed to pg_largeobject_metadata
>> - The GUC of largeobject_compat_dac is renamed to largeobject_compat_acl
>> - psql supports \dl to show owner of the largeobject
>> - add documentation for the GUC, and add it to the postgresql.conf.sample
>
> I still don't like the idea of having a GUC that turns off a
> substantial part of the security system.
>
> Am I the only one?

I also think you are right from the viewpoint of the security.
Smaller number of pitfall on configuration is basically better.

However, we already released v8.4.x or prior versions without ACL
checks on largeobjects, so it is necessary to pay attentions for
existing SQLs which expect no ACL checks on largeobject accesses.
The purpose of the GUC is to provide users compatible bahaviors on
largeobjects.

BTW, here is one idea. When the largeobject_compat_acl is turned on,
it allows to bypass ACL checks, but it generates warning message for
violated accesses. User can notice his SQL should be fixed at the
v8.5.x or later.
(It is similar to the permissive-mode in SELinux.)

Thanks,
--
OSS Platform Development Division, NEC
KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>

> Now, I'm revising the patch as follows:
> - pg_largeobject_meta is renamed to pg_largeobject_metadata
> - The GUC of largeobject_compat_dac is renamed to largeobject_compat_acl
> - psql supports \dl to show owner of the largeobject
> - add documentation for the GUC, and add it to the postgresql.conf.sample

Here is the revised patch.

The \dl command in psql is enhanced as follows:

postgres=# \dl
Large objects
ID | Owner | Description
-------+--------+---------------------
16448 | kaigai |
16449 | kaigai | test large object 1
16450 | ymj |
16451 | ymj |
16452 | ymj | test large object 2
16453 | tak |
16454 | tak |
(7 rows)

The functionality of largeobject_compat_acl (which was named as
largeobject_compat_dac at the previous patch) is changed a bit.

Its default is 'off'. When it is turned on, access control features
on largeobjects performs with the compatible mode. It also checks
access permissions on largeobjects, but its results are ignored with
notification messages to inform access violation.

It means the v8.5.x provides access control on largeobjects in default,
although it also provides compatible mode. However, it should be informed
to users their SQL to be revised.

Example)
postgres=# SET SESSION AUTHORIZATION ymj;
SET
postgres=> SELECT loread(lo_open(16453, x'40000'::int), 20);
ERROR: permission denied for largeobject 16453

postgres=# SET largeobject_compat_acl = on; <---- enables compatible mode
SET (Only superuser can set it)
postgres=# SET SESSION AUTHORIZATION ymj;
SET
postgres=> SELECT loread(lo_open(16453, x'40000'::int), 20);
NOTICE: permission denied for largeobject 16453 <---- dose not prevent it
loread
--------------------------------------------
\x255044462d312e350d0a25b5b5b5b50d0a312030
(1 row)

Thanks,

$ diffstat sepgsql-02-blob-8.5devel-r2327.patch.gz
doc/src/sgml/config.sgml | 25 +
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 | 249 ++++++++++++++++++
src/backend/catalog/dependency.c | 14 +
src/backend/catalog/pg_largeobject.c | 354 ++++++-!!!!!!!!!!!!!!!!!
src/backend/catalog/pg_shdepend.c | 8
src/backend/commands/alter.c | 5
src/backend/commands/comment.c | 11
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/backend/utils/misc/guc.c | 10
src/backend/utils/misc/postgresql.conf.sample | 1
src/bin/psql/large_obj.c | 10
src/include/catalog/dependency.h | 1
src/include/catalog/indexing.h | 3
src/include/catalog/pg_largeobject_metadata.h | 67 ++++
src/include/nodes/parsenodes.h | 1
src/include/utils/acl.h | 6
src/include/utils/syscache.h | 1
src/test/regress/expected/privileges.out | 204 ++++++++++++++
src/test/regress/expected/sanity_check.out | 3
src/test/regress/sql/privileges.sql | 83 ++++++
32 files changed, 966 insertions(+), 73 deletions(-), 320 modifications(!)
--
OSS Platform Development Division, NEC
KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>

2009/9/23 KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>:
> Jaime,
>
> KaiGai Kohei wrote:
> | > 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.
>
> Now we don't have xxx_owner() function for other database objects,
> such as tables, procedures and so on.

good point, but we have has_xxxxxx_privileges() family of functions
but i think we can add them later if needed...

>
> Jaime Casanova wrote:
>>> Do you think the "largeobject_compat_acl" is a meaningful name, instead?
>>
>> maybe something like "largeobject_security_controls"?
>
> It is important to contain a term of "compat" which means compatible,
> because this GUC does not disables all the security checks.
> The v8.4.x checks superuser privilege on using lo_import()/lo_export().
> It is also checked in this patch, even if the GUC is turned on.
>
> The purpose of the GUC is to provide compatible behavior, not to provide
> a stuff to turn on/off all the security features in largeobjects.
>

that's why the section in the postgresql.conf is called
"VERSION/PLATFORM COMPATIBILITY" and the subsection "Previous
PostgreSQL Versions" we have other compatibilty GUC and no ones of
those has the term "compat" in it...

> So, I still prefer the "largeobject_compat_acl".
>

maybe "enhanced_largeobjects_checks" or "enhanced_lo_checks"
or make the GUC an enum and name it "largeobject_control_checks" with
posible values "basic" and "enhanced"

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

2009/9/24 KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>:
>
> Example)
>  postgres=# SET SESSION AUTHORIZATION ymj;
>  SET
>  postgres=> SELECT loread(lo_open(16453, x'40000'::int), 20);
>  ERROR:  permission denied for largeobject 16453
>
>  postgres=# SET largeobject_compat_acl = on;           <---- enables compatible mode
>  SET                                                         (Only superuser can set it)
>  postgres=# SET SESSION AUTHORIZATION ymj;
>  SET
>  postgres=> SELECT loread(lo_open(16453, x'40000'::int), 20);
>  NOTICE:  permission denied for largeobject 16453      <---- dose not prevent it

i'm not really sure the warnings are worth the trouble but if you want
to do it then the NOTICE version should use another message... i'm not
comfortable with a "permission denied" that is simply ignored...

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

Jaime Casanova wrote:
> 2009/9/23 KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>:
>> Jaime,
>>
>> KaiGai Kohei wrote:
>> | > 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.
>>
>> Now we don't have xxx_owner() function for other database objects,
>> such as tables, procedures and so on.
>
> good point, but we have has_xxxxxx_privileges() family of functions
> but i think we can add them later if needed...

Yes, the has_xxxxxx_privileges() family should be added later or soon.
Anyway, what I wanted to say is we have no special functions to show
owner of the database objects.

>> Jaime Casanova wrote:
>>>> Do you think the "largeobject_compat_acl" is a meaningful name, instead?
>>> maybe something like "largeobject_security_controls"?
>> It is important to contain a term of "compat" which means compatible,
>> because this GUC does not disables all the security checks.
>> The v8.4.x checks superuser privilege on using lo_import()/lo_export().
>> It is also checked in this patch, even if the GUC is turned on.
>>
>> The purpose of the GUC is to provide compatible behavior, not to provide
>> a stuff to turn on/off all the security features in largeobjects.
>>
>
> that's why the section in the postgresql.conf is called
> "VERSION/PLATFORM COMPATIBILITY" and the subsection "Previous
> PostgreSQL Versions" we have other compatibilty GUC and no ones of
> those has the term "compat" in it...

Indeed, I put the "largeobject_compat_acl" in the compatibility section,
but no other GUCs have "compat" prefix/suffix. It seems to me fair enough.

>> So, I still prefer the "largeobject_compat_acl".
>
> maybe "enhanced_largeobjects_checks" or "enhanced_lo_checks"
> or make the GUC an enum and name it "largeobject_control_checks" with
> posible values "basic" and "enhanced"

But, isn't the "enhanced" tumid expression? It just applies native database
privilege mechanism on largeobjects, as if it does on other objects.

An other alternative is "largeobject_check_acl". What's your feeling?

Thanks,
--
OSS Platform Development Division, NEC
KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>

Jaime Casanova wrote:
> 2009/9/24 KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>:
>> Example)
>> postgres=# SET SESSION AUTHORIZATION ymj;
>> SET
>> postgres=> SELECT loread(lo_open(16453, x'40000'::int), 20);
>> ERROR: permission denied for largeobject 16453
>>
>> postgres=# SET largeobject_compat_acl = on; <---- enables compatible mode
>> SET (Only superuser can set it)
>> postgres=# SET SESSION AUTHORIZATION ymj;
>> SET
>> postgres=> SELECT loread(lo_open(16453, x'40000'::int), 20);
>> NOTICE: permission denied for largeobject 16453 <---- dose not prevent it
>
> i'm not really sure the warnings are worth the trouble but if you want
> to do it then the NOTICE version should use another message... i'm not
> comfortable with a "permission denied" that is simply ignored...

It is not a significant issue whether the compatible mode allows users
to bypass ACL checks with or without any notifications.

Which is the preferable one?

1. It always generates notifications whenever access violations.
2. It generates notifications at the first violation only.
3. It never generates notifications.

Thanks,
--
OSS Platform Development Division, NEC
KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>

The attached patch is revised from the previous revision at the following points:

- The "largeobject_compat_acl" is renamed to "largeobject_check_acl".
Its default is on, and turning it off means the largeobject stuff
performs in compatible mode for the v8.4.x or prior releases.
- Notification messages were eliminated at the compatible mode.
It always allows to bypass ACL checks without any warnings.

Thanks,

$ diffstat sepgsql-02-blob-8.5devel-r2328.patch.gz
doc/src/sgml/config.sgml | 28 ++
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 | 249 ++++++++++++++++++
src/backend/catalog/dependency.c | 14 +
src/backend/catalog/pg_largeobject.c | 357 +++++++!!!!!!!!!!!!!!!!!!
src/backend/catalog/pg_shdepend.c | 8
src/backend/commands/alter.c | 5
src/backend/commands/comment.c | 11
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/backend/utils/misc/guc.c | 10
src/backend/utils/misc/postgresql.conf.sample | 1
src/bin/psql/large_obj.c | 10
src/include/catalog/dependency.h | 1
src/include/catalog/indexing.h | 3
src/include/catalog/pg_largeobject_metadata.h | 67 ++++
src/include/nodes/parsenodes.h | 1
src/include/utils/acl.h | 6
src/include/utils/syscache.h | 1
src/test/regress/expected/privileges.out | 206 +++++++++++++++
src/test/regress/expected/sanity_check.out | 3
src/test/regress/sql/privileges.sql | 85 ++++++
32 files changed, 976 insertions(+), 77 deletions(-), 316 modifications(!)

KaiGai Kohei wrote:
> Jaime Casanova wrote:
>> 2009/9/23 KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>:
>>> Jaime,
>>>
>>> KaiGai Kohei wrote:
>>> | > 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.
>>>
>>> Now we don't have xxx_owner() function for other database objects,
>>> such as tables, procedures and so on.
>> good point, but we have has_xxxxxx_privileges() family of functions
>> but i think we can add them later if needed...
>
> Yes, the has_xxxxxx_privileges() family should be added later or soon.
> Anyway, what I wanted to say is we have no special functions to show
> owner of the database objects.
>
>>> Jaime Casanova wrote:
>>>>> Do you think the "largeobject_compat_acl" is a meaningful name, instead?
>>>> maybe something like "largeobject_security_controls"?
>>> It is important to contain a term of "compat" which means compatible,
>>> because this GUC does not disables all the security checks.
>>> The v8.4.x checks superuser privilege on using lo_import()/lo_export().
>>> It is also checked in this patch, even if the GUC is turned on.
>>>
>>> The purpose of the GUC is to provide compatible behavior, not to provide
>>> a stuff to turn on/off all the security features in largeobjects.
>>>
>> that's why the section in the postgresql.conf is called
>> "VERSION/PLATFORM COMPATIBILITY" and the subsection "Previous
>> PostgreSQL Versions" we have other compatibilty GUC and no ones of
>> those has the term "compat" in it...
>
> Indeed, I put the "largeobject_compat_acl" in the compatibility section,
> but no other GUCs have "compat" prefix/suffix. It seems to me fair enough.
>
>>> So, I still prefer the "largeobject_compat_acl".
>> maybe "enhanced_largeobjects_checks" or "enhanced_lo_checks"
>> or make the GUC an enum and name it "largeobject_control_checks" with
>> posible values "basic" and "enhanced"
>
> But, isn't the "enhanced" tumid expression? It just applies native database
> privilege mechanism on largeobjects, as if it does on other objects.
>
> An other alternative is "largeobject_check_acl". What's your feeling?
>
> Thanks,

--
OSS Platform Development Division, NEC
KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>

2009/9/24 KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>:
> The attached patch is revised from the previous revision at the following points:

Jaime,

Do you think this is Ready for Committer review at this point? If so,
please mark it that way; otherwise, what do you think are the
outstanding issues?

...Robert

2009/9/24 KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>:
> The attached patch is revised from the previous revision at the following points:
>
> - The "largeobject_compat_acl" is renamed to "largeobject_check_acl".
>  Its default is on, and turning it off means the largeobject stuff
>  performs in compatible mode for the v8.4.x or prior releases.
> - Notification messages were eliminated at the compatible mode.
>  It always allows to bypass ACL checks without any warnings.
>

a few minor points:

+ For example, the <literal>lo_import()</literal> and
+ <literal>lo_export</literal> need superuser privileges independent
+ from this setting, as if the prior version doing.

that should read "as prior versions were doing"?

and you're still using pg_largeobject_meta in some comments in
src/include/catalog/pg_largeobject_metadata.h

besides that it looks good to me...
i will mark the patch as "ready for committer"

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

Thanks for your comments.

Jaime Casanova wrote:
> 2009/9/24 KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>:
>> The attached patch is revised from the previous revision at the following points:
>>
>> - The "largeobject_compat_acl" is renamed to "largeobject_check_acl".
>> Its default is on, and turning it off means the largeobject stuff
>> performs in compatible mode for the v8.4.x or prior releases.
>> - Notification messages were eliminated at the compatible mode.
>> It always allows to bypass ACL checks without any warnings.
>>
>
> a few minor points:
>
> + For example, the <literal>lo_import()</literal> and
> + <literal>lo_export</literal> need superuser privileges independent
> + from this setting, as if the prior version doing.
>
> that should read "as prior versions were doing"?

Yes.
It seems to me same meanings, but it is unnatural for you, isn't it?

> and you're still using pg_largeobject_meta in some comments in
> src/include/catalog/pg_largeobject_metadata.h

Fixed,

The attached patch is revised based on the comments.

Below is the diffset from the previous revision (r2328).

[kaigai(at)saba ~]$ diff -u r2328.patch r2333.patch
--- r2328.patch 2009-09-28 16:37:19.000000000 +0900
+++ r2333.patch 2009-09-28 16:36:55.000000000 +0900
@@ -1,6 +1,6 @@
diff -Nrpc base/doc/src/sgml/config.sgml blob/doc/src/sgml/config.sgml
*** base/doc/src/sgml/config.sgml Thu Sep 24 08:43:31 2009
---- blob/doc/src/sgml/config.sgml Fri Sep 25 09:00:55 2009
+--- blob/doc/src/sgml/config.sgml Mon Sep 28 16:32:50 2009
*************** dynamic_library_path = 'C:\tools\postgre
*** 4797,4802 ****
--- 4797,4830 ----
@@ -27,7 +27,7 @@
+ checks corresponding to largeobjects.
+ For example, the <literal>lo_import()</literal> and
+ <literal>lo_export</literal> need superuser privileges independent
-+ from this setting, as if the prior version doing.
++ from this setting as prior versions were doing.
+ </para>
+ <para>
+ It is <literal>on</literal> by default.
@@ -1990,21 +1990,21 @@
DECLARE_UNIQUE_INDEX(pg_namespace_oid_index, 2685, on pg_namespace using btree(oid oid_ops));
diff -Nrpc base/src/include/catalog/pg_largeobject_metadata.h blob/src/include/catalog/pg_largeobject_metadata.h
*** base/src/include/catalog/pg_largeobject_metadata.h Thu Jan 1 09:00:00 1970
---- blob/src/include/catalog/pg_largeobject_metadata.h Fri Sep 25 09:00:55 2009
+--- blob/src/include/catalog/pg_largeobject_metadata.h Mon Sep 28 16:31:11 2009
***************
*** 0 ****
--- 1,67 ----
+ /*-------------------------------------------------------------------------
+ *
-+ * pg_largeobject_meta.h
-+ * definition of the system "largeobject_meta" relation (pg_largeobject_meta)
++ * pg_largeobject_metadata.h
++ * definition of the system "largeobject_metadata" relation (pg_largeobject_metadata)
+ * along with the relation's initial contents.
+ *
+ *
+ * Portions Copyright (c) 1996-2009, PostgreSQL Global Development Group
+ * Portions Copyright (c) 1994, Regents of the University of California
+ *
-+ * $PostgreSQL: pgsql/src/include/catalog/pg_largeobject_meta.h,v 1.24 2009/01/01 17:23:57 momjian Exp $
++ * $PostgreSQL: pgsql/src/include/catalog/pg_largeobject_metadata.h,v 1.24 2009/01/01 17:23:57 momjian Exp $
+ *
+ * NOTES
+ * the genbki.sh script reads this file and generates .bki
@@ -2012,14 +2012,14 @@
+ *
+ *-------------------------------------------------------------------------
+ */
-+ #ifndef PG_LARGEOBJECT_META_H
-+ #define PG_LARGEOBJECT_META_H
++ #ifndef PG_LARGEOBJECT_METADATA_H
++ #define PG_LARGEOBJECT_METADATA_H
+
+ #include "catalog/genbki.h"
+
+ /* ----------------
-+ * pg_largeobject definition. cpp turns this into
-+ * typedef struct FormData_pg_largeobject_meta
++ * pg_largeobject_metadata definition. cpp turns this into
++ * typedef struct FormData_pg_largeobject_metadata
+ * ----------------
+ */
+ #define LargeObjectMetadataRelationId 2336
@@ -2060,7 +2060,7 @@
+ extern void ac_largeobject_export(Oid loid, const char *filename);
+ extern void ac_largeobject_import(Oid loid, const char *filename);
+
-+ #endif /* PG_LARGEOBJECT_META_H */
++ #endif /* PG_LARGEOBJECT_METADATA_H */
diff -Nrpc base/src/include/nodes/parsenodes.h blob/src/include/nodes/parsenodes.h
*** base/src/include/nodes/parsenodes.h Thu Sep 24 08:43:31 2009
--- blob/src/include/nodes/parsenodes.h Thu Sep 24 09:04:49 2009

--
OSS Platform Development Division, NEC
KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>

I rebased the largeobject access controls patch to the CVS HEAD
because of the patch confliction to the default ACL patch.

The only difference was a switch-case statement was moved from
shdepDropOwned() to RemoveRoleFromObjectACL(), so we had to
change the point to be patched.

I don't think this change needs whole of reviewing again.

Actual changes are as follows:

$ diff -up r2333.patch r2353.patch
<snip>
+*** base/src/backend/catalog/aclchk.c Tue Oct 6 08:45:40 2009
+--- blob/src/backend/catalog/aclchk.c Tue Oct 6 09:44:51 2009
@@ -310,9 +310,21 @@ diff -Nrpc base/src/backend/catalog/aclc
case ACL_OBJECT_NAMESPACE:
foreach(cell, objnames)
{
+*************** RemoveRoleFromObjectACL(Oid roleid, Oid
+*** 1156,1161 ****
+--- 1184,1192 ----
+ case LanguageRelationId:
+ istmt.objtype = ACL_OBJECT_LANGUAGE;
+ break;
++ case LargeObjectMetadataRelationId:
++ istmt.objtype = ACL_OBJECT_LARGEOBJECT;
++ break;
+ case NamespaceRelationId:
+ istmt.objtype = ACL_OBJECT_NAMESPACE;
+ break;
*************** ExecGrant_Language(InternalGrant *istmt)
<snip>
-*** base/src/backend/catalog/pg_shdepend.c Thu Jun 18 10:20:52 2009
---- blob/src/backend/catalog/pg_shdepend.c Thu Sep 24 10:46:38 2009
+*** base/src/backend/catalog/pg_shdepend.c Tue Oct 6 08:45:40 2009
+--- blob/src/backend/catalog/pg_shdepend.c Tue Oct 6 09:44:51 2009
***************
-*** 24,29 ****
---- 24,30 ----
- #include "catalog/pg_conversion.h"
+*** 25,30 ****
+--- 25,31 ----
#include "catalog/pg_database.h"
+ #include "catalog/pg_default_acl.h"
#include "catalog/pg_language.h"
+ #include "catalog/pg_largeobject_metadata.h"
#include "catalog/pg_namespace.h"
#include "catalog/pg_operator.h"
#include "catalog/pg_proc.h"
-*************** shdepDropOwned(List *roleids, DropBehavi
-*** 1210,1215 ****
---- 1211,1219 ----
- case LanguageRelationId:
- istmt.objtype = ACL_OBJECT_LANGUAGE;
- break;
-+ case LargeObjectMetadataRelationId:
-+ istmt.objtype = ACL_OBJECT_LARGEOBJECT;
-+ break;
- case NamespaceRelationId:
- istmt.objtype = ACL_OBJECT_NAMESPACE;
- break;
*************** shdepReassignOwned(List *roleids, Oid ne
-*** 1365,1370 ****
---- 1369,1378 ----
+*** 1332,1337 ****
+--- 1333,1342 ----
AlterLanguageOwner_oid(sdepForm->objid, newrole);
break;

@@ -1178,9 +1178,9 @@ diff -Nrpc base/src/backend/catalog/pg_s
+ AlterLargeObjectOwner(sdepForm->objid, newrole);
+ break;
+
- default:
- elog(ERROR, "unexpected classid %d", sdepForm->classid);
- break;
+ case DefaultAclRelationId:
+ /*
+ * Ignore default ACLs; they should be handled by
diff -Nrpc base/src/backend/commands/alter.c blob/src/backend/commands/alter.c
*** base/src/backend/commands/alter.c Sat Jan 3 13:01:35 2009
--- blob/src/backend/commands/alter.c Thu Sep 24 10:46:38 2009

--
OSS Platform Development Division, NEC
KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>

KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com> writes:
> I rebased the largeobject access controls patch to the CVS HEAD
> because of the patch confliction to the default ACL patch.

Quick comment on this --- I think that using a syscache for large
objects is probably not a good idea. There is no provision in the
catcache code for limiting the cache size anymore, and that means that
anybody who touches a large number of large objects is going to blow out
memory. We removed the old cache limit code because that seemed most
sensible for the use of the caches for regular catalog objects, but
I don't think LOs will have the same characteristics with respect to
either number of objects or locality of access.

regards, tom lane

Tom Lane wrote:
> KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com> writes:
>> I rebased the largeobject access controls patch to the CVS HEAD
>> because of the patch confliction to the default ACL patch.
>
> Quick comment on this --- I think that using a syscache for large
> objects is probably not a good idea. There is no provision in the
> catcache code for limiting the cache size anymore, and that means that
> anybody who touches a large number of large objects is going to blow out
> memory. We removed the old cache limit code because that seemed most
> sensible for the use of the caches for regular catalog objects, but
> I don't think LOs will have the same characteristics with respect to
> either number of objects or locality of access.

Are you talking about syscache.c?

I added a syscache entry for pg_largeobject_metadata, not pg_largeobject
which contains data chunks. The pg_largeobject_metadata is a smaller catalog
than most of system catalogs, such as pg_class.

Thanks,
--
KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>

KaiGai Kohei escribió:

> I added a syscache entry for pg_largeobject_metadata, not pg_largeobject
> which contains data chunks. The pg_largeobject_metadata is a smaller catalog
> than most of system catalogs, such as pg_class.

The point is not the size of the cache entries, but the number of them.

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

Alvaro Herrera wrote:
> KaiGai Kohei escribió:
>
>> I added a syscache entry for pg_largeobject_metadata, not pg_largeobject
>> which contains data chunks. The pg_largeobject_metadata is a smaller catalog
>> than most of system catalogs, such as pg_class.
>
> The point is not the size of the cache entries, but the number of them.

If so, I'll replace any routines which use LARGEOBJECTOID cache.
Please wait for the revised patch.

Is there any other comment?
--
KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>

Tom Lane wrote:
> KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com> writes:
>> I rebased the largeobject access controls patch to the CVS HEAD
>> because of the patch confliction to the default ACL patch.
>
> Quick comment on this --- I think that using a syscache for large
> objects is probably not a good idea. There is no provision in the
> catcache code for limiting the cache size anymore, and that means that
> anybody who touches a large number of large objects is going to blow out
> memory. We removed the old cache limit code because that seemed most
> sensible for the use of the caches for regular catalog objects, but
> I don't think LOs will have the same characteristics with respect to
> either number of objects or locality of access.

The attached patch is the revised largeobject access controls.

It replaced any usage of system cache for pg_largeobject_metadata.
In this patch, we basically follow the manner in the pg_tablespace
which also does not have its own system cache.
For example, it needs to open the pg_largeobject_metadata relation
with AccessShareLock when pg_largeobject_aclcheck() is called, as
pg_tablespace_aclcheck() doing.

$ diffstat sepgsql-02-blob-8.5devel-r2354.patch.gz
doc/src/sgml/config.sgml | 28 +
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 | 294 ++++++++++++++++++
src/backend/catalog/dependency.c | 14
src/backend/catalog/pg_largeobject.c | 406 +++++++++++++++++!!!!!!!!
src/backend/catalog/pg_shdepend.c | 5
src/backend/commands/alter.c | 5
src/backend/commands/comment.c | 7
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 | 108 +---!
src/backend/tcop/utility.c | 3
src/backend/utils/adt/acl.c | 5
src/backend/utils/misc/guc.c | 10
src/backend/utils/misc/postgresql.conf.sample | 1
src/bin/psql/large_obj.c | 10
src/include/catalog/dependency.h | 1
src/include/catalog/indexing.h | 3
src/include/catalog/pg_largeobject.h | 4
src/include/catalog/pg_largeobject_metadata.h | 68 ++++
src/include/nodes/parsenodes.h | 1
src/include/utils/acl.h | 6
src/test/regress/expected/privileges.out | 206 +++++++++++++
src/test/regress/expected/sanity_check.out | 3
src/test/regress/sql/privileges.sql | 84 +++++
31 files changed, 1166 insertions(+), 80 deletions(-), 193 modifications(!)

--
OSS Platform Development Division, NEC
KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>

The attached patch is the revised one for largeobejct access controls,
because it conflicted to the "GRANT/REVOKE ON ALL xxx IN SCHEMA".

No other changes are here.
Please apply this one, instead of the older revision (r2354).

Thanks,

$ diffstat /home/kaigai/RPMS/SOURCES/sepgsql-02-blob-8.5devel-r2362.patch.gz
doc/src/sgml/config.sgml | 28 +
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 | 294 ++++++++++++++++++
src/backend/catalog/dependency.c | 14
src/backend/catalog/pg_largeobject.c | 406 +++++++++++++++++!!!!!!!!
src/backend/catalog/pg_shdepend.c | 5
src/backend/commands/alter.c | 5
src/backend/commands/comment.c | 7
src/backend/commands/tablecmds.c | 1
src/backend/libpq/be-fsstubs.c | 49 +-
src/backend/parser/gram.y | 21 +
src/backend/storage/large_object/inv_api.c | 108 +---!
src/backend/tcop/utility.c | 3
src/backend/utils/adt/acl.c | 5
src/backend/utils/misc/guc.c | 10
src/backend/utils/misc/postgresql.conf.sample | 1
src/bin/psql/large_obj.c | 10
src/include/catalog/dependency.h | 1
src/include/catalog/indexing.h | 3
src/include/catalog/pg_largeobject.h | 4
src/include/catalog/pg_largeobject_metadata.h | 68 ++++
src/include/nodes/parsenodes.h | 1
src/include/utils/acl.h | 6
src/test/regress/expected/privileges.out | 206 +++++++++++++
src/test/regress/expected/sanity_check.out | 3
src/test/regress/sql/privileges.sql | 84 +++++
31 files changed, 1167 insertions(+), 80 deletions(-), 193 modifications(!)

KaiGai Kohei wrote:
> Tom Lane wrote:
>> KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com> writes:
>>> I rebased the largeobject access controls patch to the CVS HEAD
>>> because of the patch confliction to the default ACL patch.
>> Quick comment on this --- I think that using a syscache for large
>> objects is probably not a good idea. There is no provision in the
>> catcache code for limiting the cache size anymore, and that means that
>> anybody who touches a large number of large objects is going to blow out
>> memory. We removed the old cache limit code because that seemed most
>> sensible for the use of the caches for regular catalog objects, but
>> I don't think LOs will have the same characteristics with respect to
>> either number of objects or locality of access.
>
> The attached patch is the revised largeobject access controls.
>
> It replaced any usage of system cache for pg_largeobject_metadata.
> In this patch, we basically follow the manner in the pg_tablespace
> which also does not have its own system cache.
> For example, it needs to open the pg_largeobject_metadata relation
> with AccessShareLock when pg_largeobject_aclcheck() is called, as
> pg_tablespace_aclcheck() doing.
>
>
> $ diffstat sepgsql-02-blob-8.5devel-r2354.patch.gz
> doc/src/sgml/config.sgml | 28 +
> 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 | 294 ++++++++++++++++++
> src/backend/catalog/dependency.c | 14
> src/backend/catalog/pg_largeobject.c | 406 +++++++++++++++++!!!!!!!!
> src/backend/catalog/pg_shdepend.c | 5
> src/backend/commands/alter.c | 5
> src/backend/commands/comment.c | 7
> 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 | 108 +---!
> src/backend/tcop/utility.c | 3
> src/backend/utils/adt/acl.c | 5
> src/backend/utils/misc/guc.c | 10
> src/backend/utils/misc/postgresql.conf.sample | 1
> src/bin/psql/large_obj.c | 10
> src/include/catalog/dependency.h | 1
> src/include/catalog/indexing.h | 3
> src/include/catalog/pg_largeobject.h | 4
> src/include/catalog/pg_largeobject_metadata.h | 68 ++++
> src/include/nodes/parsenodes.h | 1
> src/include/utils/acl.h | 6
> src/test/regress/expected/privileges.out | 206 +++++++++++++
> src/test/regress/expected/sanity_check.out | 3
> src/test/regress/sql/privileges.sql | 84 +++++
> 31 files changed, 1166 insertions(+), 80 deletions(-), 193 modifications(!)
>
>
>
> ------------------------------------------------------------------------
>
>

--
OSS Platform Development Division, NEC
KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>

KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com> writes:
> The attached patch is the revised one for largeobejct access controls,
> because it conflicted to the "GRANT/REVOKE ON ALL xxx IN SCHEMA".

I started to look through this patch with the hope of committing it, but
found out that it's not really ready.

The most serious problem is that you ripped out myLargeObjectExists(),
apparently because you didn't understand what it's there for. The reason
it's there is to ensure that pg_dump runs don't fail. pg_dump is expected
to produce a consistent dump of all large objects that existed at the
time of its transaction snapshot. With this code, pg_dump would get a
"large object doesn't exist" error on any LO that is deleted between the
time of the snapshot and the time pg_dump actually tries to dump it ---
which could be quite a large window in a large database.

The reason this is a significant problem and not just an easily fixable
oversight is that it's not entirely clear what to do instead. We can
certainly make the pure existence test use the query snapshot instead of
SnapshotNow, but what about the implied permissions tests? Should we
attempt to make them run using the version of the LO's ACL found in the
query-snapshot-time metadata row? The trouble with that is it might refer
to roles that don't exist anymore, perhaps resulting in failures down
inside the ACL checking routines. It would be safer to rely on the
current metadata row contents, but then we have the question of whether to
allow the access if the row doesn't exist according to SnapshotNow.

Now these issues arise to some extent already in pg_dump, but the current
window for failure is quite short because it obtains access share locks on
all the tables it will dump at the start of the run. With large objects
the window in which things could have changed is very much longer.

Of course, in the cases that people are most concerned about, pg_dump is
running as superuser and so the actual ACL contents don't really matter
anyway, so long as we don't fail outright before getting to the check.
So I'm kind of inclined to say that the least evil solution is to apply
the permissions check using the query-snapshot-time metadata row.
It's definitely a debatable question though. We'd also want to make sure
that the aclcheck code doesn't fail if it finds a nonexistent role ID
in the ACL.

Moving on to lesser but still significant problems, you probably already
guessed my next one: there's no pg_dump support. If the system tracks
owner and ACL for large objects, pg_dump *must* be prepared to dump that
information. It'd also be worthwhile to teach pg_dump that in servers >=
8.5, it can look in the metadata catalog to make the list of LO OIDs
instead of having to do a SELECT DISTINCT from pg_largeobject.

I notice that the patch decides to change the pg_description classoid for
LO comments from pg_largeobject's OID to pg_largeobject_metadata's. This
will break existing clients that look at pg_description (eg, pg_dump and
psql, plus any other clients that have any intelligence about comments,
for instance it probably breaks pgAdmin). And there's just not a lot of
return that I can see. I agree that using pg_largeobject_metadata would
be more consistent given the new catalog layout, but I'm inclined to think
we should stick to the old convention on compatibility grounds. Given
that choice, for consistency we'd better also use pg_largeobject's OID not
pg_largeobject_metadata's in pg_shdepend entries for LOs.

In the category of lesser issues that have still got to be fixed:

* You need to pay more attention to updating comments. For example
your changes to LargeObjectCreate render its header comment a complete
lie, but you didn't change the comment. (And what is the purpose of
renaming it to CreateLargeObject, and similarly for the adjacent
routines? You didn't change the API meaningfully, so there is no
reason to break calling code that way.)

* The documentation needs work too, eg a new system catalog requires a
page in catalogs.sgml, and I'll bet there's a few references to large
objects and/or permissions that need to be updated.

* "largeobject" is not an English word. The occurrences in user-visible
messages and documentation must get changed to "large object". I've got
mixed emotions even about using it in code identifiers, although we
certainly aren't going to rename pg_largeobject, so anything that's named
in parallel to that should stay as-is. In the same vein, "acl" is not
a word we want to expose to users, so "largeobject_check_acl" is doubly
bad as a GUC variable name. Perhaps "large_object_privilege_checks"
would do.

* I'm not really happy with the ac_largeobject_foo shim layer, and would
frankly prefer to rip it out and put those tests inline. It's poorly
thought out IMO --- eg, what the heck is that cascade boolean --- and
doesn't look like any of the rest of the Postgres code stylistically, and
it makes the calling code look different from similar tests elsewhere too.
When and if SELinux support ever gets in, I'd expect that the stubs for
it would get put into the aclchk.c routines not into all their callers.
So this doesn't seem to me that it's going in the right direction even if
we posit that that support will get in.

* Lastly, that change in psql is neither version-aware nor schema-safe.
psql is expected to still work with older server versions, so you need
two versions of that query, not just a replacement. And don't omit the
"pg_catalog." qualifier. (Also, I wonder whether it shouldn't have a way
to display permissions for LOs, though maybe that ought to be conditional.
Time for "\lo_list+" perhaps?)

regards, tom lane

Tom Lane wrote:
> KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com> writes:
>> The attached patch is the revised one for largeobejct access controls,
>> because it conflicted to the "GRANT/REVOKE ON ALL xxx IN SCHEMA".
>
> I started to look through this patch with the hope of committing it, but
> found out that it's not really ready.
>
> The most serious problem is that you ripped out myLargeObjectExists(),
> apparently because you didn't understand what it's there for. The reason
> it's there is to ensure that pg_dump runs don't fail. pg_dump is expected
> to produce a consistent dump of all large objects that existed at the
> time of its transaction snapshot. With this code, pg_dump would get a
> "large object doesn't exist" error on any LO that is deleted between the
> time of the snapshot and the time pg_dump actually tries to dump it ---
> which could be quite a large window in a large database.
>
> The reason this is a significant problem and not just an easily fixable
> oversight is that it's not entirely clear what to do instead. We can
> certainly make the pure existence test use the query snapshot instead of
> SnapshotNow, but what about the implied permissions tests? Should we
> attempt to make them run using the version of the LO's ACL found in the
> query-snapshot-time metadata row? The trouble with that is it might refer
> to roles that don't exist anymore, perhaps resulting in failures down
> inside the ACL checking routines. It would be safer to rely on the
> current metadata row contents, but then we have the question of whether to
> allow the access if the row doesn't exist according to SnapshotNow.
>
> Now these issues arise to some extent already in pg_dump, but the current
> window for failure is quite short because it obtains access share locks on
> all the tables it will dump at the start of the run. With large objects
> the window in which things could have changed is very much longer.
>
> Of course, in the cases that people are most concerned about, pg_dump is
> running as superuser and so the actual ACL contents don't really matter
> anyway, so long as we don't fail outright before getting to the check.
> So I'm kind of inclined to say that the least evil solution is to apply
> the permissions check using the query-snapshot-time metadata row.
> It's definitely a debatable question though. We'd also want to make sure
> that the aclcheck code doesn't fail if it finds a nonexistent role ID
> in the ACL.

The origin of this matter is the basis of existence was changed.
Our current basis is whether pg_largeobject has one or more data chunk for
the given loid in the correct snapshot, or not.
The newer one is whether pg_largeobject_metadata has the entry for the given
loid in the SnapshotNow, or not.

The newer basis is same as other database objects, such as functions.
But why pg_dump performs correctly for these database objects?
In my understanding, because it reads the system catalog directly in
the query snapshot. So, it will be visible, if concurrent transaction
dropped a function to be backed up.

Now, pg_dump uses libpq's large object interface which internally uses
loread()/lowrite().
If pg_dump fetches data chunks from the system catalog, it seems to me
the matter is reasonably solvable.

My assumption is that you're not talking about a generic situation when
a certain database object is unavailable even if we can find it within
the system catalog, apart from large object backups.

For example, we can easily produce a similar behavior when we tries to
use a function which can be found in pg_proc, but concurrent transaction
already removed it.
I don't believe PostgreSQL guarantees equivalence between the visibility
of system catalog and the availability of the related database object.
So, is it the simplest approach to patch on the pg_dump?

> Moving on to lesser but still significant problems, you probably already
> guessed my next one: there's no pg_dump support. If the system tracks
> owner and ACL for large objects, pg_dump *must* be prepared to dump that
> information. It'd also be worthwhile to teach pg_dump that in servers >=
> 8.5, it can look in the metadata catalog to make the list of LO OIDs
> instead of having to do a SELECT DISTINCT from pg_largeobject.

Hmm. I planed to add support to the pg_dump next to the serve-side enhancement.
If both of patches are necessary soon, I'll include it in this phase.

> I notice that the patch decides to change the pg_description classoid for
> LO comments from pg_largeobject's OID to pg_largeobject_metadata's. This
> will break existing clients that look at pg_description (eg, pg_dump and
> psql, plus any other clients that have any intelligence about comments,
> for instance it probably breaks pgAdmin). And there's just not a lot of
> return that I can see. I agree that using pg_largeobject_metadata would
> be more consistent given the new catalog layout, but I'm inclined to think
> we should stick to the old convention on compatibility grounds. Given
> that choice, for consistency we'd better also use pg_largeobject's OID not
> pg_largeobject_metadata's in pg_shdepend entries for LOs.

It seems to me reasonable.
I'll fix it with comments why it uses LargeObjectRelationId not
LargeObjectMetadataRelationId here.

> In the category of lesser issues that have still got to be fixed:
>
> * You need to pay more attention to updating comments. For example
> your changes to LargeObjectCreate render its header comment a complete
> lie, but you didn't change the comment.
OK, I'll fix it.

> (And what is the purpose of
> renaming it to CreateLargeObject, and similarly for the adjacent
> routines? You didn't change the API meaningfully, so there is no
> reason to break calling code that way.)
Yes, ExecAlterOwnerStmt() calls AlterXXXXOwner() but only largeobject
has different naming convention, if we follow the existing names such
as LargeObjectCreate().
But I don't have any strong motication for the name. I'll fix it.

> * The documentation needs work too, eg a new system catalog requires a
> page in catalogs.sgml, and I'll bet there's a few references to large
> objects and/or permissions that need to be updated.
I left for adding the page for pg_largeobject_metadata. I'll add it.

> * "largeobject" is not an English word. The occurrences in user-visible
> messages and documentation must get changed to "large object". I've got
> mixed emotions even about using it in code identifiers, although we
> certainly aren't going to rename pg_largeobject, so anything that's named
> in parallel to that should stay as-is.

OK, I'll fix it.

> In the same vein, "acl" is not
> a word we want to expose to users, so "largeobject_check_acl" is doubly
> bad as a GUC variable name. Perhaps "large_object_privilege_checks"
> would do.
Hmm. OK, I'll fix it.

> * I'm not really happy with the ac_largeobject_foo shim layer, and would
> frankly prefer to rip it out and put those tests inline. It's poorly
> thought out IMO --- eg, what the heck is that cascade boolean --- and
> doesn't look like any of the rest of the Postgres code stylistically, and
> it makes the calling code look different from similar tests elsewhere too.
> When and if SELinux support ever gets in, I'd expect that the stubs for
> it would get put into the aclchk.c routines not into all their callers.
> So this doesn't seem to me that it's going in the right direction even if
> we posit that that support will get in.

At the beginning of this commit fest, I was not clear which patch is
merged earlier, so I separated access control routines for easy integration
in the future.
However, it is not a matter to deploy inlined ACL checks in this stage.
If arguable, I'll implement it with the current style in this patch.

BTW, we tried to put SELinux hooks within pg_xxx_aclcheck() routines
but it was already failed on the first commit fest, so we are now working
for the abstranction layer for access controls.

> * Lastly, that change in psql is neither version-aware nor schema-safe.
> psql is expected to still work with older server versions, so you need
> two versions of that query, not just a replacement. And don't omit the
> "pg_catalog." qualifier. (Also, I wonder whether it shouldn't have a way
> to display permissions for LOs, though maybe that ought to be conditional.
> Time for "\lo_list+" perhaps?)

In the meantime, I'll add two versions of that query here.
If we need "\lo_list+", it can be fixed later.

Thanks,
--
OSS Platform Development Division, NEC
KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>

KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com> writes:
> Tom Lane wrote:
>> The most serious problem is that you ripped out myLargeObjectExists(),
>> apparently because you didn't understand what it's there for. The reason
>> it's there is to ensure that pg_dump runs don't fail. pg_dump is expected
>> to produce a consistent dump of all large objects that existed at the
>> time of its transaction snapshot. With this code, pg_dump would get a
>> "large object doesn't exist" error on any LO that is deleted between the
>> time of the snapshot and the time pg_dump actually tries to dump it ---
>> which could be quite a large window in a large database.

> The origin of this matter is the basis of existence was changed.
> Our current basis is whether pg_largeobject has one or more data chunk for
> the given loid in the correct snapshot, or not.
> The newer one is whether pg_largeobject_metadata has the entry for the given
> loid in the SnapshotNow, or not.

Which is wrong. You can certainly switch your attention as to which
catalog to look in, but you can't change the snapshot that the test is
referenced to.

> The newer basis is same as other database objects, such as functions.
> But why pg_dump performs correctly for these database objects?

The reason pg_dump works reasonably well is that it takes locks on
tables, and the other objects it dumps (such as functions) have
indivisible one-row representations in the catalogs. They're either
there when pg_dump looks, or they're not. What you would have here
is that pg_dump would see LO data chunks when it looks into
pg_largeobject using its transaction snapshot, and then its attempts to
open those LO OIDs would fail because the metadata is not there anymore
according to SnapshotNow.

There are certainly plenty of corner-case issues in pg_dump; I've
complained before that it's generally a bad idea to be migrating pg_dump
functionality into internal backend routines that tend to rely on
SnapshotNow. But if we change LO dumping like this, it's not going to
be a corner case --- it's going to be a common race-condition failure
with a window measured in many minutes if not hours. That's more than
sufficient reason to reject this patch, because the added functionality
isn't worth it. And pg_dump isn't even the only client that depends
on the assumption that a read-only open LO is static.

>> Moving on to lesser but still significant problems, you probably already
>> guessed my next one: there's no pg_dump support.

> Hmm. I planed to add support to the pg_dump next to the serve-side enhancement.

We do not commit system changes that lack necessary pg_dump support.
There are some things you can leave till later, but pg_dump is not
negotiable. (Otherwise, testers would be left with databases they
can't dump/reload across the next forced initdb.)

regards, tom lane

On Wed, Oct 14, 2009 at 12:02 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com> writes:
>> Tom Lane wrote:
>>> The most serious problem is that you ripped out myLargeObjectExists(),
>>> apparently because you didn't understand what it's there for.  The reason
>>> it's there is to ensure that pg_dump runs don't fail.  pg_dump is expected
>>> to produce a consistent dump of all large objects that existed at the
>>> time of its transaction snapshot.  With this code, pg_dump would get a
>>> "large object doesn't exist" error on any LO that is deleted between the
>>> time of the snapshot and the time pg_dump actually tries to dump it ---
>>> which could be quite a large window in a large database.
>
>> The origin of this matter is the basis of existence was changed.
>> Our current basis is whether pg_largeobject has one or more data chunk for
>> the given loid in the correct snapshot, or not.
>> The newer one is whether pg_largeobject_metadata has the entry for the given
>> loid in the SnapshotNow, or not.
>
> Which is wrong.  You can certainly switch your attention as to which
> catalog to look in, but you can't change the snapshot that the test is
> referenced to.
>
>> The newer basis is same as other database objects, such as functions.
>> But why pg_dump performs correctly for these database objects?
>
> The reason pg_dump works reasonably well is that it takes locks on
> tables, and the other objects it dumps (such as functions) have
> indivisible one-row representations in the catalogs.  They're either
> there when pg_dump looks, or they're not.  What you would have here
> is that pg_dump would see LO data chunks when it looks into
> pg_largeobject using its transaction snapshot, and then its attempts to
> open those LO OIDs would fail because the metadata is not there anymore
> according to SnapshotNow.
>
> There are certainly plenty of corner-case issues in pg_dump; I've
> complained before that it's generally a bad idea to be migrating pg_dump
> functionality into internal backend routines that tend to rely on
> SnapshotNow.  But if we change LO dumping like this, it's not going to
> be a corner case --- it's going to be a common race-condition failure
> with a window measured in many minutes if not hours.  That's more than
> sufficient reason to reject this patch, because the added functionality
> isn't worth it.  And pg_dump isn't even the only client that depends
> on the assumption that a read-only open LO is static.
>
>>> Moving on to lesser but still significant problems, you probably already
>>> guessed my next one: there's no pg_dump support.
>
>> Hmm. I planed to add support to the pg_dump next to the serve-side enhancement.
>
> We do not commit system changes that lack necessary pg_dump support.
> There are some things you can leave till later, but pg_dump is not
> negotiable.  (Otherwise, testers would be left with databases they
> can't dump/reload across the next forced initdb.)

As part of closing out this CommitFest, I am marking this patch as
Returned with Feedback. Because the amount of work that remains to be
done is so substantial, that might have been a good idea anyway, but
since the clock is expiring the point is moot.

...Robert

Tom Lane wrote:
>> The newer basis is same as other database objects, such as functions.
>> But why pg_dump performs correctly for these database objects?
>
> The reason pg_dump works reasonably well is that it takes locks on
> tables, and the other objects it dumps (such as functions) have
> indivisible one-row representations in the catalogs. They're either
> there when pg_dump looks, or they're not. What you would have here
> is that pg_dump would see LO data chunks when it looks into
> pg_largeobject using its transaction snapshot, and then its attempts to
> open those LO OIDs would fail because the metadata is not there anymore
> according to SnapshotNow.

It needs to break down the matter more simple.

This patch adds the pg_largeobject_metadata catalog, and a certain entry
within the catalog is refered by multiple entries within pg_largeobject.
At the viewpoint of data model, it is equivalent that any pg_largeobject
tuples which share same LOID always have common copy of the metadata.
(If we tries to implement this actually, it needs to keep consistency of
the part of metadata for each writer operations!)

In the current design, when we access a certain large object with read-only
mode, query's snapshot is used, not always SnapshotNow.
If we assume any data chunk has its metadata part, the metadata should be
also refered from the query's snapshot. In fact, this patch stores the
part of metadata within pg_largeobject_metadata. But it seems to me the
principle is that same snapshot which used for data chunks should be
applied to scan the metadata.

So, I can agree the following approach:

> So I'm kind of inclined to say that the least evil solution is to apply
> the permissions check using the query-snapshot-time metadata row.
> It's definitely a debatable question though. We'd also want to make sure
> that the aclcheck code doesn't fail if it finds a nonexistent role ID
> in the ACL.

In this approach, we cannot apply the current pg_largeobject_aclcheck()
because it does not have an argument to deliver the preferable snapshot.
So, we need to extract acl field from the tuple being visible from the
appropriate snapshot, then calls aclmask() routine.

The aclmask() just compares identifiers in numeris representation,
so it seems to me this routine does not raise an error if it finds
a nonexistent role ID from the viewpoint of SnapshotNow.

aclmask(const Acl *acl, Oid roleid, Oid ownerId,
AclMode mask, AclMaskHow how)

It requires five arguments. The acl and ownerId can be fetched from the
metadata with query's snapshot. The roleid can be given by GetUserId().
The mask and how are constant.

An I missing something?

> We do not commit system changes that lack necessary pg_dump support.
> There are some things you can leave till later, but pg_dump is not
> negotiable. (Otherwise, testers would be left with databases they
> can't dump/reload across the next forced initdb.)

OK, I'll add support pg_dump soon.

Thanks,
--
OSS Platform Development Division, NEC
KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>

Robert Haas wrote:
> On Wed, Oct 14, 2009 at 12:02 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com> writes:
>>> Tom Lane wrote:
>>>> The most serious problem is that you ripped out myLargeObjectExists(),
>>>> apparently because you didn't understand what it's there for. The reason
>>>> it's there is to ensure that pg_dump runs don't fail. pg_dump is expected
>>>> to produce a consistent dump of all large objects that existed at the
>>>> time of its transaction snapshot. With this code, pg_dump would get a
>>>> "large object doesn't exist" error on any LO that is deleted between the
>>>> time of the snapshot and the time pg_dump actually tries to dump it ---
>>>> which could be quite a large window in a large database.
>>> The origin of this matter is the basis of existence was changed.
>>> Our current basis is whether pg_largeobject has one or more data chunk for
>>> the given loid in the correct snapshot, or not.
>>> The newer one is whether pg_largeobject_metadata has the entry for the given
>>> loid in the SnapshotNow, or not.
>> Which is wrong. You can certainly switch your attention as to which
>> catalog to look in, but you can't change the snapshot that the test is
>> referenced to.
>>
>>> The newer basis is same as other database objects, such as functions.
>>> But why pg_dump performs correctly for these database objects?
>> The reason pg_dump works reasonably well is that it takes locks on
>> tables, and the other objects it dumps (such as functions) have
>> indivisible one-row representations in the catalogs. They're either
>> there when pg_dump looks, or they're not. What you would have here
>> is that pg_dump would see LO data chunks when it looks into
>> pg_largeobject using its transaction snapshot, and then its attempts to
>> open those LO OIDs would fail because the metadata is not there anymore
>> according to SnapshotNow.
>>
>> There are certainly plenty of corner-case issues in pg_dump; I've
>> complained before that it's generally a bad idea to be migrating pg_dump
>> functionality into internal backend routines that tend to rely on
>> SnapshotNow. But if we change LO dumping like this, it's not going to
>> be a corner case --- it's going to be a common race-condition failure
>> with a window measured in many minutes if not hours. That's more than
>> sufficient reason to reject this patch, because the added functionality
>> isn't worth it. And pg_dump isn't even the only client that depends
>> on the assumption that a read-only open LO is static.
>>
>>>> Moving on to lesser but still significant problems, you probably already
>>>> guessed my next one: there's no pg_dump support.
>>> Hmm. I planed to add support to the pg_dump next to the serve-side enhancement.
>> We do not commit system changes that lack necessary pg_dump support.
>> There are some things you can leave till later, but pg_dump is not
>> negotiable. (Otherwise, testers would be left with databases they
>> can't dump/reload across the next forced initdb.)
>
> As part of closing out this CommitFest, I am marking this patch as
> Returned with Feedback. Because the amount of work that remains to be
> done is so substantial, that might have been a good idea anyway, but
> since the clock is expiring the point is moot.

Could you wait for the next 24 hours please?

It is unproductive to break off the discussion for a month,
even if the patch will be actually commited at the December.

Thanks,
--
OSS Platform Development Division, NEC
KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>

2009/10/14 KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>:
> Robert Haas wrote:
>> On Wed, Oct 14, 2009 at 12:02 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>> KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com> writes:
>>>> Tom Lane wrote:
>>>>> The most serious problem is that you ripped out myLargeObjectExists(),
>>>>> apparently because you didn't understand what it's there for.  The reason
>>>>> it's there is to ensure that pg_dump runs don't fail.  pg_dump is expected
>>>>> to produce a consistent dump of all large objects that existed at the
>>>>> time of its transaction snapshot.  With this code, pg_dump would get a
>>>>> "large object doesn't exist" error on any LO that is deleted between the
>>>>> time of the snapshot and the time pg_dump actually tries to dump it ---
>>>>> which could be quite a large window in a large database.
>>>> The origin of this matter is the basis of existence was changed.
>>>> Our current basis is whether pg_largeobject has one or more data chunk for
>>>> the given loid in the correct snapshot, or not.
>>>> The newer one is whether pg_largeobject_metadata has the entry for the given
>>>> loid in the SnapshotNow, or not.
>>> Which is wrong.  You can certainly switch your attention as to which
>>> catalog to look in, but you can't change the snapshot that the test is
>>> referenced to.
>>>
>>>> The newer basis is same as other database objects, such as functions.
>>>> But why pg_dump performs correctly for these database objects?
>>> The reason pg_dump works reasonably well is that it takes locks on
>>> tables, and the other objects it dumps (such as functions) have
>>> indivisible one-row representations in the catalogs.  They're either
>>> there when pg_dump looks, or they're not.  What you would have here
>>> is that pg_dump would see LO data chunks when it looks into
>>> pg_largeobject using its transaction snapshot, and then its attempts to
>>> open those LO OIDs would fail because the metadata is not there anymore
>>> according to SnapshotNow.
>>>
>>> There are certainly plenty of corner-case issues in pg_dump; I've
>>> complained before that it's generally a bad idea to be migrating pg_dump
>>> functionality into internal backend routines that tend to rely on
>>> SnapshotNow.  But if we change LO dumping like this, it's not going to
>>> be a corner case --- it's going to be a common race-condition failure
>>> with a window measured in many minutes if not hours.  That's more than
>>> sufficient reason to reject this patch, because the added functionality
>>> isn't worth it.  And pg_dump isn't even the only client that depends
>>> on the assumption that a read-only open LO is static.
>>>
>>>>> Moving on to lesser but still significant problems, you probably already
>>>>> guessed my next one: there's no pg_dump support.
>>>> Hmm. I planed to add support to the pg_dump next to the serve-side enhancement.
>>> We do not commit system changes that lack necessary pg_dump support.
>>> There are some things you can leave till later, but pg_dump is not
>>> negotiable.  (Otherwise, testers would be left with databases they
>>> can't dump/reload across the next forced initdb.)
>>
>> As part of closing out this CommitFest, I am marking this patch as
>> Returned with Feedback.  Because the amount of work that remains to be
>> done is so substantial, that might have been a good idea anyway, but
>> since the clock is expiring the point is moot.
>
> Could you wait for the next 24 hours please?
>
> It is unproductive to break off the discussion for a month,
> even if the patch will be actually commited at the December.

Well, I don't know that I have the deciding vote here. I don't have
any super-powers to make Tom - or anyone else - review the next
version of this patch for this CommitFest, the next CommitFest, or at
all. In my ideal world, Tom would review every patch in his area of
expertise the day it came in and commit it right away, especially the
ones that implement features I happen to like. But unless I offer to
pay Tom's salary, and maybe not even then, that isn't going to happen.

Backing up a little bit, my understanding of the CommitFest process is
that it is a time for everyone to stop working on their own patches
and help review and commit the patches of others. Because everyone
wants to get back to their own work, we try very hard to wrap
CommitFests up in one month so that everyone can then have a full
month to do their own work before the next CommitFest starts. I don't
see any reason to believe that this patch is so important that we
should make an exception to that policy on its behalf, and I think
it's unfair of you to ask. Tom, I, and others have almost entirely
put aside our own development work for the last month to focus on this
CommitFest. You haven't offered to help, are now asking us to put
aside our work for even longer for your benefit. Furthermore, you
haven't offered to help with either of the previous two CommitFests in
which I've been involved either, despite having submitted large,
complex patches that required substantial reviewing effort.

All that having been said, I have no intention of or desire to
foreclose discussion on this patch, or any desire to postpone the time
that it gets reviewed and committed. Besides the general desire to
let everyone get back to their own work if they so choose, the other
point of marking this as Returned with Feedback is to say that it
isn't going to get committed before alpha2 is stamped, which I think
is probably true even if a new version shows up in the next 5 minutes.
But that doesn't mean we can't keep talking about it, and I hope we
do.

...Robert

Robert Haas wrote:
> 2009/10/14 KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>:
>> Robert Haas wrote:
>>> On Wed, Oct 14, 2009 at 12:02 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>>> KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com> writes:
>>>>> Tom Lane wrote:
>>>>>> The most serious problem is that you ripped out myLargeObjectExists(),
>>>>>> apparently because you didn't understand what it's there for. The reason
>>>>>> it's there is to ensure that pg_dump runs don't fail. pg_dump is expected
>>>>>> to produce a consistent dump of all large objects that existed at the
>>>>>> time of its transaction snapshot. With this code, pg_dump would get a
>>>>>> "large object doesn't exist" error on any LO that is deleted between the
>>>>>> time of the snapshot and the time pg_dump actually tries to dump it ---
>>>>>> which could be quite a large window in a large database.
>>>>> The origin of this matter is the basis of existence was changed.
>>>>> Our current basis is whether pg_largeobject has one or more data chunk for
>>>>> the given loid in the correct snapshot, or not.
>>>>> The newer one is whether pg_largeobject_metadata has the entry for the given
>>>>> loid in the SnapshotNow, or not.
>>>> Which is wrong. You can certainly switch your attention as to which
>>>> catalog to look in, but you can't change the snapshot that the test is
>>>> referenced to.
>>>>
>>>>> The newer basis is same as other database objects, such as functions.
>>>>> But why pg_dump performs correctly for these database objects?
>>>> The reason pg_dump works reasonably well is that it takes locks on
>>>> tables, and the other objects it dumps (such as functions) have
>>>> indivisible one-row representations in the catalogs. They're either
>>>> there when pg_dump looks, or they're not. What you would have here
>>>> is that pg_dump would see LO data chunks when it looks into
>>>> pg_largeobject using its transaction snapshot, and then its attempts to
>>>> open those LO OIDs would fail because the metadata is not there anymore
>>>> according to SnapshotNow.
>>>>
>>>> There are certainly plenty of corner-case issues in pg_dump; I've
>>>> complained before that it's generally a bad idea to be migrating pg_dump
>>>> functionality into internal backend routines that tend to rely on
>>>> SnapshotNow. But if we change LO dumping like this, it's not going to
>>>> be a corner case --- it's going to be a common race-condition failure
>>>> with a window measured in many minutes if not hours. That's more than
>>>> sufficient reason to reject this patch, because the added functionality
>>>> isn't worth it. And pg_dump isn't even the only client that depends
>>>> on the assumption that a read-only open LO is static.
>>>>
>>>>>> Moving on to lesser but still significant problems, you probably already
>>>>>> guessed my next one: there's no pg_dump support.
>>>>> Hmm. I planed to add support to the pg_dump next to the serve-side enhancement.
>>>> We do not commit system changes that lack necessary pg_dump support.
>>>> There are some things you can leave till later, but pg_dump is not
>>>> negotiable. (Otherwise, testers would be left with databases they
>>>> can't dump/reload across the next forced initdb.)
>>> As part of closing out this CommitFest, I am marking this patch as
>>> Returned with Feedback. Because the amount of work that remains to be
>>> done is so substantial, that might have been a good idea anyway, but
>>> since the clock is expiring the point is moot.
>> Could you wait for the next 24 hours please?
>>
>> It is unproductive to break off the discussion for a month,
>> even if the patch will be actually commited at the December.
>
> Well, I don't know that I have the deciding vote here. I don't have
> any super-powers to make Tom - or anyone else - review the next
> version of this patch for this CommitFest, the next CommitFest, or at
> all. In my ideal world, Tom would review every patch in his area of
> expertise the day it came in and commit it right away, especially the
> ones that implement features I happen to like. But unless I offer to
> pay Tom's salary, and maybe not even then, that isn't going to happen.

I'm sorry for this a little bit emotional response.

Anyway, what I wanted to say was my wish that I wanted to conclude
or make a direction for the issue (what snapshot should be applied.)
It is unclear now, so I would like to continue the discussion a bit
more.

> Backing up a little bit, my understanding of the CommitFest process is
> that it is a time for everyone to stop working on their own patches
> and help review and commit the patches of others. Because everyone
> wants to get back to their own work, we try very hard to wrap
> CommitFests up in one month so that everyone can then have a full
> month to do their own work before the next CommitFest starts. I don't
> see any reason to believe that this patch is so important that we
> should make an exception to that policy on its behalf, and I think
> it's unfair of you to ask. Tom, I, and others have almost entirely
> put aside our own development work for the last month to focus on this
> CommitFest. You haven't offered to help, are now asking us to put
> aside our work for even longer for your benefit. Furthermore, you
> haven't offered to help with either of the previous two CommitFests in
> which I've been involved either, despite having submitted large,
> complex patches that required substantial reviewing effort.

I have to focus on my patches with highest priority in CommitFest,
but it may be possible to help reviewing the proposed patches in
the off-fest season. It is illegal/undesirable for the process?

We already can find a patch corresponding to security feature.

Thanks,
--
OSS Platform Development Division, NEC
KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>

KaiGai Kohei wrote:
> I have to focus on my patches with highest priority in CommitFest,
> but it may be possible to help reviewing the proposed patches in
> the off-fest season. It is illegal/undesirable for the process?

No, that's absolutely fine. During commitfests patch review is needed
the most, but please do help whenever you have the time.

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com

2009/10/15 Heikki Linnakangas <heikki(dot)linnakangas(at)enterprisedb(dot)com>:
> KaiGai Kohei wrote:
>> I have to focus on my patches with highest priority in CommitFest,
>> but it may be possible to help reviewing the proposed patches in
>> the off-fest season. It is illegal/undesirable for the process?
>
> No, that's absolutely fine. During commitfests patch review is needed
> the most, but please do help whenever you have the time.

I agree. I would also mention that reviewing a patch can often be
done in a few hours, and there are certainly periods of downtime
during a CommitFest when your own patches won't need attention. Of
course, a major patch like Hot Standby needs a lot more reviewing, but
that's an exceptional case, and there's really no need for you to bite
off something that ambitious. Just reviewing 1 or 2 small patches per
CommitFest would be much appreciated, if it's something you can
manage.

Reviewing patches at other times is very helpful too. Many times
patch authors complain about not getting feedback between CommitFests,
so anything you can pick up and give feedback on helps move the
project forward (and also decreases the amount that has to get done
during the next CommitFest).

Thanks,

...Robert