Largeobject Access Controls (r2460)
in [PgSql]
|
Prev: explain output infelicity in psql
Next: Winflex
From: Takahiro Itagaki on 11 Dec 2009 03:48 Takahiro Itagaki <itagaki.takahiro(a)oss.ntt.co.jp> wrote: > In addition of the patch, we also need to fix pg_restore with > --clean option. I added DropBlobIfExists() in pg_backup_db.c. > > A revised patch attached. Please check further mistakes. ....and here is an additional fix for contrib modules. Regards, --- Takahiro Itagaki NTT Open Source Software Center
From: Bruce Momjian on 11 Dec 2009 10:39 KaiGai Kohei wrote: > Takahiro Itagaki wrote: > > KaiGai Kohei <kaigai(a)ak.jp.nec.com> wrote: > > > >> Tom Lane wrote: > >>> Takahiro Itagaki <itagaki.takahiro(a)oss.ntt.co.jp> writes: > >>>> <structname>pg_largeobject</structname> should not be readable by the > >>>> public, since the catalog contains data in large objects of all users. > >>> This is going to be a problem, because it will break applications that > >>> expect to be able to read pg_largeobject. Like, say, pg_dump. > >> Is it a right behavior, even if we have permission checks on large objects? > > > > Can we use column-level access control here? > > > > REVOKE ALL ON pg_largeobject FROM PUBLIC; > > => GRANT SELECT (loid) ON pg_largeobject TO PUBLIC; > > Indeed, it seems to me reasonable. > > > We use "SELECT loid FROM pg_largeobject LIMIT 1" in pg_dump. We could > > replace pg_largeobject_metadata instead if we try to fix only pg_dump, > > but it's no wonder that any other user applications use such queries. > > I think to allow reading loid is a balanced solution. > > Right, I also remind this query has to be fixed up by other reason right now. > If all the large objects are empty, this query can return nothing, even if > large object entries are in pg_largeobject_metadata. "metadata" seems very vague. Can't we come up with a more descriptive name? Also, how will this affect pg_migrator? pg_migrator copies pg_largeobject and its index from the old to the new server. Is the format inside pg_largeobject changed by this patch? What happens when there is no entry in pg_largeobject_metadata for a specific row? -- Bruce Momjian <bruce(a)momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + If your life is a hard drive, Christ can be your backup. + -- 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: Bruce Momjian on 11 Dec 2009 11:42 KaiGai Kohei wrote: > >>> We use "SELECT loid FROM pg_largeobject LIMIT 1" in pg_dump. We could > >>> replace pg_largeobject_metadata instead if we try to fix only pg_dump, > >>> but it's no wonder that any other user applications use such queries. > >>> I think to allow reading loid is a balanced solution. > >> Right, I also remind this query has to be fixed up by other reason right now. > >> If all the large objects are empty, this query can return nothing, even if > >> large object entries are in pg_largeobject_metadata. > > > > "metadata" seems very vague. Can't we come up with a more descriptive > > name? > > What about "property"? > The "metadata" was the suggested name from Robert Haas at the last > commit fest, because we may store any other properties of a large > object in this catalog future. Well, we usually try to be more specific about what something represents and only later abstract it out if needed, but if everyone else is fine with 'metadata', then just leave it unchanged. > > Also, how will this affect pg_migrator? pg_migrator copies > > pg_largeobject and its index from the old to the new server. Is the > > format inside pg_largeobject changed by this patch? > > The format of pg_largeobject was not touched. Good. > > What happens when > > there is no entry in pg_largeobject_metadata for a specific row? > > In this case, these rows become orphan. > So, I think we need to create an empty large object with same LOID on > pg_migrator. It makes an entry on pg_largeobject_metadata without > writing anything to the pg_largeobject. > I guess rest of migrations are not difference. Correct? Uh, yea, pg_migrator could do that pretty easily. -- Bruce Momjian <bruce(a)momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + If your life is a hard drive, Christ can be your backup. + -- 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 11 Dec 2009 10:59 Bruce Momjian さんは書きました: > KaiGai Kohei wrote: >> Takahiro Itagaki wrote: >>> KaiGai Kohei <kaigai(a)ak.jp.nec.com> wrote: >>> >>>> Tom Lane wrote: >>>>> Takahiro Itagaki <itagaki.takahiro(a)oss.ntt.co.jp> writes: >>>>>> <structname>pg_largeobject</structname> should not be readable by the >>>>>> public, since the catalog contains data in large objects of all users. >>>>> This is going to be a problem, because it will break applications that >>>>> expect to be able to read pg_largeobject. Like, say, pg_dump. >>>> Is it a right behavior, even if we have permission checks on large objects? >>> Can we use column-level access control here? >>> >>> REVOKE ALL ON pg_largeobject FROM PUBLIC; >>> => GRANT SELECT (loid) ON pg_largeobject TO PUBLIC; >> Indeed, it seems to me reasonable. >> >>> We use "SELECT loid FROM pg_largeobject LIMIT 1" in pg_dump. We could >>> replace pg_largeobject_metadata instead if we try to fix only pg_dump, >>> but it's no wonder that any other user applications use such queries. >>> I think to allow reading loid is a balanced solution. >> Right, I also remind this query has to be fixed up by other reason right now. >> If all the large objects are empty, this query can return nothing, even if >> large object entries are in pg_largeobject_metadata. > > "metadata" seems very vague. Can't we come up with a more descriptive > name? What about "property"? The "metadata" was the suggested name from Robert Haas at the last commit fest, because we may store any other properties of a large object in this catalog future. > Also, how will this affect pg_migrator? pg_migrator copies > pg_largeobject and its index from the old to the new server. Is the > format inside pg_largeobject changed by this patch? The format of pg_largeobject was not touched. > What happens when > there is no entry in pg_largeobject_metadata for a specific row? In this case, these rows become orphan. So, I think we need to create an empty large object with same LOID on pg_migrator. It makes an entry on pg_largeobject_metadata without writing anything to the pg_largeobject. I guess rest of migrations are not difference. Correct? Thanks, -- 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 11 Dec 2009 23:04
Takahiro Itagaki wrote: > KaiGai Kohei <kaigai(a)ak.jp.nec.com> wrote: > >>>> We have to reference pg_largeobject_metadata to check whether a certain >>>> large objct exists, or not. >> It is a case when we create a new large object, but write nothing. > > OK, that makes sense. > > In addition of the patch, we also need to fix pg_restore with > --clean option. I added DropBlobIfExists() in pg_backup_db.c. > > A revised patch attached. Please check further mistakes. + void + DropBlobIfExists(ArchiveHandle *AH, Oid oid) + { + const char *lo_relname; + const char *lo_colname; + + if (PQserverVersion(AH->connection) >= 80500) + { + lo_relname = "pg_largeobject_metadata"; + lo_colname = "oid"; + } + else + { + lo_relname = "pg_largeobject"; + lo_colname = "loid"; + } + + /* Call lo_unlink only if exists to avoid not-found error. */ + ahprintf(AH, "SELECT CASE WHEN EXISTS(SELECT 1 FROM pg_catalog.%s WHERE %s = '%u') THEN pg_catalog.lo_unlink('%u') END;\n", + lo_relname, lo_colname, oid, oid); + } I think the following approach is more reasonable for the current design. if (PQserverVersion(AH->connection) >= 80500) { /* newer query */ ahprintf(AH, "SELECT pg_catalog.lo_unlink(oid) " "FROM pg_catalog.pg_largeobject_metadata " "WHERE oid = %u;\n", oid); } else { /* original query */ ahprintf(AH, "SELECT CASE WHEN EXISTS(SELECT 1 FROM pg_catalog.pg_largeobject WHERE loid = '%u') " "THEN pg_catalog.lo_unlink('%u') END;\n", oid, oid); } We don't have any reason why still CASE ... WHEN and subquery for the given LOID. Right? The fix-lo-contrib.patch looks good for me. > BTW, we can optimize lo_truncate because we allow metadata-only large > objects. inv_truncate() doesn't have to update the first data tuple to > be zero length. It only has to delete all corresponding tuples like as: > DELETE FROM pg_largeobject WHERE loid = {obj_desc->id} Right, when inv_truncate takes an aligned length by LOBLKSIZE. I'll also submit a small patch on CF-Jan, OK? Thanks, -- KaiGai Kohei <kaigai(a)kaigai.gr.jp> -- Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers |