|From:||Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>|
|To:||Peter Eisentraut <peter(dot)eisentraut(at)enterprisedb(dot)com>|
|Cc:||David Rowley <dgrowleyml(at)gmail(dot)com>, pgsql-hackers <pgsql-hackers(at)postgresql(dot)org>|
|Subject:||Re: automatically generating node support functions|
|Views:||Raw Message | Whole Thread | Download mbox | Resend email|
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
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)
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
* 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
* 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
* 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
|Next Message||Thomas Munro||2022-07-06 20:56:33||Re: EINTR in ftruncate()|
|Previous Message||Thomas Munro||2022-07-06 20:46:26||Re: Bump MIN_WINNT to 0x0600 (Vista) as minimal runtime in 16~|