RE: Partial aggregates pushdown

Hi Mr.Bruce.

Tuesday, September 26, 2023 7:31 Bruce Momjian
> On Mon, Sep 25, 2023 at 03:18:13AM +0000, Fujii.Yuki@df.MitsubishiElectric.co.jp wrote:
> > Hi Mr.Bruce, Mr.Pyhalov, Mr.Finnerty, hackers.
> >
> > Thank you for your valuable comments. I sincerely apologize for the very late reply.
> > Here is a response to your comments or a fix to the patch.
> >
> > Tuesday, August 8, 2023 at 3:31 Bruce Momjian
> > > > I have modified the program except for the point "if the version of the remote server is less than PG17".
> > > > Instead, we have addressed the following.
> > > > "If check_partial_aggregate_support is true and the remote server
> > > > version is older than the local server version, postgres_fdw does
> > > > not assume that the partial aggregate function is on the remote server unless the partial aggregate function
andthe 
> aggregate function match."
> > > > The reason for this is to maintain compatibility with any
> > > > aggregate function that does not support partial aggregate in one version of V1 (V1 is PG17 or higher), even if
the
> next version supports partial aggregate.
> > > > For example, string_agg does not support partial aggregation in
> > > > PG15, but it will support partial aggregation in PG16.
> > >
> > > Just to clarify, I think you are saying:
> > >
> > >         If check_partial_aggregate_support is true and the remote server
> > >         version is older than the local server version, postgres_fdw
> > >         checks if the partial aggregate function exists on the remote
> > >         server during planning and only uses it if it does.
> > >
> > > I tried to phrase it in a positive way, and mentioned the plan time
> > > distinction.  Also, I am sorry I was away for most of July and am
> > > just getting to this.
> > Thanks for your comment. In the documentation, the description of
> > check_partial_aggregate_support is as follows (please see postgres-fdw.sgml).
> > --
> > check_partial_aggregate_support (boolean) Only if this option is true,
> > during query planning, postgres_fdw connects to the remote server and check if the remote server version is older
than
> the local server version. If so, postgres_fdw assumes that for each built-in aggregate function, the partial
aggregate
> function is not defined on the remote server unless the partial aggregate function and the aggregate function match.
The
> default is false.
> > --
>
> My point is that there are three behaviors:
>
> *  false - no check
> *  true, remote version >= sender - no check
> *  true, remove version < sender - check
>
> Here is your code:
>
>     + * Check that a buit-in aggpartialfunc exists on the remote server. If
>     + * check_partial_aggregate_support is false, we assume the partial aggregate
>     + * function exsits on the remote server. Otherwise we assume the partial
>     + * aggregate function exsits on the remote server only if the remote server
>     + * version is not less than the local server version.
>     + */
>     +static bool
>     +is_builtin_aggpartialfunc_shippable(Oid aggpartialfn, PgFdwRelationInfo *fpinfo)
>     +{
>     +       bool            shippable = true;
>     +
>     +       if (fpinfo->check_partial_aggregate_support)
>     +       {
>     +               if (fpinfo->remoteversion == 0)
>     +               {
>     +                       PGconn     *conn = GetConnection(fpinfo->user, false, NULL);
>     +
>     +                       fpinfo->remoteversion = PQserverVersion(conn);
>     +               }
>     +               if (fpinfo->remoteversion < PG_VERSION_NUM)
>     +                       shippable = false;
>     +       }
>     +       return shippable;
>     +}
>
> I think this needs to be explained in the docs.  I am ready to adjust the patch to improve the wording whenever you
are
> ready.  Should I do it now and post an updated version for you to use?
The following explanation was omitted from the documentation, so I added it.
> *  false - no check
> *  true, remove version < sender - check
I have responded to your comment, but if there is a problem with the wording, could you please suggest a correction?

Sincerely yours,
Yuuki Fujii

--
Yuuki Fujii
Information Technology R&D Center Mitsubishi Electric Corporation

> -----Original Message-----
> From: Bruce Momjian <bruce@momjian.us>
> Sent: Tuesday, September 26, 2023 7:31 AM
> To: Fujii Yuki/藤井 雄規(MELCO/情報総研 ＤＭ最適Ｇ) <Fujii.Yuki@df.MitsubishiElectric.co.jp>
> Cc: Alexander Pyhalov <a.pyhalov@postgrespro.ru>; Finnerty, Jim <jfinnert@amazon.com>; PostgreSQL-development
> <pgsql-hackers@postgresql.org>; Andres Freund <andres@anarazel.de>; Tom Lane <tgl@sss.pgh.pa.us>; Tomas
> Vondra <tomas.vondra@enterprisedb.com>; Julien Rouhaud <rjuju123@gmail.com>; Daniel Gustafsson
> <daniel@yesql.se>; Ilya Gladyshev <i.gladyshev@postgrespro.ru>
> Subject: Re: Partial aggregates pushdown
>
> On Mon, Sep 25, 2023 at 03:18:13AM +0000, Fujii.Yuki@df.MitsubishiElectric.co.jp wrote:
> > Hi Mr.Bruce, Mr.Pyhalov, Mr.Finnerty, hackers.
> >
> > Thank you for your valuable comments. I sincerely apologize for the very late reply.
> > Here is a response to your comments or a fix to the patch.
> >
> > Tuesday, August 8, 2023 at 3:31 Bruce Momjian
> > > > I have modified the program except for the point "if the version of the remote server is less than PG17".
> > > > Instead, we have addressed the following.
> > > > "If check_partial_aggregate_support is true and the remote server
> > > > version is older than the local server version, postgres_fdw does
> > > > not assume that the partial aggregate function is on the remote server unless the partial aggregate function
andthe 
> aggregate function match."
> > > > The reason for this is to maintain compatibility with any
> > > > aggregate function that does not support partial aggregate in one version of V1 (V1 is PG17 or higher), even if
the
> next version supports partial aggregate.
> > > > For example, string_agg does not support partial aggregation in
> > > > PG15, but it will support partial aggregation in PG16.
> > >
> > > Just to clarify, I think you are saying:
> > >
> > >         If check_partial_aggregate_support is true and the remote server
> > >         version is older than the local server version, postgres_fdw
> > >         checks if the partial aggregate function exists on the remote
> > >         server during planning and only uses it if it does.
> > >
> > > I tried to phrase it in a positive way, and mentioned the plan time
> > > distinction.  Also, I am sorry I was away for most of July and am
> > > just getting to this.
> > Thanks for your comment. In the documentation, the description of
> > check_partial_aggregate_support is as follows (please see postgres-fdw.sgml).
> > --
> > check_partial_aggregate_support (boolean) Only if this option is true,
> > during query planning, postgres_fdw connects to the remote server and check if the remote server version is older
than
> the local server version. If so, postgres_fdw assumes that for each built-in aggregate function, the partial
aggregate
> function is not defined on the remote server unless the partial aggregate function and the aggregate function match.
The
> default is false.
> > --
>
> My point is that there are three behaviors:
>
> *  false - no check
> *  true, remote version >= sender - no check
> *  true, remove version < sender - check
>
> Here is your code:
>
>     + * Check that a buit-in aggpartialfunc exists on the remote server. If
>     + * check_partial_aggregate_support is false, we assume the partial aggregate
>     + * function exsits on the remote server. Otherwise we assume the partial
>     + * aggregate function exsits on the remote server only if the remote server
>     + * version is not less than the local server version.
>     + */
>     +static bool
>     +is_builtin_aggpartialfunc_shippable(Oid aggpartialfn, PgFdwRelationInfo *fpinfo)
>     +{
>     +       bool            shippable = true;
>     +
>     +       if (fpinfo->check_partial_aggregate_support)
>     +       {
>     +               if (fpinfo->remoteversion == 0)
>     +               {
>     +                       PGconn     *conn = GetConnection(fpinfo->user, false, NULL);
>     +
>     +                       fpinfo->remoteversion = PQserverVersion(conn);
>     +               }
>     +               if (fpinfo->remoteversion < PG_VERSION_NUM)
>     +                       shippable = false;
>     +       }
>     +       return shippable;
>     +}
>
> I think this needs to be explained in the docs.  I am ready to adjust the patch to improve the wording whenever you
are
> ready.  Should I do it now and post an updated version for you to use?
>
> --
>   Bruce Momjian  <bruce@momjian.us>        https://momjian.us
>   EDB                                      https://enterprisedb.com
>
>   Only you can decide what is important to you.