Re: automatically generating node support functions

Peter Eisentraut <peter(dot)eisentraut(at)enterprisedb(dot)com> writes:
> [ v7-0001-Automatically-generate-node-support-functions.patch ]

I have gone through this and made some proposed changes (attached),
and I think it is almost committable. There is one nasty problem
we need a solution to, which is that pgindent is not at all on board
with this idea of attaching node attrs to typedefs. It pushes them
to the next line, like this:

@@ -691,7 +709,8 @@
(rel)->reloptkind == RELOPT_OTHER_JOINREL || \
(rel)->reloptkind == RELOPT_OTHER_UPPER_REL)

-typedef struct RelOptInfo pg_node_attr(no_copy_equal, no_read)
+typedef struct RelOptInfo
+pg_node_attr(no_copy_equal, no_read)
{
NodeTag type;

which is already enough to break the simplistic parsing in
gen_node_support.pl. Now, we could fix that parsing logic to deal
with this layout, but this also seems to change pgindent's opinion
of whether the subsequent braced material is part of a typedef or a
function. That results in it injecting a lot of vertical space
that wasn't there before, which is annoying.

I experimented a bit and found that we could do it this way:

typedef struct RelOptInfo
{
+ pg_node_attr(no_copy_equal, no_read)
+
NodeTag type;

without (AFAICT) confusing pgindent, but I've not tried to adapt
the perl script or the code to that style.

Anyway, besides that, I have some comments that I've implemented
in the attached delta patch.

* After further thought I'm okay with your theory that attaching
special copy/equal rules to specific field types is appropriate.
We might at some point want the pg_node_attr(copy_as_scalar)
approach too, but we can always add that later. However, I thought
some more comments about it were needed in the *nodes.h files,
so I added those. (My general feeling about this is that if
anyone needs to look into gen_node_support.pl to understand how
the backend works, we've failed at documentation.)

* As written, the patch created equal() support for all Plan structs,
which is quite a bit of useless code bloat. I solved this by
separating no_copy and no_equal properties, so that we could mark
Plan as no_equal while still having copy support.

* I did not like the semantics of copy_ignore one bit: it was
relying on the pre-zeroing behavior of makeNode() to be sane at
all, and I don't want that to be a requirement. (I foresee
wanting to flat-copy node contents and turn COPY_SCALAR_FIELD
into a no-op.) I replaced it with copy_as(VALUE) to provide
better-defined semantics.

* Likewise, read_write_ignore left the contents of the field after
reading too squishy for me. I invented read_as(VALUE) parallel
to copy_as() to fix the semantics, and added a check that you
can only use read_write_ignore if the struct is no_read or
you provide read_as(). (This could be factored differently
of course.)

* I threw in a bunch more no_read markers to bring the readfuncs.c
contents into closer alignment with what we have today. Maybe
there is an argument for accepting that code bloat, but it's a
discussion to have later. In any case, most of the pathnodes.h
structs HAVE to be marked no_read because there's no sane way
to reconstruct them from outfuncs output.

* I got rid of the code that stripped underscores from outfuncs
struct labels. That seemed like an entirely unnecessary
behavioral change.

* FWIW, I'm okay with the question about

# XXX Previously, for subtyping, only the leaf field name is
# used. Ponder whether we want to keep it that way.

I thought that it might make the output too cluttered, but after
some study of the results from printing plans and planner data
structures, it's not a big addition, and indeed I kind of like it.

* Fixed a bug in write_only_req_outer code.

* Made Plan and Join into abstract nodes.

Anyway, if we can fix the impedance mismatch with pgindent,
I think this is committable. There is a lot of follow-on
work that could be considered, but I'd like to get the present
changes in place ASAP so that other patches can be rebased
onto something stable.

I've attached a delta patch, and also repeated v7 so as not
to confuse the cfbot.

regards, tom lane