RE: Partial aggregates pushdown

Hi Mr.Bruce, hackers.

I updated the patch.
The following is a list of comments received on the previous version of the patch
and my update to them in this version of the patch.

[comment1]
> From: Bruce Momjian <bruce(at)momjian(dot)us>
> Sent: Saturday, April 8, 2023 10:50 AM
> Uh, we actually want the patch to implement partial aggregate pushdown for all
> builtin data types that can support it.

I improved the patch to push partial aggregate down for all builtin aggregates that
support it.

[comment2]
> From: Bruce Momjian <bruce(at)momjian(dot)us>
> Sent: Thursday, April 13, 2023 3:51 PM
> In summary, we don't do any version check for built-in function pushdown, so
> we don't need it for aggregates either. We check extension functions against
> the extension pushdown list, so we should be checking this for partial
> aggregate pushdown, and for partition-wise aggregate pushdown.

I removed partialagg_minversion from pg_aggregate and removed the version
check for partial aggregate pushdown by it. postgres_fdw assumes that every
built-in aggregate function has its aggpartialfunc on remote server.
postgres_fdw assumes that a user-defined aggregate function has its
aggpartialfunc on remote server only when the user-defined aggregate function
and the aggpartialfunc of it belong to some extension that's listed in the
foreign server's extensions option.

[comment3]
> From: Bruce Momjian <bruce(at)momjian(dot)us>
> Sent: Saturday, April 8, 2023 10:50 AM
> > > > 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 appended description for partial aggregate pushdown feature by postgres_fdw
to existing documents.
The following is a list of sgml files that have been appended and the
contents of the additions.
postgres-fdw.sgml : Description about this partial aggregate pushdown feature
and definition of aggpartialfunc.
* Unlike existing aggregate pushdown feature, this partial aggregate
pushdown feature is a one of the built-in sharding features in PostgreSQL.
So I added a section about built-in sharding feature in PostgreSQL, and in that
section I added a description of this partial aggregate pushdown feature.
In this document, a description of the built-in sharding feature in PostgreSQL
is based on [1].
xaggr.sgml :Partial aggregate pushdown feature for user-defined
aggregate functions.
create_aggregate.sgml:Description about additional parameters for
partial aggregate pushdown feature.
catalogs.sgml :Description about aggpartialfn column.

[comment4]
> From: Bruce Momjian <bruce(at)momjian(dot)us>
> Sent: Thursday, April 13, 2023 3:13 PM
> > There is one more thing I would like your opinion on.
> > As the major version of PostgreSQL increase, it is possible that the
> > new builtin aggregate functions are added to the newer PostgreSQL.
> > This patch assume that aggpartialfns definitions exist in BKI files.
> > Due to this assumption, someone should add aggpartialfns definitions of
> new builtin aggregate functions to BKI files.
> > There are two possible ways to address this issue. Would the way1 be
> sufficient?
> > Or would way2 be preferable?
> > way1) Adding documentaion for how to add these definitions to BKI files
> > way2) Improving this patch to automatically add these definitions to BKI
> files by some tool such as initdb.
>
> I think documentation is sufficient. You already showed that someone can do
> this with CREATE AGGREGATE for non-builtin aggregates.

The update addressesing to comment3 also addresses this comment.
If a new aggregate function is added in future,
definition of aggpartialfunc in postgres-fdw.sgml
helps postgres_fdw developer add a new aggpartialfunc corresponding to the aggregate
function to existing BKI files.
For user-defined aggregate functions, description in xaggr.sgml and
create_aggregate.sgml help a user create an aggpartialfunc corresponding to
a user-defined aggregate function.

In addition, I added a validation mechanism to determine whether an
aggpartialfunc corresponding to an aggregate function is correctly created.
The aggpartialfunc information for the built-in aggregate functions is registered
in the system catalog pg_aggregate at the time the database cluster is created
from BKI files.
So I added this validation process to the regression test of postgres_fdw.

However, I am concerned that it might be more appropriate to add this validation
process to the build process or initdb, rather than to the regression test.
I would appreciate comments from the PostgreSQL community on this point.
For aggpartialfunc for user-defined functions,
I added this validation process to pg_aggregate.c.

[comment5]
> From: Bruce Momjian <bruce(at)momjian(dot)us>
> Sent: Friday, April 7, 2023 11:00 AM
> I think 'partialaggfn' should be named 'aggpartialfn' to match other columns in
> pg_aggregate.

Fixed.

[1] PostgreSQL wiki, WIP PostgreSQL Sharding
https://wiki.postgresql.org/wiki/WIP_PostgreSQL_Sharding

Sincerely yours,
Yuuki Fujii

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