RE: Partial aggregates pushdown

Hi. Mr.Pyhalov.

> From: Alexander Pyhalov <a(dot)pyhalov(at)postgrespro(dot)ru>
> Sent: Wednesday, February 28, 2024 10:43 PM
> > 1. Transmitting state value safely between machines
> >> From: Robert Haas <robertmhaas(at)gmail(dot)com>
> >> Sent: Wednesday, December 6, 2023 10:25 PM the problems around
> >> transmitting serialized bytea blobs between machines that can't be
> >> assumed to fully trust each other will need to be addressed in some
> >> way, which seems like it will require a good deal of design work,
> >> forming some kind of consensus, and then implementation work to
> >> follow.
> > I have considered methods for safely transmitting state values between
> > different machines.
> > I have taken into account the version policy of PostgreSQL (5 years of
> > support) and the major version release cycle over the past 10 years (1
> > year), and as a result, I have made the assumption that transmission
> > is allowed only when the difference between the local version and the
> > remote version is 5 or less.
> > I believe that by adding new components, "export function" and "import
> > function", to the aggregate functions, and further introducing a new
> > SQL keyword to the query syntax of aggregate expressions, we can
> > address this issue.
> >
...
>
> I honestly think that achieving cross-version compatibility in this way puts a significant burden on developers. Can we
> instead always use the more or less universal export and import function to fix possible issues with binary representations
> on different architectures and just refuse to push down partial aggregates on server version mismatch? At least at the first
> step?
Thank you for your comment. I agree with your point that the proposed method would impose a significant burden on developers. In order to ensure cross-version compatibility, it is necessary to impose constraints on the format of the state values exchanged between servers, which would indeed burden developers. As you mentioned, I think that it is realistic to allow partial aggregation pushdown only when coordinating between the same versions in the first step.

> From: Alexander Pyhalov <a(dot)pyhalov(at)postgrespro(dot)ru>
> Sent: Wednesday, February 28, 2024 10:43 PM
> > 3. Fixing the behavior when the HAVING clause is present
> >> From: Robert Haas <robertmhaas(at)gmail(dot)com>
> >> Sent: Tuesday, November 28, 2023 4:08 AM
> >>
> >> On Wed, Nov 22, 2023 at 1:32 AM Alexander Pyhalov
> >> <a(dot)pyhalov(at)postgrespro(dot)ru> wrote:
> >> > Hi. HAVING is also a problem. Consider the following query
> >> >
> >> > SELECT count(a) FROM t HAVING count(a) > 10 - we can't push it down
> >> > to foreign server as HAVING needs full aggregate result, but
> >> > foreign server don't know it.
> >>
> >> I don't see it that way. What we would push to the foreign server
> >> would be something like SELECT count(a) FROM t. Then, after we get
> >> the results back and combine the various partial counts locally, we
> >> would locally evaluate the HAVING clause afterward. That is, partial
> >> aggregation is a barrier to pushing down HAVING clause itself, but it
> >> doesn't preclude pushing down the aggregation.
> > I have made modifications in the attached patch to ensure that when
> > the HAVING clause is present, the HAVING clause is executed locally
> > while the partial aggregations are pushed down.
> >
> >
>
> Sorry, I don't see how it works. When we have partial aggregates and having clause, foreign_grouping_ok() returns false and
> add_foreign_grouping_paths() adds no paths.
> I'm not saying it's necessary to fix this in the first patch version.
Our sincere apologies. I had attached an older version before this modification.

> From: Alexander Pyhalov <a(dot)pyhalov(at)postgrespro(dot)ru>
> Sent: Wednesday, February 28, 2024 10:43 PM
> contrib/postgres_fdw/deparse.c: comment before appendFunctionName() has gone, this seems to be wrong.
Fixed.

> From: Alexander Pyhalov <a(dot)pyhalov(at)postgrespro(dot)ru>
> Sent: Wednesday, February 28, 2024 10:43 PM
> In finalize_aggregate()
>
> 1079 /*
> 1080 * Apply the agg's finalfn if one is provided, else return
> transValue.
> 1081 */
>
> Comment should be updated to note behavior for agg_partial aggregates.
Fixed.

> From: Alexander Pyhalov <a(dot)pyhalov(at)postgrespro(dot)ru>
> Sent: Wednesday, February 28, 2024 10:43 PM
> In this if branch, should we check just for peragg->aggref->agg_partial and peragg->aggref->aggtranstype ==
> INTERNALOID? It seems that if
> peragg->aggref->aggtranstype == INTERNALOID and there's no
> serialfn_oid, it's likely an error (and one should be generated).
As you pointed out, I have made modifications to the source code so that it terminates with an error if serialfn is invalid.

Sincerely yours,
Yuuki Fujii

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