Re: refactoring comment.c

(2010/08/16 11:50), Robert Haas wrote:
> On Fri, Aug 6, 2010 at 11:15 PM, KaiGai Kohei<kaigai@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 <kaigai@ak.jp.nec.com>