Re: Partial aggregates pushdown

2023-04-07 Thread Tom Lane
Bruce Momjian writes: > On Fri, Apr 7, 2023 at 09:55:00PM -0400, Tom Lane wrote: >> Uh, what? Why would we not be able to tell from the remote server's >> version number whether it has this ability? > The issue is not a mismatch of postgres_fdw versions but the extension > versions and whether

Re: Partial aggregates pushdown

2023-04-07 Thread Bruce Momjian
On Fri, Apr 7, 2023 at 09:55:00PM -0400, Tom Lane wrote: > Bruce Momjian writes: > > What I don't want is an error-prone setup where administrators have to > > remember what the per-server settings are. Based on your suggestion, > > let's allow CREATE SERVER to have an option

Re: Partial aggregates pushdown

2023-04-07 Thread Tom Lane
Bruce Momjian writes: > What I don't want is an error-prone setup where administrators have to > remember what the per-server settings are. Based on your suggestion, > let's allow CREATE SERVER to have an option 'enable_async_aggregate' (is > that the right name?), which defaults to 'true' for

Re: Partial aggregates pushdown

2023-04-07 Thread Bruce Momjian
On Fri, Apr 7, 2023 at 09:25:52AM +, fujii.y...@df.mitsubishielectric.co.jp wrote: > Hi Mr.Momjian > > > First, my apologies for not addressing this sooner. I was so focused on my > > own tasks that I didn't realize this very important patch was not getting > > attention. I will try my

RE: Partial aggregates pushdown

2023-04-07 Thread fujii.y...@df.mitsubishielectric.co.jp
Hi Mr.Momjian > First, my apologies for not addressing this sooner. I was so focused on my > own tasks that I didn't realize this very important patch was not getting > attention. I will try my best to get it into PG 17. Thank you very much for your comments. I will improve this patch for

Re: Partial aggregates pushdown

2023-04-06 Thread Bruce Momjian
On Fri, Mar 31, 2023 at 05:49:21AM +, fujii.y...@df.mitsubishielectric.co.jp wrote: > Hi Mr.Momjian > > > First, am I correct? > Yes, you are correct. This patch uses new special aggregate functions for > partial aggregate > (then we call this partialaggfunc). First, my apologies for not

RE: Partial aggregates pushdown

2023-03-30 Thread fujii.y...@df.mitsubishielectric.co.jp
Hi Mr.Momjian > First, am I correct? Yes, you are correct. This patch uses new special aggregate functions for partial aggregate (then we call this partialaggfunc). > Second, how far away is this from being committable > and/or what work needs to be done to get it committable, either for PG 16

Re: Partial aggregates pushdown

2023-03-30 Thread Bruce Momjian
On Thu, Dec 15, 2022 at 10:23:05PM +, fujii.y...@df.mitsubishielectric.co.jp wrote: > Hi Mr.Freund. > > > cfbot complains about some compiler warnings when building with clang: > > https://cirrus-ci.com/task/6606268580757504 > > > > deparse.c:3459:22: error: equality comparison with

RE: Partial aggregates pushdown

2022-12-15 Thread fujii.y...@df.mitsubishielectric.co.jp
Hi Mr.Freund. > cfbot complains about some compiler warnings when building with clang: > https://cirrus-ci.com/task/6606268580757504 > > deparse.c:3459:22: error: equality comparison with extraneous parentheses > [-Werror,-Wparentheses-equality] > if ((node->aggsplit == AGGSPLIT_SIMPLE))

Re: Partial aggregates pushdown

2022-12-07 Thread Andres Freund
Hi, On 2022-12-05 02:03:49 +, fujii.y...@df.mitsubishielectric.co.jp wrote: > > Attaching minor fixes. I haven't proof-read all comments (but perhaps, they > > need attention from some native speaker). > Thank you. I fixed according to your patch. > And I fixed have proof-read all comments

RE: Partial aggregates pushdown

2022-12-04 Thread fujii.y...@df.mitsubishielectric.co.jp
Hi Mr.Pyhalov. > Attaching minor fixes. I haven't proof-read all comments (but perhaps, they > need attention from some native speaker). Thank you. I fixed according to your patch. And I fixed have proof-read all comments and messages. > Tested it with queries from >

Re: Partial aggregates pushdown

2022-12-01 Thread Alexander Pyhalov
fujii.y...@df.mitsubishielectric.co.jp писал 2022-12-01 05:23: Hi Mr.Pyhalov. Hi. Attaching minor fixes. I haven't proof-read all comments (but perhaps, they need attention from some native speaker). Tested it with queries from https://github.com/swarm64/s64da-benchmark-toolkit, works as

RE: Partial aggregates pushdown

2022-11-30 Thread fujii.y...@df.mitsubishielectric.co.jp
Hi Mr.Pyhalov. > One more issue I started to think about - now we don't check > partialagg_minversion for "simple" aggregates at all. Is it correct? It seems > that , > for example, we could try to pushdown bit_or(int8) to old servers, but it > didn't > exist, for example, in 8.4. I think it's

Re: Partial aggregates pushdown

2022-11-30 Thread Alexander Pyhalov
fujii.y...@df.mitsubishielectric.co.jp писал 2022-11-30 13:01: 2) Do we really have to look at pg_proc in partial_agg_ok() and deparseAggref()? Perhaps, looking at aggtranstype is enough? You are right. I fixed according to your comment. partial_agg_ok() still looks at pg_proc. -- Best

Re: Partial aggregates pushdown

2022-11-30 Thread Alexander Pyhalov
fujii.y...@df.mitsubishielectric.co.jp писал 2022-11-30 13:01: Hi Mr.Pyhalov. 1) In previous version of the patch aggregates, which had partialaggfn, were ok to push down. And it was a definite sign that aggregate can be pushed down. Now we allow pushing down an aggregate, which prorettype is

RE: Partial aggregates pushdown

2022-11-30 Thread fujii.y...@df.mitsubishielectric.co.jp
Hi Mr.Pyhalov. > 1) In previous version of the patch aggregates, which had partialaggfn, were > ok > to push down. And it was a definite sign that aggregate can be pushed down. > Now we allow pushing down an aggregate, which prorettype is not internal and > aggfinalfn is not defined. Is it safe

Re: Partial aggregates pushdown

2022-11-30 Thread Alexander Pyhalov
Hi, Yuki. 1) In previous version of the patch aggregates, which had partialaggfn, were ok to push down. And it was a definite sign that aggregate can be pushed down. Now we allow pushing down an aggregate, which prorettype is not internal and aggfinalfn is not defined. Is it safe for all

RE: Partial aggregates pushdown

2022-11-29 Thread fujii.y...@df.mitsubishielectric.co.jp
Hi Mr.Pyhalov. Thank you for comments. > I've looked through the patch. Overall I like this approach, but have > the following comments. > > 1) Why should we require partialaggfn for min()/max()/count()? We could > just use original functions for a lot of aggregates, and so it would be >

Re: Partial aggregates pushdown

2022-11-22 Thread Alexander Pyhalov
fujii.y...@df.mitsubishielectric.co.jp писал 2022-11-22 04:01: Hi Mr.Vondra, Mr.Pyhalov, Everyone. I discussed with Mr.Pyhalov about the above draft by directly sending mail to him(outside of pgsql-hackers). Mr.Pyhalov allowed me to update his patch along with the above draft. So I update

Re: [CAUTION!! freemail] Re: Partial aggregates pushdown

2022-11-22 Thread Ted Yu
On Tue, Nov 22, 2022 at 1:11 AM fujii.y...@df.mitsubishielectric.co.jp < fujii.y...@df.mitsubishielectric.co.jp> wrote: > Hi Mr.Yu. > > Thank you for comments. > > > + * Check that partial aggregate agg has compatibility > > > > If the `agg` refers to func parameter, the parameter name is aggform

Re: Partial aggregates pushdown

2022-11-21 Thread Ted Yu
On Mon, Nov 21, 2022 at 5:02 PM fujii.y...@df.mitsubishielectric.co.jp < fujii.y...@df.mitsubishielectric.co.jp> wrote: > Hi Mr.Vondra, Mr.Pyhalov, Everyone. > > I discussed with Mr.Pyhalov about the above draft by directly sending mail > to > him(outside of pgsql-hackers). Mr.Pyhalov allowed me

RE: Partial aggregates pushdown

2022-07-31 Thread fujii.y...@df.mitsubishielectric.co.jp
Hi Mr.Vondra, Mr.Pyhalov. I'm interesied in Mr.Pyhalov's patch due to the following background. --Background I develop postgresql's extension such as fdw in my work. I'm interested in using postgresql for OLAP. I think the function of a previous patch "Push aggregation down to base relations

Re: Partial aggregates pushdown

2022-03-22 Thread Alexander Pyhalov
Tomas Vondra писал 2022-03-22 15:28: On 3/22/22 01:49, Andres Freund wrote: On 2022-01-17 15:27:53 +0300, Alexander Pyhalov wrote: Alexander Pyhalov писал 2022-01-17 15:26: Updated patch. Sorry, missed attachment. Needs another update: http://cfbot.cputube.org/patch_37_3369.log Marked as

Re: Partial aggregates pushdown

2022-03-22 Thread Tomas Vondra
On 3/22/22 01:49, Andres Freund wrote: > On 2022-01-17 15:27:53 +0300, Alexander Pyhalov wrote: >> Alexander Pyhalov писал 2022-01-17 15:26: >>> Updated patch. >> >> Sorry, missed attachment. > > Needs another update: http://cfbot.cputube.org/patch_37_3369.log > > Marked as waiting on author. >

Re: Partial aggregates pushdown

2022-03-21 Thread Andres Freund
On 2022-01-17 15:27:53 +0300, Alexander Pyhalov wrote: > Alexander Pyhalov писал 2022-01-17 15:26: > > Updated patch. > > Sorry, missed attachment. Needs another update: http://cfbot.cputube.org/patch_37_3369.log Marked as waiting on author. - Andres

Re: Partial aggregates pushdown

2022-01-17 Thread Alexander Pyhalov
Alexander Pyhalov писал 2022-01-17 15:26: Zhihong Yu писал 2022-01-17 11:43: Hi, + FdwScanPrivateConvertors + * Generate attinmeta if there are some converters: I think it would be better if converter is spelled the same way across the patch. For build_conv_list(): + if

Re: Partial aggregates pushdown

2022-01-17 Thread Alexander Pyhalov
Zhihong Yu писал 2022-01-17 11:43: Hi, + FdwScanPrivateConvertors + * Generate attinmeta if there are some converters: I think it would be better if converter is spelled the same way across the patch. For build_conv_list(): + if (IS_UPPER_REL(foreignrel)) You can return NIL for

Re: Partial aggregates pushdown

2022-01-17 Thread Zhihong Yu
On Sun, Jan 16, 2022 at 11:47 PM Alexander Pyhalov wrote: > Julien Rouhaud писал 2022-01-14 15:16: > > Hi, > > > > On Mon, Nov 15, 2021 at 04:01:51PM +0300, Alexander Pyhalov wrote: > >> > >> I've updated patch - removed catversion dump. > > > > This version of the patchset doesn't apply

Re: Partial aggregates pushdown

2022-01-16 Thread Alexander Pyhalov
Julien Rouhaud писал 2022-01-14 15:16: Hi, On Mon, Nov 15, 2021 at 04:01:51PM +0300, Alexander Pyhalov wrote: I've updated patch - removed catversion dump. This version of the patchset doesn't apply anymore: http://cfbot.cputube.org/patch_36_3369.log === Applying patches on top of

Re: Partial aggregates pushdown

2022-01-14 Thread Julien Rouhaud
Hi, On Mon, Nov 15, 2021 at 04:01:51PM +0300, Alexander Pyhalov wrote: > > I've updated patch - removed catversion dump. This version of the patchset doesn't apply anymore: http://cfbot.cputube.org/patch_36_3369.log === Applying patches on top of PostgreSQL commit ID

Re: Partial aggregates pushdown

2021-11-15 Thread Alexander Pyhalov
Daniel Gustafsson писал 2021-11-15 13:16: On 3 Nov 2021, at 15:50, Alexander Pyhalov wrote: Daniel Gustafsson писал 2021-11-03 16:45: On 2 Nov 2021, at 10:12, Alexander Pyhalov wrote: Updated and rebased patch. + state = (Int128AggState *) palloc0(sizeof(Int128AggState)); +

Re: Partial aggregates pushdown

2021-11-15 Thread Daniel Gustafsson
> On 3 Nov 2021, at 15:50, Alexander Pyhalov wrote: > > Daniel Gustafsson писал 2021-11-03 16:45: >>> On 2 Nov 2021, at 10:12, Alexander Pyhalov wrote: >>> Updated and rebased patch. >> +state = (Int128AggState *) palloc0(sizeof(Int128AggState)); >> +state->calcSumX2 = false; >> + >> +

Re: Partial aggregates pushdown

2021-11-03 Thread Alexander Pyhalov
Daniel Gustafsson писал 2021-11-03 16:45: On 2 Nov 2021, at 10:12, Alexander Pyhalov wrote: Updated and rebased patch. + state = (Int128AggState *) palloc0(sizeof(Int128AggState)); + state->calcSumX2 = false; + + if (!PG_ARGISNULL(0)) + { +#ifdef HAVE_INT128 +

Re: Partial aggregates pushdown

2021-11-03 Thread Daniel Gustafsson
> On 2 Nov 2021, at 10:12, Alexander Pyhalov wrote: > Updated and rebased patch. + state = (Int128AggState *) palloc0(sizeof(Int128AggState)); + state->calcSumX2 = false; + + if (!PG_ARGISNULL(0)) + { +#ifdef HAVE_INT128 + do_int128_accum(state, (int128)

Re: Partial aggregates pushdown

2021-11-02 Thread Alexander Pyhalov
Hi. Updated and rebased patch. Ilya Gladyshev писал 2021-11-02 00:31: Hi, On 21.10.2021 13:55, Alexander Pyhalov wrote: Hi. Updated patch. 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

Re: Partial aggregates pushdown

2021-11-01 Thread Tomas Vondra
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

Re: Partial aggregates pushdown

2021-11-01 Thread Tomas Vondra
On 11/1/21 22:31, Ilya Gladyshev wrote: Hi, On 21.10.2021 13:55, Alexander Pyhalov wrote: Hi. Updated patch. 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

Re: Partial aggregates pushdown

2021-11-01 Thread Ilya Gladyshev
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

Re: Partial aggregates pushdown

2021-11-01 Thread Ilya Gladyshev
Hi, On 21.10.2021 13:55, Alexander Pyhalov wrote: Hi. Updated patch. 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. I don't quite understand why this is

Re: Partial aggregates pushdown

2021-11-01 Thread Alexander Pyhalov
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

Re: Partial aggregates pushdown

2021-11-01 Thread Peter Eisentraut
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

Re: Partial aggregates pushdown

2021-10-22 Thread Alexander Pyhalov
Zhihong Yu писал 2021-10-22 00:43: Hi, w.r.t. 0001-Partial-aggregates-push-down-v03.patch Hi. For partial_agg_ok(), + if (agg->aggdistinct || agg->aggvariadic || agg->aggkind != AGGKIND_NORMAL || agg->aggorder != NIL) + ok = false; Since SearchSysCache1() is not called yet, you

Re: Partial aggregates pushdown

2021-10-21 Thread Zhihong Yu
Hi, w.r.t. 0001-Partial-aggregates-push-down-v03.patch For partial_agg_ok(), + if (agg->aggdistinct || agg->aggvariadic || agg->aggkind != AGGKIND_NORMAL || agg->aggorder != NIL) + ok = false; Since SearchSysCache1() is not called yet, you can return false directly. + if

Re: Partial aggregates pushdown

2021-10-21 Thread Alexander Pyhalov
Tomas Vondra писал 2021-10-19 16:25: On 10/19/21 08:56, Alexander Pyhalov wrote: Hi. Tomas Vondra писал 2021-10-15 17:56: As for the proposed approach, it's probably good enough for the first version to restrict this to aggregates where the aggregate result is sufficient, i.e. we don't need

Re: Partial aggregates pushdown

2021-10-19 Thread Tomas Vondra
On 10/19/21 08:56, Alexander Pyhalov wrote: Hi. Tomas Vondra писал 2021-10-15 17:56: As for the proposed approach, it's probably good enough for the first version to restrict this to aggregates where the aggregate result is sufficient, i.e. we don't need any new export/import procedures. But

Re: Partial aggregates pushdown

2021-10-19 Thread Alexander Pyhalov
Hi. Tomas Vondra писал 2021-10-15 17:56: As for the proposed approach, it's probably good enough for the first version to restrict this to aggregates where the aggregate result is sufficient, i.e. we don't need any new export/import procedures. But it's very unlikely we'd want to restrict it

Re: Partial aggregates pushdown

2021-10-15 Thread Tomas Vondra
On 10/15/21 21:31, Stephen Frost wrote: Greetings, * Tomas Vondra (tomas.von...@enterprisedb.com) wrote: On 10/15/21 17:05, Alexander Pyhalov wrote: Tomas Vondra писал 2021-10-15 17:56: And then we should extend this for aggregates with more complex internal states (e.g. avg), by supporting

Re: Partial aggregates pushdown

2021-10-15 Thread Stephen Frost
Greetings, * Tomas Vondra (tomas.von...@enterprisedb.com) wrote: > On 10/15/21 17:05, Alexander Pyhalov wrote: > >Tomas Vondra писал 2021-10-15 17:56: > >>And then we should extend this for aggregates with more complex > >>internal states (e.g. avg), by supporting a function that "exports" >

Re: Partial aggregates pushdown

2021-10-15 Thread Tomas Vondra
On 10/15/21 17:05, Alexander Pyhalov wrote: Tomas Vondra писал 2021-10-15 17:56: Hi Alexander, Hi. And then we should extend this for aggregates with more complex internal states (e.g. avg), by supporting a function that "exports" the aggregate state - similar to serial/deserial functions,

Re: Partial aggregates pushdown

2021-10-15 Thread Alexander Pyhalov
Tomas Vondra писал 2021-10-15 17:56: Hi Alexander, Hi. And then we should extend this for aggregates with more complex internal states (e.g. avg), by supporting a function that "exports" the aggregate state - similar to serial/deserial functions, but needs to be portable. I think the

Re: Partial aggregates pushdown

2021-10-15 Thread Tomas Vondra
Hi Alexander, On 10/15/21 15:15, Alexander Pyhalov wrote: Hi. One of the issues when we try to use sharding in PostgreSQL is absence of partial aggregates pushdown. I see several opportunities to alleviate this issue. If we look at Citus, it implements aggregate, calculating internal state

<    1   2