Re: refactoring comment.c

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