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

From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: Ashutosh Bapat <ashutosh(dot)bapat(at)enterprisedb(dot)com>
Cc: pgsql-hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: 0/NULL/NIL assignments in build_*_rel()
Date: 2017-02-06 05:04:40
Message-ID: 8335.1486357480@sss.pgh.pa.us
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

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.

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.

regards, tom lane

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Ashutosh Bapat 2017-02-06 05:27:55 Re: 0/NULL/NIL assignments in build_*_rel()
Previous Message Ashutosh Sharma 2017-02-06 05:02:03 Re: [COMMITTERS] pgsql: pageinspect: Try to fix some bugs in previous commit.