Re: refactoring comment.c

Robert Haas <robertmhaas(at)gmail(dot)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