Re: 0/NULL/NIL assignments in build_*_rel()

On Mon, Feb 6, 2017 at 10:34 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Ashutosh Bapat <ashutosh(dot)bapat(at)enterprisedb(dot)com> writes:
>> Both build_simple_rel() and build_join_rel() allocate RelOptInfo using
>> makeNode(), which returned a zeroed out memory. The functions then
>> assign values like false, NULL, 0 or NIL which essentially contain
>> zero valued bytes. This looks like needless work. So, we are spending
>> some CPU cycles unnecessarily in those assignments. That may not be
>> much time wasted, but whenever someone adds a field to RelOptInfo,
>> those functions need to be updated with possibly a zero value
>> assignment. That looks like an unnecessary maintenance burden. Should
>> we just drop all those zero value assignments from there?
>
> I'd vote for not. The general programming style in the backend is to
> ignore the fact that makeNode() zeroes the node's storage and initialize
> all the fields explicitly. The primary advantage of that, IMO, is that
> you can grep for references to a particular field and understand
> everyplace that affects it. As an example of the value, if you want to
> add a field X and you try to figure out what you need to modify by
> grepping for references to existing field Y, with this proposal you would
> miss places that were node initializations unless Y *always* needed to be
> initialized nonzero.

In the case of adding a new field, I would rather rely on where the
structure itself is used rather than another member. For example while
adding a field to RelOptInfo, to find out places to initialize it, I
would grep for makeNode(RelOptInfo) or such to find where a new node
gets allocated, rather than say relids. Grepping for usages of a
particular field might find zero valued assignments useful, but not
necessarily worth maintaining that code.

>
> I could be convinced that it is a good idea to depart from this theory
> in places where it makes a significant performance difference ... but
> you've not offered any reason to think relnode.c is one.
>

I don't think that will bring any measurable performance improvement,
unless we start creating millions of RelOptInfos in a query. My real
pain is

>> whenever someone adds a field to RelOptInfo,
>> those functions need to be updated with possibly a zero value
>> assignment. That looks like an unnecessary maintenance burden.

Should we at least move those assignments into a separate function?
--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company