Re: Partial aggregates pushdown

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