On 11/1/21 22:53, Ilya Gladyshev wrote:
On 01.11.2021 13:30, Alexander Pyhalov wrote:
Peter Eisentraut писал 2021-11-01 12:47:
On 21.10.21 12:55, Alexander Pyhalov wrote:
Now aggregates with internal states can be pushed down, if they are
marked as pushdown safe (this flag is set to true for min/max/sum),
have internal states and associated converters. Converters are
called locally, they transform aggregate result to serialized
internal representation.
As converters don't have access to internal aggregate state, partial
aggregates like avg() are still not pushable.
It seems to me that the system should be able to determine from the
existing aggregate catalog entry whether an aggregate can be pushed
down. For example, it could check aggtranstype != internal and
similar. A separate boolean flag should not be necessary.
Hi.
I think we can't infer this property from existing flags. For example,
if I have avg() with bigint[] argtranstype, it doesn't mean we can
push down it. We couldn't also decide if partial aggregete is safe to
push down based on aggfinalfn presence (for example, it is defined for
sum(numeric), but we can push it down.
I think one potential way to do it would be to allow pushing down
aggregates that EITHER have state of the same type as their return type,
OR have a conversion function that converts their return value to the
type of their state.
IMO just checking (aggtranstype == result type) entirely ignores the
issue of portability - we've never required the aggregate state to be
portable in any meaningful way (between architectures, minor/major
versions, ...) and it seems foolish to just start relying on it here.
Imagine for example an aggregate using bytea state, storing some complex
C struct in it. You can't just copy that between architectures.
It's a bit like why we don't simply copy data types to network, but pass
them through input/output or send/receive functions. The new flag is a
way to mark aggregates where this is safe, and I don't think we can do
away without it.
The more I think about this, the more I'm convinced the proper way to do
this would be adding export/import functions, similar to serial/deserial
functions, with the extra portability guarantees. And we'd need to do
that for all aggregates, not just those with (aggtranstype == internal).
I get it - the idea of the patch is that keeping the data types the same
makes it much simpler to pass the aggregate state (compared to having to
export/import it). But I'm not sure it's the right approach.
regards
--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
