Re: Convert planner's AggInfo and AggTransInfo to Nodes

Peter Eisentraut <peter(dot)eisentraut(at)enterprisedb(dot)com> writes:
> On 18.07.22 18:08, Tom Lane wrote:
>> I'm kind of tempted to mount an effort to get rid of as many of
>> pathnodes.h's "read_write_ignore" annotations as possible. Some are
>> necessary to prevent infinite recursion, and others represent considered
>> judgments that they'd bloat node dumps more than they're worth --- but
>> I think quite a lot of them arose from plain laziness about updating
>> outfuncs.c. With the infrastructure we have now, that's no longer
>> a good reason.

> That was my impression as well, and I agree it would be good to sort
> that out.

I had a go at doing this, and ended up with something that seems
reasonable for now (attached). The thing that'd have to be done to
make additional progress is to convert a lot of partitioning-related
structs into full Nodes. That seems like it might possibly be
worth doing, but I don't feel like doing it. I doubt that making
planner node dumps smarter is a sufficient excuse for that anyway.
(But possibly if we then larded related code with castNode() and
sibling macros, there'd be enough of a gain in type-safety to
justify it?)

I learned a couple of interesting things along the way:

* I'd thought we already had outfuncs support for writing an array
of node pointers. We don't, but it's easily added. I chose to
write the array with parenthesis decoration, mainly because that
eases moving around it in emacs.

* WRITE_OID_ARRAY and WRITE_BOOL_ARRAY needed extension to handle a null
array pointer. I think we should make all the WRITE_FOO_ARRAY macros
work alike, so I added that to all of them. I first tried to make the
rest work like WRITE_INDEX_ARRAY, but that failed because readfuncs.c
isn't expecting "<>" for an empty array; it's expecting nothing at
all. (Note there is no readfuncs equivalent to WRITE_INDEX_ARRAY.)
What I've done here is to change WRITE_INDEX_ARRAY to work like the
others and print nothing for an empty array, but I wonder if now
wouldn't be a good time to redefine the serialized representation
to be more robust. I'm imagining "<>" for a NULL array pointer and
"(item item item)" otherwise, allowing a cross-check that we're
getting the right number of items.

* gen_node_support.pl was being insufficiently careful about parsing
type names, so I tightened its regexes a bit.

regards, tom lane