Re: Partial aggregates pushdown

Zhihong Yu писал 2021-10-22 00:43:
> Hi,
> w.r.t. 0001-Partial-aggregates-push-down-v03.patch
> 

Hi.

> For partial_agg_ok(),
> 
> +   if (agg->aggdistinct || agg->aggvariadic || agg->aggkind !=
> AGGKIND_NORMAL || agg->aggorder != NIL)
> +       ok = false;
> 
> Since SearchSysCache1() is not called yet, you can return false
> directly.

Fixed.

> 
> +       if (aggform->aggpartialpushdownsafe != true)
> 
> The above can be written as:
> 
>        if (!aggform->aggpartialpushdownsafe)

Fixed.

> 
> For build_conv_list():
> 
> +           Oid         converter_oid = InvalidOid;
> +
> +           if (IsA(tlentry->expr, Aggref))
> 
> ...
> +           }
> +           convlist = lappend_oid(convlist, converter_oid);
> 
> Do you intend to append InvalidOid to convlist (when tlentry->expr is
> not Aggref) ?

Yes, for each tlist member (which matches fpinfo->grouped_tlist in case 
when foreignrel is UPPER_REL) we need to find corresponding converter.
If we don't append InvalidOid, we can't find convlist member, 
corresponding to tlist member. Added comments to build_conv_list.

Also fixed error in pg_dump.c (we selected '0' when 
aggpartialconverterfn was not defined in schema, but checked for '-').

-- 
Best regards,
Alexander Pyhalov,
Postgres Professional