(2010/08/16 11:50), Robert Haas wrote: > On Fri, Aug 6, 2010 at 11:15 PM, KaiGai Kohei<kai...@kaigai.gr.jp> wrote: >> [brief review] > > OK, here's an updated patch: > > 1. I fixed the typo Alvaro spotted. > > 2. I haven't done anything about moving the definition of > ObjectAddress elsewhere, as Alvaro suggested, because I'm not sure > quite where it ought to go. I still think it's a good idea, though > I'm not dead set on it, either. Suggestions? > > 3. I fixed the issue Kaigai Kohei spotted, regarding > LargeObjectRelationId vs. LargeObjectMetadataRelationId, by adding a > grotty hack. However, I feel that I'm not so much adding a new grotty > hack as working around an existing grotty hack which was added for > reasons I'm unclear on. Is there a pg_upgrade-related reason not to > revert the original hack instead? >
When we were developing largeobject access controls, Tom Lane commented as follows: * Re: [HACKERS] [PATCH] Largeobject access controls http://marc.info/?l=pgsql-hackers&m=125548822906571&w=2 | 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. He concerned about existing applications which have knowledge about internal layout of system catalogs, then I fixed up the patch according to the suggestion. > 4. In response to Kaigai Kohei's complaint about lockmode possibly > being NoLock, I've just added an Assert() that it isn't, in lieu of > trying to do something sensible in that case. I can't at present > think of a situation in which being able to call it that way would be > useful, and the Assert() seems like it ought to be enough warning to > anyone coming along later that they'd better think twice before > thinking that will work. > > 5. Since I'm hoping Tom will read this, I ran it through filterdiff. :-) > -- KaiGai Kohei <kai...@ak.jp.nec.com> -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
