Re: 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