Re: refactoring comment.c

Robert Haas <robertmhaas@gmail.com> writes:
> 5. Since I'm hoping Tom will read this, I ran it through filterdiff.  :-)

OK, I looked ;-).  It mostly looks good, but of course I've got some
opinions...

> 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?

I think the problem is you're trying to put this into backend/parser
which is not really the right place for it.  It's an execution-time
animal not a parse-time animal.  I would put it into backend/catalog,
perhaps named objectaddress.c, and similarly the header file would be
objectaddress.h.  Then it would be reasonable to move struct
ObjectAddress into this header and have dependency.h #include it.
There might be some other stuff in dependency.c that more naturally
belongs here, too.

> 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?

It's not pg_upgrade (nor psql, nor pg_dump) we are protecting here.
It's third-party applications that might understand the contents of
pg_description, pg_depend, etc.  I think that hack is quite small
and localized enough to live with, rather than causing a flag day
for an unknown number of clients by changing the representation.

+     /*
+      * Translate the parser representation which identifies this object into
+      * an ObjectAddress. get_object_address() will throw an error if the
+      * object does not exist.
+      */
+     address = get_object_address(stmt->objtype, stmt->objname, stmt->objargs,
+                                  &relation, ShareUpdateExclusiveLock);

I think this comment should also explicitly mention that we're getting a
lock to protect against concurrent DROPs.

+ /*
+  * Translate an object name and arguments (as passed by the parser) to an
+  * ObjectAddress.
+  *
+  * The returned object will be locked using the specified lockmode.  If a
+  * sub-object is looked up, the parent object will be locked instead.
+  *
+  * We don't currently provide a function to release the locks acquired here;
+  * typically, the lock must be held until commit to guard against a concurrent
+  * drop operation.
+  */
+ ObjectAddress
+ get_object_address(ObjectType objtype, List *objname, List *objargs,
+                    Relation *relp, LOCKMODE lockmode)

This comment's a bit shy of a load too, since it totally fails to
mention the relp argument, or specify what the caller is required to
do with it, or explain how the locking on the relation works.

As a matter of style, I'd suggest putting the single externally callable
function (ie get_object_address) at the top of the file not the bottom.
People shouldn't have to read through the entire file before finding out
what API it is supposed to provide to the outside world.
        regards, tom lane