Re: Largeobject Access Controls (r2460)

2009/12/22 KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>:
> (2009/12/21 9:39), KaiGai Kohei wrote:
>> (2009/12/19 12:05), Robert Haas wrote:
>>> On Fri, Dec 18, 2009 at 9:48 PM, Tom Lane<tgl(at)sss(dot)pgh(dot)pa(dot)us>   wrote:
>>>> Robert Haas<robertmhaas(at)gmail(dot)com>   writes:
>>>>> Oh.  This is more complicated than it appeared on the surface.  It
>>>>> seems that the string "BLOB COMMENTS" actually gets inserted into
>>>>> custom dumps somewhere, so I'm not sure whether we can just change it.
>>>>>    Was this issue discussed at some point before this was committed?
>>>>> Changing it would seem to require inserting some backward
>>>>> compatibility code here.  Another option would be to add a separate
>>>>> section for "BLOB METADATA", and leave "BLOB COMMENTS" alone.  Can
>>>>> anyone comment on what the Right Thing To Do is here?
>>>>
>>>> The BLOB COMMENTS label is, or was, correct for what it contained.
>>>> If this patch has usurped it to contain other things
>>>
>>> It has.
>>>
>>>> I would argue
>>>> that that is seriously wrong.  pg_dump already has a clear notion
>>>> of how to handle ACLs for objects.  ACLs for blobs ought to be
>>>> made to fit into that structure, not dumped in some random place
>>>> because that saved a few lines of code.
>>>
>>> OK.  Hopefully KaiGai or Takahiro can suggest a fix.
>
> The patch has grown larger than I expected before, because the way
> to handle large objects are far from any other object classes.
>
> Here are three points:
>
> 1) The new BLOB ACLS section was added.
>
> It is a single purpose section to describe GRANT/REVOKE statements
> on large objects, and BLOB COMMENTS section was reverted to put
> only descriptions.
>
> Because we need to assume a case when the database holds massive
> number of large objects, it is not reasonable to store them using
> dumpACL(). It chains an ACL entry with the list of TOC entries,
> then, these are dumped. It means pg_dump may have to register massive
> number of large objects in the local memory space.
>
> Currently, we also store GRANT/REVOKE statements in BLOB COMMENTS
> section, but confusable. Even if pg_restore is launched with
> --no-privileges options, it cannot ignore GRANT/REVOKE statements
> on large objects. This fix enables to distinguish ACLs on large
> objects from other properties, and to handle them correctly.
>
> 2) The BLOBS section was separated for each database users.
>
> Currently, the BLOBS section does not have information about owner
> of the large objects to be restored. So, we tried to alter its
> ownership in the BLOB COMMENTS section, but incorrect.
>
> The --use-set-session-authorization option requires to restore
> ownership of objects without ALTER ... OWNER TO statements.
> So, we need to set up correct database username on the section
> properties.
>
> This patch renamed the hasBlobs() by getBlobs(), and changed its
> purpose. It registers DO_BLOBS, DO_BLOB_COMMENTS and DO_BLOB_ACLS
> for each large objects owners, if necessary.
> For example, if here are five users owning large objects, getBlobs()
> shall register five TOC entries for each users, and dumpBlobs(),
> dumpBlobComments() and dumpBlobAcls() shall be also invoked five
> times with the username.
>
> 3) _LoadBlobs()
>
> For regular database object classes, _printTocEntry() can inject
> "ALTER xxx OWNER TO ..." statement on the restored object based on
> the ownership described in the section header.
> However, we cannot use this infrastructure for large objects as-is,
> because one BLOBS section can restore multiple large objects.
>
> _LoadBlobs() is a routine to restore large objects within a certain
> section. This patch modifies this routine to inject "ALTER LARGE
> OBJECT <loid> OWNER TO <user>" statement for each large objects
> based on the ownership of the section.
> (if --use-set-session-authorization is not given.)
>
>
> $ diffstat pgsql-fix-pg_dump-blob-privs.patch
>  pg_backup_archiver.c |    4
>  pg_backup_custom.c   |   11 !
>  pg_backup_files.c    |    9 !
>  pg_backup_tar.c      |    9 !
>  pg_dump.c            |  312 +++++++----!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
>  pg_dump.h            |    9 !
>  pg_dump_sort.c       |    8 !
>  7 files changed, 68 insertions(+), 25 deletions(-), 269 modifications(!)

I will review this sooner if I have time, but please make sure it gets
added to the next CommitFest so we don't lose it. I think it also
needs to be added here, since AFAICS this is a must-fix for 8.5.

http://wiki.postgresql.org/wiki/PostgreSQL_8.5_Open_Items

Thanks,

...Robert