Largeobject Access Controls (r2460)
in [PgSql]
|
Prev: [HACKERS] is any reason why we cannot cast from record (row) to typed row?
Next: [HACKERS] 8.5 vs. 9.0
From: KaiGai Kohei on 5 Feb 2010 01:57 (2010/02/05 13:53), Takahiro Itagaki wrote: > > KaiGai Kohei<kaigai(a)kaigai.gr.jp> wrote: > >>> default: both contents and metadata >>> --data-only: same >>> --schema-only: neither >> >> However, it means only large object performs an exceptional object class >> that dumps its owner, acl and comment even if --data-only is given. >> Is it really what you suggested, isn't it? > > I wonder we still need to have both "BLOB ITEM" and "BLOB DATA" > even if we will take the all-or-nothing behavior. Can we handle > BLOB's owner, acl, comment and data with one entry kind? I looked at the corresponding code. Currently, we have three _LoadBlobs() variations in pg_backup_tar.c, pg_backup_files.c and pg_backup_custom.c. In the _tar.c and _files.c case, we can reasonably fetch data contents of the blob to be restored. All we need to do is to provide an explicit filename to the tarOpen() function, and a blob is not necessary to be restored sequentially. It means pg_restore can restore an arbitrary file when it found a new unified blob entry. In the _custom.c case, its _LoadBlobs() is called from _PrintTocData() when the given TocEntry is "BLOBS", and it tries to load the following multiple blobs. However, I could not find any restriction that custom format cannot have multiple "BLOBS" section. In other word, we can write out multiple sections with a blob for each a new unified blob entry. Right now, it seems to me it is feasible to implement what you suggested. The matter is whether we should do it, or not. At least, it seems to me better than some of exceptional treatments in pg_dump and pg_restore from the perspective of design. What is your opinion? Thanks, -- OSS Platform Development Division, NEC KaiGai Kohei <kaigai(a)ak.jp.nec.com> -- Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
From: Alvaro Herrera on 8 Feb 2010 08:23 Takahiro Itagaki escribi�: > > KaiGai Kohei <kaigai(a)kaigai.gr.jp> wrote: > > > > default: both contents and metadata > > > --data-only: same > > > --schema-only: neither > > > > However, it means only large object performs an exceptional object class > > that dumps its owner, acl and comment even if --data-only is given. > > Is it really what you suggested, isn't it? > > I wonder we still need to have both "BLOB ITEM" and "BLOB DATA" > even if we will take the all-or-nothing behavior. Can we handle > BLOB's owner, acl, comment and data with one entry kind? I don't think this is necessarily a good idea. We might decide to treat both things separately in the future and it having them represented separately in the dump would prove useful. -- Alvaro Herrera http://www.CommandPrompt.com/ The PostgreSQL Company - Command Prompt, Inc. -- Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
From: KaiGai Kohei on 8 Feb 2010 00:17 (2010/02/05 13:53), Takahiro Itagaki wrote: > > KaiGai Kohei<kaigai(a)kaigai.gr.jp> wrote: > >>> default: both contents and metadata >>> --data-only: same >>> --schema-only: neither >> >> However, it means only large object performs an exceptional object class >> that dumps its owner, acl and comment even if --data-only is given. >> Is it really what you suggested, isn't it? > > I wonder we still need to have both "BLOB ITEM" and "BLOB DATA" > even if we will take the all-or-nothing behavior. Can we handle > BLOB's owner, acl, comment and data with one entry kind? The attached patch was a refactored one according to the suggestion. In the default or --data-only, it dumps data contents of large objects and its properties (owner, comment and access privileges), but it dumps nothing when --schema-only is given. default: both contents and metadata --data-only: same --schema-only: neither It replaced existing "BLOBS" and "BLOB COMMENTS" sections by the new "LARGE OBJECT" section which is associated with a certain large object. Its section header contains OID of the large object to be restored, so the pg_restore tries to load the specified large object from the given archive. _PrintTocData() handlers were modified to support the "LARGE OBJECT" section that loads the specified large object only, not whole of the archived ones like "BLOBS". It also support to read "BLOBS" and "BLOB COMMENTS" sections, but never write out these legacy sections any more. The archive file will never contain "blobs.toc", because we can find OID of the large objects to be restored in the section header, without any special purpose files. It also allows to omit _StartBlobs() and _EndBlobs() method in tar and file format. Basically, I like this approach more than the previous combination of "BLOB ITEM" and "BLOB DATA". However, we have a known issue here. The ACL section is categorized to REQ_SCHEMA in _tocEntryRequired(), so we cannot dump them when --data-only options, even if large object itself is dumped out. Of course, we can solve it with putting a few more exceptional treatments, although it is not graceful. However, it seems to me the matter comes from that _tocEntryRequired() can only returns a mask of REQ_SCHEMA and REQ_DATA. Right now, it is not easy to categorize ACL/COMMENT section into either of them. I think we should consider REQ_ACL and REQ_COMMENT to inform caller whether the appeared section to be dumped now, or not. Any idea? Thanks, -- OSS Platform Development Division, NEC KaiGai Kohei <kaigai(a)ak.jp.nec.com>
From: KaiGai Kohei on 9 Feb 2010 00:03 (2010/02/08 22:23), Alvaro Herrera wrote: > Takahiro Itagaki escribió: >> >> KaiGai Kohei<kaigai(a)kaigai.gr.jp> wrote: >> >>>> default: both contents and metadata >>>> --data-only: same >>>> --schema-only: neither >>> >>> However, it means only large object performs an exceptional object class >>> that dumps its owner, acl and comment even if --data-only is given. >>> Is it really what you suggested, isn't it? >> >> I wonder we still need to have both "BLOB ITEM" and "BLOB DATA" >> even if we will take the all-or-nothing behavior. Can we handle >> BLOB's owner, acl, comment and data with one entry kind? > > I don't think this is necessarily a good idea. We might decide to treat > both things separately in the future and it having them represented > separately in the dump would prove useful. I agree. From design perspective, the single section approach is more simple than dual section, but its change set is larger than the dual. The attached patch revised the previous implementation which have two types of sections, to handle options correctly, as follows: * default: both contents and metadata * --data-only: same * --schema-only: neither Below is the points to be introduced. The _tocEntryRequired() makes its decision whether the given TocEntry to be dumped here, or not, based on the given context. Previously, all the sections which needs cleanups and access privileges were not belonging to SECTION_DATA, so, data sections were ignored, even if it needs to restore cleanup code and access privileges. At the pg_backup_archiver.c:329 chunk, it checks whether we need to clean up the existing object specified by the TocEntry. If the entry is "BLOB ITEM", _tocEntryRequired() returns REQ_DATA (if --data-only given), then it does not write out the cleanup code. (We have to unlink the existing large objects to be restored prior to creation of them, so it got unavailable to clean up at _StartBlob().) At the pg_backup_archiver.c:381 chunk, it checks whether we need to restore access privileges, or not, using the given "ACL" TocEntry. In similar way, the caller does not expect access privileges being restored when --data-only is given. The _tocEntryRequired() was also modified to handle large objects correctly. Previously, when TocEntry does not have its own dumper (except for "SEQUENCE SET" section), it was handled as a SECTION_SCHEMA. If the 16th argument of ArchiveEntry() was NULL, it does not have its own dumper function, even if the section is SECTION_DATA. Also, the dumpBlobItem() calls ArchiveEntry() without its dumper, so it is misidentified as a schema section. The "ACL" section of large objects are also misidentified. So, I had to add these special treatments. The dumpACL() is a utility function to write out GRANT/REVOKE statement for the given acl string. When --data-only is given, it returns immediately without any works. It prevented to dump access privileges of large objects. However, all the caller already checks "if (dataOnly)" cases prior to its invocation. So, I removed this check from the dumpACL(). Thanks, -- OSS Platform Development Division, NEC KaiGai Kohei <kaigai(a)ak.jp.nec.com>
From: Takahiro Itagaki on 9 Feb 2010 06:16
KaiGai Kohei <kaigai(a)ak.jp.nec.com> wrote: > > I don't think this is necessarily a good idea. We might decide to treat > > both things separately in the future and it having them represented > > separately in the dump would prove useful. > > I agree. From design perspective, the single section approach is more > simple than dual section, but its change set is larger than the dual. OK. When I tested a custom dump with pg_restore, --clean & --single-transaction will fail with the new dump format because it always call lo_unlink() even if the large object doesn't exist. It comes from dumpBlobItem: ! dumpBlobItem(Archive *AH, BlobInfo *binfo) ! appendPQExpBuffer(dquery, "SELECT lo_unlink(%s);\n", binfo->dobj.name); The query in DropBlobIfExists() could avoid errors -- should we use it here? | SELECT lo_unlink(oid) FROM pg_largeobject_metadata WHERE oid = %s; BTW, --clean option is ambiguous if combined with --data-only. Restoring large objects fails for the above reason if previous objects don't exist, but table data are restored *without* truncation of existing data. Will normal users expect TRUNCATE-before-load for --clean & --data-only cases? Present behaviors are; Table data - Appended. (--clean is ignored) Large objects - End with an error if object doesn't exist. IMO, ideal behaviors are: Table data - Truncate existing data and load new ones. Large objects - Work like as MERGE (or REPLACE, UPSERT). Comments? Regards, --- Takahiro Itagaki NTT Open Source Software Center -- Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers |