Convert planner's AggInfo and AggTransInfo to Nodes

Tom Lane <tgl@sss.pgh.pa.us> writes:

I got annoyed just now upon finding that pprint() applied to the planner's
"root" pointer doesn't dump root->agginfos or root->aggtransinfos. That's
evidently because AggInfo and AggTransInfo aren't proper Nodes, just bare
structs, which presumably is because somebody couldn't be bothered to
write outfuncs support for them. I'd say that was a questionable shortcut
even when it was made, and there's certainly precious little excuse now
that gen_node_support.pl can do all the heavy lifting. Hence, PFA a
little finger exercise to turn them into Nodes. I took the opportunity
to improve related comments too, and in particular to fix some comments
that leave the impression that preprocess_minmax_aggregates still does
its own scan of the query tree. (It was momentary confusion over that
idea that got me to the point of being annoyed in the first place.)

Any objections so far?

It seems like a reasonable idea, but I don't know enough to judge the
wider ramifications of it. But one thing that the patch should also do,
is switch to using the l*_node() functions instead of manual casting.

The ones I noticed in the patch/context are below, but there are a few
more:

foreach(lc, root->agginfos)
{
AggInfo *agginfo = (AggInfo *) lfirst(lc);

AggInfo *agginfo = lfirst_node(AggInfo, lc);

[…]

foreach(lc, transnos)
{
int			transno = lfirst_int(lc);
-		AggTransInfo *pertrans = (AggTransInfo *) list_nth(root->aggtransinfos, transno);
+		AggTransInfo *pertrans = (AggTransInfo *) list_nth(root->aggtransinfos,
+														   transno);
+		AggTransInfo *pertrans = (AggTransInfo *) list_nth(root->aggtransinfos,
+														   transno);

AggTransInfo *pertrans = list_nth_node(AggTransInfo, root->aggtransinfos,
transno);

- ilmari

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.

=?utf-8?Q?Dagfinn_Ilmari_Manns=C3=A5ker?= <ilmari@ilmari.org> writes:

It seems like a reasonable idea, but I don't know enough to judge the
wider ramifications of it. But one thing that the patch should also do,
is switch to using the l*_node() functions instead of manual casting.

Hm, I didn't bother with that on the grounds that there's no question
what should be in those two lists. But I guess it's not much extra
code, so pushed that way.

regards, tom lane

Peter Eisentraut <peter.eisentraut@enterprisedb.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

I wrote:

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

Concretely, about like this.

regards, tom lane