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

On Mon, Feb 6, 2017 at 11:10 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Ashutosh Bapat <ashutosh(dot)bapat(at)enterprisedb(dot)com> writes:
>> On Mon, Feb 6, 2017 at 10:34 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>> 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.
>
> Maybe. There's certainly something to be said for developing a different
> style in which we only initialize fields that need to be nonzero ... but
> if we were to go for that, relnode.c would not be the only place to fix,
> or even the best place to start.
>

As against any other Node structure, RelOptInfo somehow stood out
clearly for me in this aspect. May be because it's a large structure.
But yes, this might be a problem with other structures as well.

> Also, grepping for makeNode(FooStruct) works for cases where FooStructs
> are *only* built that way. But there's more than one place in the backend
> where we build structs in other ways, typically by declaring one as a
> local variable. It would take some effort to define a universal
> convention here and make sure all existing code follows it.
>

I think only makeNode() or palloc0(... * sizeof(nodename)) usages can
benefit from this. In other cases the fields need to be explicitly
initialized. Also, that would be a lot of code churn. May be we should
restrict to some large Node structures like RelOptInfo.

>> Should we at least move those assignments into a separate function?
>
> As far as relnode.c goes, I don't really think that's an improvement,
> because it complicates dealing with fields that need to be initialized
> differently for baserels and joinrels.
>

I am thinking more on the lines of makeVar() - create a function
makeRelOptInfo() or such, which accepts values for fields which need
assignment other than zero value and call it from build_join_rel() and
build_simple_rel() passing joinrel and base rel specific values resp.

--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company