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