Hi Mr.Pyhalov. > 1) In previous version of the patch aggregates, which had partialaggfn, were > ok > to push down. And it was a definite sign that aggregate can be pushed down. > Now we allow pushing down an aggregate, which prorettype is not internal and > aggfinalfn is not defined. Is it safe for all user-defined (or builtin) > aggregates, > even if they are generally shippable? Aggcombinefn is executed locally and we > check that aggregate function itself is shippable. Is it enough? Perhaps, we > could use partialagg_minversion (like aggregates with partialagg_minversion > == -1 should not be pushed down) or introduce separate explicit flag? In what case partial aggregate pushdown is unsafe for aggregate which has not internal aggtranstype and has no aggfinalfn? By reading [1], I believe that if aggcombinefn of such aggregate recieves return values of original aggregate functions in each remote then it must produce same value that would have resulted from scanning all the input in a single operation.
> 2) Do we really have to look at pg_proc in partial_agg_ok() and > deparseAggref()? Perhaps, looking at aggtranstype is enough? You are right. I fixed according to your comment. > 3) I'm not sure if CREATE AGGREGATE tests with invalid > PARTIALAGGFUNC/PARTIALAGG_MINVERSION should be in postgres_fdw > tests or better should be moved to src/test/regress/sql/create_aggregate.sql, > as they are not specific to postgres_fdw Thank you. I moved these tests to src/test/regress/sql/create_aggregate.sql. [1] https://www.postgresql.org/docs/15/xaggr.html#XAGGR-PARTIAL-AGGREGATES Sincerely yours, Yuuki Fujii -- Yuuki Fujii Information Technology R&D Center Mitsubishi Electric Corporation
0001-Partial-aggregates-push-down-v14.patch
Description: 0001-Partial-aggregates-push-down-v14.patch
