Thread: Another refactoring proposal: move stuff into nodes/nodeFuncs.[ch]

So I was starting to implement an exprLocation() function according to
previous discussion
http://archives.postgresql.org/pgsql-hackers/2008-08/msg01131.php
and I wondered where to put it.  One idea is next to exprType() in
parser/parse_expr.c, but that seems a bit unsatisfactory because it's
likely to eventually be used in many subsystems besides parser/.
So this resurrected a bee that's been in my bonnet for awhile: the
backend has a number of widely-used functions that provide general-purpose
functionality on node trees, but are scattered around in random places
depending on where the need arose first.  Some examples
areexprType()exprTypmod()expression_tree_walker()expression_tree_mutator()query_tree_walker()query_tree_mutator()
ISTM all of these belong in a central location under backend/nodes.
You could make weaker cases for some other functions like
contain_vars_of_level(), but these ones are definitely widely used
and pretty general-purpose.

The advantages of doing this would be (a) reduce the number of places
to look in when implementing a new node type; (b) eliminate some
cross-subsystem #inclusions that weaken modularity of the backend.

What I'm thinking of doing is migrating these functions into
nodes/nodeFuncs.h and nodes/nodeFuncs.c, which are existing files that
contain almost nothing useful (I think the functions that are there
now are dead or nearly so).

Any objections?  Any nominees for additional functions to put there?
        regards, tom lane

On Mon, 2008-08-25 at 11:24 -0400, Tom Lane wrote:

> The advantages of doing this would be (a) reduce the number of places
> to look in when implementing a new node type; (b) eliminate some
> cross-subsystem #inclusions that weaken modularity of the backend.

Are we doing either of those things in this release?

Might these changes screw up patches already in progress? Can we hold
off making these changes until we're sure the latter isn't true?

-- Simon Riggs           www.2ndQuadrant.comPostgreSQL Training, Services and Support

Simon Riggs <simon@2ndQuadrant.com> writes:
> On Mon, 2008-08-25 at 11:24 -0400, Tom Lane wrote:
>> The advantages of doing this would be (a) reduce the number of places
>> to look in when implementing a new node type; (b) eliminate some
>> cross-subsystem #inclusions that weaken modularity of the backend.

> Are we doing either of those things in this release?

Yes, it's done already.

> Might these changes screw up patches already in progress? Can we hold
> off making these changes until we're sure the latter isn't true?

Some patches might need small adjustments (to find the code in a
different file) but that does not strike me as an argument for not
changing things.  We have applied far more invasive patches in the past
and undoubtedly will do so again in future.  In any case, it's still a
long way until beta freeze, so there's plenty of time to deal with
fallout.  I don't see that some other time would be better.
        regards, tom lane