From: | Tomas Vondra <tomas(at)vondra(dot)me> |
---|---|
To: | Bruce Momjian <bruce(at)momjian(dot)us> |
Cc: | pgsql-hackers(at)lists(dot)postgresql(dot)org |
Subject: | Re: Partial aggregates pushdown |
Date: | 2024-08-22 19:54:02 |
Message-ID: | a1cb8e1a-93bd-4688-854d-805f68b70a89@vondra.me |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On 8/22/24 20:56, Bruce Momjian wrote:
> On Wed, Aug 21, 2024 at 05:41:02PM +0200, Tomas Vondra wrote:
>> On 8/8/24 13:48, Jelte Fennema-Nio wrote:
>>> SUMMARY OF THREAD
>>>
>>> The design of patch 0001 is agreed upon by everyone on the thread (so
>>> far). This adds the PARTIAL_AGGREGATE label for aggregates, which will
>>> cause the finalfunc not to run. It also starts using PARTIAL_AGGREGATE
>>> for pushdown of aggregates in postgres_fdw. In 0001 PARTIAL_AGGREGATE
>>> is only supported for aggregates with a non-internal/pseudo type as
>>> the stype.
>>
>> I don't have a strong opinion on this, but I wonder if someone might
>> object this essentially extends the syntax with something that is not
>> (and never will be) in the SQL standard. I wonder if there's some
>> precedent for encoding such explicit execution instructions into the
>> query itself?
>>
>> That reminds me - the PARTIAL_AGGREGATE label is per aggregate, but I
>> don't think it makes much sense to do this only for some aggregates,
>> right? Do we have a way to make sure the query is "consistent"? I'm not
>> sure if doing this on the source (before pushdown) is enough. Could
>> there be a difference in what the remote instance supports?
>>
>> The only alternative that I can think of (and that I believe was already
>> mentioned in this thread) is to set some GUC that forces the top-most
>> query level to do this (all aggregates at that level). That's have the
>> benefit of always affecting all aggregates.
>
> You make a very good point above. Would there ever be cases where a
> targetlist would have multiple aggregates, and some can be pushed down,
> and some have to return all matching rows so the sender can compute the
> aggregate? If so, how would we handle that? How does parallelism
> handle that now?
I think the only sane way to handle this is to disable partial pushdown
for that query, fetch all rows and do the aggregate locally.
>>> The design for patch 0002 is still under debate. This would expand on
>>> the functionality added by adding support for PARTIAL_AGGREGATE for
>>> aggregates with an internal stype. This is done by returning a byte
>>> array containing the bytes that the serialfunc of the aggregate
>>> returns.
>>>
>>> A competing proposal for 0002 is to instead change aggregates to not
>>> use an internal stype anymore, and create dedicated types. The main
>>> downside here is that infunc and outfunc would need to be added for
>>> text serialization, in addition to the binary serialization. An open
>>> question is: Can we change the requirements for CREATE TYPE, so that
>>> types can be created without infunc and outfunc.
>>>
>>
>> I think it's +0.5 for the new dedicated data types from me.
>>
>> I admit I'm too lazy to read the whole thread from scratch, but I
>> believe we did discuss the possibility to reuse the serial/deserial
>> functions we already have, but the reason against that was the missing
>> cross-version stability. Parallel queries always run within a single
>> instance, hence there are no concerns about other versions. But this is
>> meant to support the remote node having a wildly different version.
>
> It is more than different versions. Different CPU architectures can
> store data types differently in binary. It would be kind of interesting
> to have a serial/deserial that was in text format. I guess that is
> where my RECORD idea came from that I just emailed to the list.
>
I believe everything in this thread assumes "binary" in the same sense
as the protocol, with the same rules for sending data in text/binary
formats, etc. With binary for integers meaning "network order" and that
sort of stuff. Which is mostly independent of PG version.
When I say version dependency, I mean the structure of the aggregate
state itself. That is, in one version the state may store (A,B), while
in the other version it may be (B,C). For example, we could store
different values in the array, or something like that.
> What I would really like is something similar to pg_proc.proargtypes
> (which is data type oidvector) for pg_aggregate where we can supply the
> oids of the pg_aggregate.aggtranstype and construct a record on the fly
> to return to the FDW caller, rather than having to create specialized
> functions for every aggregate that needs to return several different
> data types, e.g., AVG(interval), #4 in my previous email. I realize
> this would require the creation of data types int128 and int128 array.
>
Isn't that really just a definition of a composite type?
I still don't understand which part of the code constructs the record.
If the aggregate has aggstate internal, I don't see how could it be
anything else than the aggregate itself.
>> I guess we might introduce another pair of serial/deserial functions,
>> with this guarantee. I know we (me and Jelte) discussed that in person
>> at some point, and there were arguments for doing the data types. But I
>> managed to forget the details :-(
>>
>>> WHAT IS NEEDED?
>>>
>>> The things needed for this patch are that docs need to be added, and
>>> detailed codereview needs to be done.
>>
>> Yeah, I think the docs are must-have for a proper review.
>
> I think the docs are on hold until we decide on a transfer method.
>
--
Tomas Vondra
From | Date | Subject | |
---|---|---|---|
Next Message | Rafia Sabih | 2024-08-22 19:56:10 | Re: pgstattuple: fix free space calculation |
Previous Message | Marcos Pegoraro | 2024-08-22 19:33:18 | Re: Detailed release notes |