Feedback-ID: i1ff147bf:Fastmail
From: =?utf-8?Q?Dagfinn_Ilmari_Manns=C3=A5ker?= <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
References: <742479.1658160504@sss.pgh.pa.us>
Date: Mon, 18 Jul 2022 18:42:47 +0100
In-Reply-To: <742479.1658160504@sss.pgh.pa.us> (Tom Lane's message of "Mon, 18
	Jul 2022 12:08:24 -0400")
Message-ID: <87pmi2iag8.fsf@wibble.ilmari.org>
User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/27.1 (gnu/linux)
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 8bit
Archived-At: 
 <https://www.postgresql.org/message-id/87pmi2iag8.fsf%40wibble.ilmari.org>
Precedence: bulk

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