Re: refactoring comment.c

Robert Haas <robertmhaas@gmail.com> writes:
> Any other kibitzing before I commit this?

Sure ...

+  * If the object is a relation or a child object of a relation (e.g. an
+  * attribute or contraint, *relp will set to point to that relation).  This

Parenthesis in the wrong place here, grammar and spelling not much better.
Also, I still feel that this comment could do better about explaining the
behavior, particularly with respect to locking.  Perhaps say

+  * If the target object is a relation or a child object of a relation
+  * (e.g. an attribute or constraint), the relation is also opened, and *relp
+  * receives the open relcache entry pointer; otherwise *relp is set to NULL.
+  * This is a bit grotty but it makes life simpler, since the caller will
+  * typically need the relcache entry too.  Caller must close the relcache
+  * entry when done with it.  The relation is locked with the specified
+  * lockmode if the target object is the relation itself or an attribute,
+  * but for other child objects, only AccessShareLock is acquired on the
+  * relation.


+ 	ScanKeyInit(&skey[0], ObjectIdAttributeNumber, BTEqualStrategyNumber,
+ 				F_OIDEQ, ObjectIdGetDatum(address.objectId));

There's a standard convention for the layout of ScanKeyInit calls, and
this isn't it.  Trivial, I know, but it's better to make similar code
look similar.

There's no longer any need for a diff in src/backend/parser/Makefile.

+ #define OBJECTADDRESS_H
+ 
+ #include "nodes/parsenodes.h"
+ #include "nodes/pg_list.h"
+ #include "storage/lock.h"
+ #include "utils/rel.h"

You shouldn't need pg_list.h here, as parsenodes.h surely includes it.

			regards, tom lane