From: Dagfinn Ilmari Mannsåker <ilmari@ilmari.org>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: pgsql-hackers@lists.postgresql.org
Subject: Re: Convert planner's AggInfo and AggTransInfo to Nodes
Date: Mon, 18 Jul 2022 18:42:47 +0100
Message-ID: <87pmi2iag8.fsf@wibble.ilmari.org> (raw)
In-Reply-To: <742479.1658160504@sss.pgh.pa.us>
References: <742479.1658160504@sss.pgh.pa.us>
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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: ilmari@ilmari.org, tgl@sss.pgh.pa.us, pgsql-hackers@lists.postgresql.org
Subject: Re: Convert planner's AggInfo and AggTransInfo to Nodes
In-Reply-To: <87pmi2iag8.fsf@wibble.ilmari.org>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox