Re: Planner, check if can use consider HASH for groupings (src/backend/optimizer/plan/planner.c) - Mailing list pgsql-hackers

From Ranier Vilela
Subject Re: Planner, check if can use consider HASH for groupings (src/backend/optimizer/plan/planner.c)
Date
Msg-id CAEudQAo5n8FeyzW1pLDj5+hyTjB_O_HcVG6huY0ZQMrHLsUvtg@mail.gmail.com
Whole thread Raw
In response to Re: Planner, check if can use consider HASH for groupings (src/backend/optimizer/plan/planner.c)  (Tomas Vondra <tomas.vondra@2ndquadrant.com>)
Responses Re: Planner, check if can use consider HASH for groupings (src/backend/optimizer/plan/planner.c)  (Tomas Vondra <tomas.vondra@2ndquadrant.com>)
List pgsql-hackers
Em qui., 17 de set. de 2020 às 17:09, Tomas Vondra <tomas.vondra@2ndquadrant.com> escreveu:
On Thu, Sep 17, 2020 at 02:12:12PM -0300, Ranier Vilela wrote:
>Hi,
>
>In case gd->any_hashable is FALSE, grouping_is_hashable is never called.
>In this case, the planner could use HASH for groupings, but will never know.
>

The condition is pretty simple - if the query has grouping sets, look at
grouping sets, otherwise look at groupClause. For this to be an issue
the query would need to have both grouping sets and (independent) group
clause - which AFAIK is not possible.
Uh?
(parse->groupClause != NIL) If different from NIL we have ((independent) group clause), grouping_is_hashable should check?
(gd ? gd->any_hashable : grouping_is_hashable(parse->groupClause))))
If gd is not NIL and gd->any_hashtable is FALSE?


For hashing to be worth considering, at least one grouping set has to be
hashable - otherwise it's pointless.

Granted, you could have something like

     GROUP BY GROUPING SETS ((a), (b)), c

which I think essentially says "add c to every grouping set" and that
will be covered by the any_hashable check.
I am not going into the merit of whether or not it is worth hashing.
grouping_is_hashable as a last resort, must decide.
 
>Apparently gd pointer, will never be NULL there, verified with Assert(gd !=
>NULL).
>

Um, what? If you add the assert right before the if condition, you won't
even be able to do initdb. It's pretty clear it'll crash for any query
without grouping sets.
Here not:
Assert(gd != NULL);
create_ordinary_grouping_paths(root, input_rel, grouped_rel,
  agg_costs, gd, &extra,
  &partially_grouped_rel);
 
Otherwise Coverity would be right.
CID 1412604 (#1 of 1): Dereference after null check (FORWARD_NULL)13. var_deref_model: Passing null pointer gd to create_ordinary_grouping_paths, which dereferences it. [show details]

Which cannot be true, gd is never NULL here.

regards,
Ranier Vilela

pgsql-hackers by date:

Previous
From: Tom Lane
Date:
Subject: Re: factorial function/phase out postfix operators?
Next
From: John Naylor
Date:
Subject: Re: WIP: BRIN multi-range indexes