Thread: Re: [HACKERS] [COMMITTERS] pgsql: Add missing support for new node fields

> Add missing support for new node fields
>
> Commit b6fb534f added two new node fields but neglected to add copy and
> comparison support for them, Mea culpa, should have checked for that.

I've been annoyed by these stupid functions and forgetting to update them
since I run into them while trying to fix an issue in pg_stat_statement
some time ago.

I've started to develop a perl script to generate most of them from
headers. It is not done yet, but it looks that it can work in the end with
limited effort. Currently it works for copy & equal.

Is there some interest to generate the x00kB of sources rather than edit
them everytime, or forgetting it from time to time, or does everyone like
it as it is?

--
Fabien.

Hi,

On 2017-03-21 07:22:57 +0100, Fabien COELHO wrote:
> > Add missing support for new node fields
> >
> > Commit b6fb534f added two new node fields but neglected to add copy and
> > comparison support for them, Mea culpa, should have checked for that.
>
> I've been annoyed by these stupid functions and forgetting to update them
> since I run into them while trying to fix an issue in pg_stat_statement some
> time ago.
>
> I've started to develop a perl script to generate most of them from headers.
> It is not done yet, but it looks that it can work in the end with limited
> effort. Currently it works for copy & equal.

It'd have to do out/read as well imo.


> Is there some interest to generate the x00kB of sources rather than edit
> them everytime, or forgetting it from time to time, or does everyone like it
> as it is?

From my POV yes.  But it's not quite as trivial as just generating it
based on fields. Some fields are intentionally skipped, e.g. location,
for equalfuncs, but not copy/out/readfuncs. So there needs to be a way
to specify such special rules.

- Andres

Andres Freund <andres@anarazel.de> writes:
> On 2017-03-21 07:22:57 +0100, Fabien COELHO wrote:
>> I've been annoyed by these stupid functions and forgetting to update them
>> since I run into them while trying to fix an issue in pg_stat_statement some
>> time ago.
>>
>> I've started to develop a perl script to generate most of them from headers.
>> It is not done yet, but it looks that it can work in the end with limited
>> effort. Currently it works for copy & equal.

> It'd have to do out/read as well imo.

Yeah.  A partial solution is pretty much useless.  Even with out/read
support as well, I'm not sure it's not useless, because you'd still
have to remember to consider places like expression_tree_walker and
expression_tree_mutator.  Those are not really amenable to automatic
generation AFAICS, because they have to understand the actual semantics
of each field.

It's conceivable that you could get somewhere if the starting point were
some marked-up representation of every node type's field list, rather than
just a C declaration.  (IOW, include/nodes/*.h would become generated
files as well.)  But really, isn't that just moving the error locale from
"forgetting to update equalfuncs.c" to "forgetting to include the correct
marker keywords"?  And people would have to learn about this generator
tool and its input language.

In the end there's very little substitute for checking every reference
to a struct type when you add/modify one of its fields.  grep is your
friend.
        regards, tom lane

On Tue, Mar 21, 2017 at 10:56 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Andres Freund <andres@anarazel.de> writes:
>> On 2017-03-21 07:22:57 +0100, Fabien COELHO wrote:
>>> I've been annoyed by these stupid functions and forgetting to update them
>>> since I run into them while trying to fix an issue in pg_stat_statement some
>>> time ago.
>>>
>>> I've started to develop a perl script to generate most of them from headers.
>>> It is not done yet, but it looks that it can work in the end with limited
>>> effort. Currently it works for copy & equal.
>
>> It'd have to do out/read as well imo.
>
> Yeah.  A partial solution is pretty much useless.  Even with out/read
> support as well, I'm not sure it's not useless, because you'd still
> have to remember to consider places like expression_tree_walker and
> expression_tree_mutator.  Those are not really amenable to automatic
> generation AFAICS, because they have to understand the actual semantics
> of each field.

Well, let's not move the goalposts into the outer solar system.  There
are plenty of changes to node structure that don't require
expression_tree_walker or expression_tree_mutator to be touched at
all.  Expression nodes aren't touched all that frequently compared to
path, plan, etc. nodes.

IMHO, what would be a lot more useful than something that generates
{read,equal,copy,out}funcs.c automatically would be something that
just checks them for trivial errors of omission.  For example, if you
read a list of structure members from the appropriate header files and
cross-checked it against the list of structure members mentioned in
the body of a copy/equal/read/outfunc, you'd catch probably 80% of the
mistakes people make right there.  If you could also check for a
copy/read/equal/outfunc that ought to have been present but was
omitted entirely, that'd probably up it to 90%.

The idea would be that if you added a field that wasn't supposed to be
copied, you'd have to add something to copyfuncs.c that said, e.g.

/* NOTCOPIED: mymember */

...and the checking script would ignore things so flagged.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Hello Andres,

>> It is not done yet, but it looks that it can work in the end with limited
>> effort. Currently it works for copy & equal.
>
> It'd have to do out/read as well imo.

Sure. This part is WIP, though.

>> Is there some interest to generate the x00kB of sources rather than edit
>> them everytime, or forgetting it from time to time, or does everyone like it
>> as it is?
>
> From my POV yes.  But it's not quite as trivial as just generating it
> based on fields. Some fields are intentionally skipped, e.g. location,
> for equalfuncs, but not copy/out/readfuncs. So there needs to be a way
> to specify such special rules.

Indeed, I noticed these particularities... For some case it can be
automated with limited input. For really special cases (eg a few read/out
functions) , the script is told to skip some node types and the special
manual version is taken from the file instead of being generated.

--
Fabien.

Hello Robert,

> IMHO, what would be a lot more useful than something that generates
> {read,equal,copy,out}funcs.c automatically would be something that
> just checks them for trivial errors of omission.

Hmmm. Checking for errors is actually more complicated than generating the
function: basically you have to generate the function, at least
implicitely, then parse the actual functions, then compare the two, then 
generate meaningful messages. Thrice the work.

> The idea would be that if you added a field that wasn't supposed to be
> copied, you'd have to add something to copyfuncs.c that said, e.g.
>
> /* NOTCOPIED: mymember */

Yep, I was thinking of maybe use directives added to header files to 
handle some special cases, but the real special cases would maybe more 
readily turned to manual to keep a simpler generation script.

I do not fancy relying on another representation/language because of Tom's 
objection that it would mean another language to learn, and I do not think 
that it is desirable in pg.

-- 
Fabien.

Another idea here:

Instead of making COPY_PARSE_PLAN_TREES a compile-time option, make it a
run-time option, and make that somehow selectable while running the test
suite.  It would be much easier to run the test suite with an option on
the command line, instead of having to manually edit a header file and
recompiling, then switching it back, etc.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services