Tom Lane <t...@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
