Largeobject Access Controls (r2460)
in [PgSql]
|
Prev: explain output infelicity in psql
Next: Winflex
From: Takahiro Itagaki on 18 Dec 2009 01:48 Robert Haas <robertmhaas(a)gmail.com> wrote: > In both cases, I'm lost. Help? They might be contrasted with the comments for myLargeObjectExists. Since we use MVCC visibility in loread(), metadata for large object also should be visible in MVCC rule. If I understand them, they say: * pg_largeobject_aclmask_snapshot requires a snapshot which will be used in loread(). * Don't use LargeObjectExists if you need MVCC visibility. > In acldefault(), there is this comment: > /* Grant SELECT,UPDATE by default, for now */ > This doesn't seem to match what the code is doing, so I think we > should remove it. Ah, ACL_NO_RIGHTS is the default. > I also notice that dumpBlobComments() is now misnamed, but it seems > we've chosen to add a comment mentioning that fact rather than fixing it. Hmmm, now it dumps not only comments but also ownership of large objects. Should we rename it dumpBlobMetadata() or so? 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
From: KaiGai Kohei on 18 Dec 2009 03:44 (2009/12/18 15:48), Takahiro Itagaki wrote: > > Robert Haas<robertmhaas(a)gmail.com> wrote: > >> In both cases, I'm lost. Help? > > They might be contrasted with the comments for myLargeObjectExists. > Since we use MVCC visibility in loread(), metadata for large object > also should be visible in MVCC rule. > > If I understand them, they say: > * pg_largeobject_aclmask_snapshot requires a snapshot which will be > used in loread(). > * Don't use LargeObjectExists if you need MVCC visibility. Yes, correct. >> In acldefault(), there is this comment: >> /* Grant SELECT,UPDATE by default, for now */ >> This doesn't seem to match what the code is doing, so I think we >> should remove it. > > Ah, ACL_NO_RIGHTS is the default. Oops, it reflects very early phase design, but fixed later. >> I also notice that dumpBlobComments() is now misnamed, but it seems >> we've chosen to add a comment mentioning that fact rather than fixing it. > > Hmmm, now it dumps not only comments but also ownership of large objects. > Should we rename it dumpBlobMetadata() or so? It seems to me quite natural. The attached patch fixes them. Thanks, -- OSS Platform Development Division, NEC KaiGai Kohei <kaigai(a)ak.jp.nec.com>
From: Robert Haas on 18 Dec 2009 09:00 2009/12/18 KaiGai Kohei <kaigai(a)ak.jp.nec.com>: > (2009/12/18 15:48), Takahiro Itagaki wrote: >> >> Robert Haas<robertmhaas(a)gmail.com> wrote: >> >>> In both cases, I'm lost. Help? >> >> They might be contrasted with the comments for myLargeObjectExists. >> Since we use MVCC visibility in loread(), metadata for large object >> also should be visible in MVCC rule. >> >> If I understand them, they say: >> * pg_largeobject_aclmask_snapshot requires a snapshot which will be >> used in loread(). >> * Don't use LargeObjectExists if you need MVCC visibility. > > Yes, correct. > >>> In acldefault(), there is this comment: >>> /* Grant SELECT,UPDATE by default, for now */ >>> This doesn't seem to match what the code is doing, so I think we >>> should remove it. >> >> Ah, ACL_NO_RIGHTS is the default. > > Oops, it reflects very early phase design, but fixed later. > >>> I also notice that dumpBlobComments() is now misnamed, but it seems >>> we've chosen to add a comment mentioning that fact rather than fixing it. >> >> Hmmm, now it dumps not only comments but also ownership of large objects. >> Should we rename it dumpBlobMetadata() or so? > > It seems to me quite natural. > > The attached patch fixes them. I think we might want to go with dumpBlobProperties(), because dumpBlobMetadata() might lead you to think that all of the properties being dumped are stored in pg_largeobject_metadata, which is not the case. I do also wonder why we are calling these blobs in this code rather than large objects, but that problem predates this patch and I think we might as well leave it alone for now. ....Robert -- 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: Robert Haas on 18 Dec 2009 20:55 On Fri, Dec 18, 2009 at 9:00 AM, Robert Haas <robertmhaas(a)gmail.com> wrote: > 2009/12/18 KaiGai Kohei <kaigai(a)ak.jp.nec.com>: >> (2009/12/18 15:48), Takahiro Itagaki wrote: >>> >>> Robert Haas<robertmhaas(a)gmail.com> wrote: >>> >>>> In both cases, I'm lost. Help? >>> >>> They might be contrasted with the comments for myLargeObjectExists. >>> Since we use MVCC visibility in loread(), metadata for large object >>> also should be visible in MVCC rule. >>> >>> If I understand them, they say: >>> * pg_largeobject_aclmask_snapshot requires a snapshot which will be >>> used in loread(). >>> * Don't use LargeObjectExists if you need MVCC visibility. >> >> Yes, correct. >> >>>> In acldefault(), there is this comment: >>>> /* Grant SELECT,UPDATE by default, for now */ >>>> This doesn't seem to match what the code is doing, so I think we >>>> should remove it. >>> >>> Ah, ACL_NO_RIGHTS is the default. >> >> Oops, it reflects very early phase design, but fixed later. >> >>>> I also notice that dumpBlobComments() is now misnamed, but it seems >>>> we've chosen to add a comment mentioning that fact rather than fixing it. >>> >>> Hmmm, now it dumps not only comments but also ownership of large objects. >>> Should we rename it dumpBlobMetadata() or so? >> >> It seems to me quite natural. >> >> The attached patch fixes them. > > I think we might want to go with dumpBlobProperties(), because > dumpBlobMetadata() might lead you to think that all of the properties > being dumped are stored in pg_largeobject_metadata, which is not the > case. 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? ....Robert -- 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: Robert Haas on 18 Dec 2009 20:58
On Fri, Dec 18, 2009 at 1:48 AM, Takahiro Itagaki <itagaki.takahiro(a)oss.ntt.co.jp> wrote: >> In both cases, I'm lost. Help? > > They might be contrasted with the comments for myLargeObjectExists. > Since we use MVCC visibility in loread(), metadata for large object > also should be visible in MVCC rule. > > If I understand them, they say: > * pg_largeobject_aclmask_snapshot requires a snapshot which will be > used in loread(). > * Don't use LargeObjectExists if you need MVCC visibility. Part of what I'm confused about (and what I think should be documented in a comment somewhere) is why we're using MVCC visibility in some places but not others. In particular, there seem to be some bits of the comment that imply that we do this for read but not for write, which seems really strange. It may or may not actually be strange, but I don't understand it. ....Robert -- Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers |