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

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.

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.

> 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.

regards, tom lane