Re: Final Patch for GROUPING SETS

On 2015-05-12 05:36:19 +0200, Andres Freund wrote:
> What I dislike so far:
> * Minor formatting things. Just going to fix and push the ones I
> dislike.
> * The Hopcroft-Karp stuff not being separate
> * The increased complexity of grouping_planner. It'd imo be good if some
> of that could be refactored into a separate function. Specifically the
> else if (parse->hasAggs || (parse->groupingSets && parse->groupClause))
> block.
> * I think it'd not hurt to add rule deparse check for the function in
> GROUPING SETS case. I didn't see one at least.

* The code in nodeAgg.c isn't pretty in places. Stuff like if
(node->chain_depth > 0) estate->agg_chain_head = save_chain_head;...
Feels like a good bit of cleanup would be possible there.

> I think the problem is "just" that for each variable, in each grouping
> set - a very large number in a large cube - we're recursing through the
> whole ChainAggregate tree, as each Var just points to a var one level
> lower.
>
> It might be worthwhile to add a little hack that deparses the variables
> agains the "lowest" relevant node (i.e. the one below the last chain
> agg). Since they'll all have the same targetlist that ought to be safe.

I've prototype hacked this, and indeed, adding a shortcut from the
intermediate chain nodes to the 'leaf chain node' cuts the explain time
from 11 to 2 seconds on some arbitrary statement. The remaining time is
the equivalent problem in the sort nodes...

I'm not terribly bothered by this. We can relatively easily fix this up
if required.

Greetings,

Andres Freund