Horton

Thread

Commits

  1. Dump more fields when dumping planner internal data structures.

  2. Make serialization of Nodes' scalar-array fields more robust.

  3. Convert planner's AggInfo and AggTransInfo structs to proper Nodes.

  1. Convert planner's AggInfo and AggTransInfo to Nodes

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-07-18T16:08:24Z

    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?

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.

In particular, I'm tempted to make a dump of PlannerInfo include
all the baserel RelOptInfos (not joins though; there could be a
mighty lot of those.)  I think we didn't print the simple_rel_array[]
array before mostly because outfuncs didn't use to have reasonable
support for printing arrays.

Thoughts?

			regards, tom lane

  2. Re: Convert planner's AggInfo and AggTransInfo to Nodes

    Dagfinn Ilmari Mannsåker <ilmari@ilmari.org> — 2022-07-18T17:42:47Z

    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

  3. Re: Convert planner's AggInfo and AggTransInfo to Nodes

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2022-07-19T15:49:34Z

    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.

  4. Re: Convert planner's AggInfo and AggTransInfo to Nodes

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-07-19T16:31:08Z

    =?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

  5. Re: Convert planner's AggInfo and AggTransInfo to Nodes

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-07-19T19:23:57Z

    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

  6. Re: Convert planner's AggInfo and AggTransInfo to Nodes

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-07-19T23:08:55Z

    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