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.

--
Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-committers

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

--
Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-committers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

+1

The work on nodes maintenance is not too much, but some smart check can be
nice.

Regards

Pavel

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.

--
Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-committers

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.

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers