Re: WIP: Aggregation push-down

Andy Fan <zhihui.fan1213@gmail.com> wrote:

>  > 1) v14-0001-Introduce-RelInfoList-structure.patch
>  > -------------------------------------------------
>  > 
>  > - I'm not entirely sure why we need this change. We had the list+hash
>  > before, so I assume we do this because we need the output functions?
> 
>  I believe that this is what Tom proposed in [1], see "Maybe an appropriate
>  preliminary patch is to refactor the support code ..." near the end of that
>  message. The point is that now we need two lists: one for "plain" relations
>  and one for grouped ones.
> 
>  
> I guess what Toms suggested[1] is to store the the grouped ones into 
> root->upper_rels rather than a separated member, see fetch_upper_rel
> or UpperRelationKind.  If we do need the list+hash method for long list lookup, 
> we can merge it into upper_rels.  If we want this benefits at other place rather 
> than root->upper_rels, we can store a generic type in RelInfoList, looks currently
> it is bounded to RelAggInfo besides RelOptInfo.   But overall, personally I think we can 
> ignore such optimization at the first stage to save the attention of the core reviewers
> since they are too precious :)    Just FYI
> 
> [1] https://www.postgresql.org/message-id/9726.1542577439@sss.pgh.pa.us 

Hm, you seem to be right, not sure why I missed the point. I thought that the
reason Tom doesn't like storing the grouped relations in simple_rel_array is
that we only need the grouped base relations inside query_planner(), but
simple_rel_array is used higher in the stack. So I introduced a new field and
used it only in query_planner() and subroutines.

Yes, it's better to use root->upper_rels than to introduce the new field. I'll
adjust the patch. Thanks.

-- 
Antonin Houska
Web: https://www.cybertec-postgresql.com