Re: Partial aggregates pushdown

On Fri, Apr 7, 2023 at 09:25:52AM +0000, Fujii(dot)Yuki(at)df(dot)MitsubishiElectric(dot)co(dot)jp wrote:
> Hi Mr.Momjian
>
> > First, my apologies for not addressing this sooner. I was so focused on my
> > own tasks that I didn't realize this very important patch was not getting
> > attention. I will try my best to get it into PG 17.
> Thank you very much for your comments.
> I will improve this patch for PG17.
> I believe that this patch will help us use PostgreSQL's built-in sharding for OLAP.

Agreed! Again, my apologies for not helping with this _much_ sooner.
You have done amazing work here.

> > What amazes me is that you didn't need to create _any_ actual aggregate
> > functions. Rather, you just needed to hook existing functions into the
> > aggregate tables for partial FDW execution.
> Yes. This patch enables partial aggregate pushdown using
> only existing functions which belong to existing aggregate function
> and are needed by parallel query(such as state transition function and serialization function).
> This patch does not need new types of function belonging to aggregate functions
> and does not need new functions belonging to existing aggregate functions.

Very nice.

> > I suggest we remove the version check requirement --- instead just document
> > that the FDW Postgres version should be the same or newer than the calling
> > Postgres server --- that way, we can assume that whatever is in the system
> > catalogs of the caller is in the receiving side.
> Thanks for the comment. I will modify this patch according to your comment.
>
> > We should add a GUC to turn off
> > this optimization for cases where the FDW Postgres version is older than the
> > caller. This handles case 1-2.
> Thanks for the advice here too.
> I thought it would be more appropriate to add a foregin server option of
> postgres_fdw rather than adding GUC.
> Would you mind if I ask you what you think about it?

I like the GUC idea because it gives administrators a single place to
check if the feature is enabled. However, I can imagine cases where you
might have multiple remote FDW servers and some might be older than the
sending server.

What I don't want is an error-prone setup where administrators have to
remember what the per-server settings are. Based on your suggestion,
let's allow CREATE SERVER to have an option 'enable_async_aggregate' (is
that the right name?), which defaults to 'true' for _all_ servers, even
those that don't support async aggregates. With that, all FDW servers
are enabled by default, and if the FDW extension supports async
aggregates, they will automatically be pushed down and will report an
error only if the remote FDW is too old to support it.

> > > 2. Automation of creating definition of partialaggfuncs In development
> > > of v17, I manually create definition of partialaggfuncs for avg, min, max, sum,
> > count.
> > > I am concerned that this may be undesirable.
> > > So I am thinking that v17 should be modified to automate creating
> > > definition of partialaggfuncs for all built-in aggregate functions.
> >
> > Are there any other builtin functions that need this? I think we can just
> > provide documention for extensions on how to do this.
> For practical purposes, it is sufficient
> if partial aggregate for the above functions can be pushed down.
> I think you are right, it would be sufficient to document how to achieve
> partial aggregate pushdown for other built-in functions.

Uh, we actually want the patch to implement partial aggregate pushdown
for all builtin data types that can support it. Is that done? I think
it is only extension aggregates, which we do not control, that need this
documentation.

> > > 3. Documentation
> > > I need add explanation of partialaggfunc to documents on postgres_fdw and
> > other places.
> >
> > I can help with that once we decide on the above.
> Thank you. In the next verion of this patch, I will add documents on postgres_fdw
> and other places.

Good.

> > I think 'partialaggfn' should be named 'aggpartialfn' to match other columns in
> > pg_aggregate.
> Thanks for the comment. I will modify this patch according to your comment.
>
> > For case 3, I don't even know how much pushdown those do of _any_
> > aggregates to non-PG servers, let along parallel FDW ones. Does anyone
> > know the details?
> To allow partial aggregate pushdown for non-PG FDWs,
> I think we need to add pushdown logic to their FDWs for each function.
> For example, we need to add logic avg() -> sum()/count() to their FDWs for avg.
> To allow parallel partial aggregate by non-PG FDWs,
> I think we need to add FDW Routines for Asynchronous Execution to their FDWs[1].

Okay, I think we can just implement this for 1-2 and let extensions
worry about 3.

> > I am confused by these changes to pg_aggegate:
> >
> > +{ aggfnoid => 'sum_p_int8', aggtransfn => 'int8_avg_accum',
> > + aggfinalfn => 'int8_avg_serialize', aggcombinefn =>
> > +'int8_avg_combine',
> > + aggserialfn => 'int8_avg_serialize', aggdeserialfn =>
> > +'int8_avg_deserialize',
> > + aggtranstype => 'internal', aggtransspace => '48' },
> >
> > ...
> >
> > +{ aggfnoid => 'sum_p_numeric', aggtransfn => 'numeric_avg_accum',
> > + aggfinalfn => 'numeric_avg_serialize', aggcombinefn =>
> > +'numeric_avg_combine',
> > + aggserialfn => 'numeric_avg_serialize',
> > + aggdeserialfn => 'numeric_avg_deserialize',
> > + aggtranstype => 'internal', aggtransspace => '128' },
> >
> > Why are these marked as 'sum' but use 'avg' functions?
> This reason is that sum(int8)/sum(numeric) shares some functions with avg(int8)/avg(numeric),
> and sum_p_int8 is aggpartialfn of sum(int8) and sum_p_numeric is aggpartialfn of sum(numeric).

Ah, I see this now, thanks.

--
Bruce Momjian <bruce(at)momjian(dot)us> https://momjian.us
EDB https://enterprisedb.com

Embrace your flaws. They make you human, rather than perfect,
which you will never be.