Re: Partial aggregates pushdown - Mailing list pgsql-hackers
From | Jelte Fennema-Nio |
---|---|
Subject | Re: Partial aggregates pushdown |
Date | |
Msg-id | CAGECzQSKMO9C8-9Eo-feE3BJTF68EWKHXY4-xf_5BMKm8yAHPw@mail.gmail.com Whole thread Raw |
In response to | RE: Partial aggregates pushdown ("Fujii.Yuki@df.MitsubishiElectric.co.jp" <Fujii.Yuki@df.MitsubishiElectric.co.jp>) |
Responses |
RE: Partial aggregates pushdown
|
List | pgsql-hackers |
On Sun, 23 Jun 2024 at 10:24, Fujii.Yuki@df.MitsubishiElectric.co.jp <Fujii.Yuki@df.mitsubishielectric.co.jp> wrote: > I attached the POC patch(the second one) which supports avg(int8) whose standard format is _numeric type. Okay, very interesting. So instead of defining the serialization/deserialization functions to text/binary, you serialize the internal type to an existing non-internal type, which then in turn gets serialized to text. In the specific case of avg(int8) this is done to an array of numeric (with length 2). > However, I do not agree that I modify the internal transtype to the native data type. The reasons are the following three. > 1. Generality > I believe we should develop a method that can theoretically apply to any aggregate function, even if we cannot implementit immediately. However, I find it exceptionally challenging to demonstrate that any internal transtype can be universallyconverted to a native data type for aggregate functions that are parallel-safe under the current parallel queryfeature. Specifically, proving this for user-defined aggregate functions presents a significant difficulty in my view. > On the other hand, I think that the usage of export and import functions can theoretically apply to any aggregate functions. The only thing required when doing CREATE TYPE is having an INPUT and OUTPUT function for the type, which (de)serialize the type to text format. As far as I can tell by definition that requirement should be fine for any aggregates that we can do partial aggregation pushdown for. To clarify I'm not suggesting we should change any of the internal representation of the type for the current internal aggregates. I'm suggesting we create new native types (i.e. do CREATE TYPE) for those internal representations and then use the name of that type instead of internal. > 2. Amount of codes. > It could need more codes. I think it would be about the same as your proposal. Instead of serializing to an intermediary existing type you serialize to string straight away. I think it would probably be slightly less code actually, since you don't have to add code to handle the new aggpartialexportfn and aggpartialimportfn columns. > 3. Concern about performance > I'm concerned that altering the current internal data types could impact performance. As explained above in my proposal all the aggregation code would remain unchanged, only new native types will be added. Thus performance won't be affected, because all aggregation code will be the same. The only thing that's changed is that the internal type now has a name and an INPUT and OUTPUT function. > I know. In my proposal, the standard format is not seriarized data by serialfn, instead, is text or other native data type. > Just to clarify, I'm writing this to avoid any potential misunderstanding. Ah alright, that definitely clarifies the proposal. I was looking at the latest patch file on the thread and that one was still using serialfn. Your new one indeed doesn't, so this is fine. > Basically responding to your advice, > for now, I prepare two POC patches. Great! I definitely think this makes the review/discussion easier. > The first supports case a, currently covering only avg(int4) and other aggregate functions that do not require import orexport functions, such as min, max, and count. Not a full review but some initial notes: 1. Why does this patch introduce aggpartialpushdownsafe? I'd have expected that any type with a non-pseudo/internal type as aggtranstype would be safe to partially push down. 2. It seems the int4_avg_import function shouldn't be part of this patch (but maybe of a future one). 3. I think splitting this patch in two pieces would make it even easier to review: First adding support for the new PARTIAL_AGGREGATE keyword (adds the new feature). Second, using PARTIAL_AGGREGATE in postgres_fdw (starts using the new feature). Citus would only need the first patch not the second one, so I think the PARTIAL_AGGREGATE feature has merit to be added on its own, even without the postgres_fdw usage. 4. Related to 3, I think it would be good to have some tests of PARTIAL_AGGREGATE that don't involve postgres_fdw at all. I also spotted some comments too that mention FDW, even though they apply to the "pure" PARTIAL_AGGREGATE code. 5. This comment now seems incorrect: - * Apply the agg's finalfn if one is provided, else return transValue. + * If the agg's finalfn is provided and PARTIAL_AGGREGATE keyword is + * not specified, apply the agg's finalfn. + * If PARTIAL_AGGREGATE keyword is specified and the transValue type + * is internal, apply the agg's serialfn. In this case the agg's + * serialfn must not be invalid. Otherwise return transValue. 6. These errors are not on purpose afaict (if they are a comment in the test would be good to explain why) +SELECT b, avg(a), max(a), count(*) FROM pagg_tab GROUP BY b ORDER BY 1; +ERROR: could not connect to server "loopback" +DETAIL: invalid connection option "partial_aggregate_support" > The second supports case b and commonly used functions like sum and avg. Currently, it only includes avg(int8).
pgsql-hackers by date: