On Wed, 21 Jul 2021 at 22:09, David Rowley wrote:
>
> On Wed, 21 Jul 2021 at 13:39, James Coleman wrote:
> > Thanks for doing the math measuring how much we could impact things.
> >
> > I'm +lots on getting this committed as is.
>
> Ok good. I plan on taking a final look at the v10 patch tomorrow
On July 22, 2021 8:38 AM David Rowley
> On Thu, 22 Jul 2021 at 12:27, houzj.f...@fujitsu.com
> wrote:
> > The above seems can be shorter like the following ?
> >
> > for (;;)
> > {
> > slot = ExecProcNode(outerNode);
> > if (TupIsNull(slot))
> > break;
> >
On Thu, 22 Jul 2021 at 12:27, houzj.f...@fujitsu.com
wrote:
> The above seems can be shorter like the following ?
>
> for (;;)
> {
> slot = ExecProcNode(outerNode);
> if (TupIsNull(slot))
> break;
> if (node->datumSort)
> {
> slot_get
From: David Rowley
> On Wed, 21 Jul 2021 at 13:39, James Coleman wrote:
> > Thanks for doing the math measuring how much we could impact things.
> >
> > I'm +lots on getting this committed as is.
>
> Ok good. I plan on taking a final look at the v10 patch tomorrow morning NZ
> time (about 12 hou
On Wed, 21 Jul 2021 at 13:39, James Coleman wrote:
> Thanks for doing the math measuring how much we could impact things.
>
> I'm +lots on getting this committed as is.
Ok good. I plan on taking a final look at the v10 patch tomorrow
morning NZ time (about 12 hours from now) and if all is well, I
On Tue, Jul 20, 2021 at 4:35 AM David Rowley wrote:
>
> On Tue, 20 Jul 2021 at 01:10, James Coleman wrote:
> > To be clear up front: I'm in favor of the patch, and I don't want to
> > put unnecessary stumbling blocks up for it getting committed. So if we
> > decide to proceed as is, that's fine w
On Tue, 20 Jul 2021 at 23:28, Ranier Vilela wrote:
> I took a look at cost_tuplesort and I think that some small adjustments could
> be made as part of the improvement process.
> It is attached.
> 1. long is a very problematic type; better int64?
> 2. 1024 can be int, not long?
> 3. 2 changed all
On Fri, 16 Jul 2021 at 15:44, David Rowley wrote:
>
> On Fri, 16 Jul 2021 at 02:53, Ronan Dunklau wrote:
> > Please find attached a v9 just moving the flag setting to ExecInitSort, and
> > my
> > apologies if I misunderstood your point.
>
> I took this and adjusted a few things and ended up with
Em ter., 20 de jul. de 2021 às 05:35, David Rowley
escreveu:
> On Tue, 20 Jul 2021 at 01:10, James Coleman wrote:
> > To be clear up front: I'm in favor of the patch, and I don't want to
> > put unnecessary stumbling blocks up for it getting committed. So if we
> > decide to proceed as is, that'
On Tue, 20 Jul 2021 at 01:10, James Coleman wrote:
> To be clear up front: I'm in favor of the patch, and I don't want to
> put unnecessary stumbling blocks up for it getting committed. So if we
> decide to proceed as is, that's fine with me.
Thanks for making that clear.
> But I'm not sure that
On Sat, Jul 17, 2021 at 4:36 AM David Rowley wrote:
>
> On Sat, 17 Jul 2021 at 01:14, James Coleman wrote:
> > The only remaining question I have is whether or not costing needs to
> > change, given the very significant speedup for datum sort.
>
> I'm looking at cost_tuplesort and the only thing
Le samedi 17 juillet 2021, 10:36:09 CEST David Rowley a écrit :
> On Sat, 17 Jul 2021 at 01:14, James Coleman wrote:
> > The only remaining question I have is whether or not costing needs to
> > change, given the very significant speedup for datum sort.
>
> I'm looking at cost_tuplesort and the
On Sat, 17 Jul 2021 at 01:14, James Coleman wrote:
> The only remaining question I have is whether or not costing needs to
> change, given the very significant speedup for datum sort.
I'm looking at cost_tuplesort and the only thing that I think might
make sense would be to adjust how the input_
On Thu, Jul 15, 2021 at 11:45 PM David Rowley wrote:
>
> On Fri, 16 Jul 2021 at 02:53, Ronan Dunklau wrote:
> > Please find attached a v9 just moving the flag setting to ExecInitSort, and
> > my
> > apologies if I misunderstood your point.
>
> I took this and adjusted a few things and ended up w
Em sex., 16 de jul. de 2021 às 00:45, David Rowley
escreveu:
> On Fri, 16 Jul 2021 at 02:53, Ronan Dunklau
> wrote:
> > Please find attached a v9 just moving the flag setting to ExecInitSort,
> and my
> > apologies if I misunderstood your point.
>
> I took this and adjusted a few things and ende
On Fri, 16 Jul 2021 at 02:53, Ronan Dunklau wrote:
> Please find attached a v9 just moving the flag setting to ExecInitSort, and my
> apologies if I misunderstood your point.
I took this and adjusted a few things and ended up with the attached patch.
The changes are fairly minor. I made the brac
On Fri, 16 Jul 2021 at 02:53, Ronan Dunklau wrote:
>
> Le jeudi 15 juillet 2021, 16:19:23 CEST David Rowley a écrit :>
> > Ronan's latest results plus John's make me think there's no need to
> > separate out the node function as I did in v8. However, I do think v6
> > could learn a little from v8
On Thu, Jul 15, 2021 at 10:19 AM David Rowley wrote:
>
> On Fri, 16 Jul 2021 at 01:44, James Coleman wrote:
> >
> > On Wed, Jul 14, 2021 at 9:22 PM David Rowley wrote:
> > >
> > > On Thu, 15 Jul 2021 at 12:30, Ranier Vilela wrote:
> > > >
> > > > Em qua., 14 de jul. de 2021 às 21:21, David Rowl
Le jeudi 15 juillet 2021, 16:19:23 CEST David Rowley a écrit :>
> Ronan's latest results plus John's make me think there's no need to
> separate out the node function as I did in v8. However, I do think v6
> could learn a little from v8. I think I'd rather see the sort method
> determined in Exec
Em qui., 15 de jul. de 2021 às 11:19, David Rowley
escreveu:
> On Fri, 16 Jul 2021 at 01:44, James Coleman wrote:
> >
> > On Wed, Jul 14, 2021 at 9:22 PM David Rowley
> wrote:
> > >
> > > On Thu, 15 Jul 2021 at 12:30, Ranier Vilela
> wrote:
> > > >
> > > > Em qua., 14 de jul. de 2021 às 21:21,
Em qua., 14 de jul. de 2021 às 22:22, David Rowley
escreveu:
> On Thu, 15 Jul 2021 at 12:30, Ranier Vilela wrote:
> >
> > Em qua., 14 de jul. de 2021 às 21:21, David Rowley
> escreveu:
> >> But, in v8 there is no additional branch, so no branch to mispredict.
> >> I don't really see how your ex
On Fri, 16 Jul 2021 at 01:44, James Coleman wrote:
>
> On Wed, Jul 14, 2021 at 9:22 PM David Rowley wrote:
> >
> > On Thu, 15 Jul 2021 at 12:30, Ranier Vilela wrote:
> > >
> > > Em qua., 14 de jul. de 2021 às 21:21, David Rowley
> > > escreveu:
> > >> But, in v8 there is no additional branch,
On Wed, Jul 14, 2021 at 9:22 PM David Rowley wrote:
>
> On Thu, 15 Jul 2021 at 12:30, Ranier Vilela wrote:
> >
> > Em qua., 14 de jul. de 2021 às 21:21, David Rowley
> > escreveu:
> >> But, in v8 there is no additional branch, so no branch to mispredict.
> >> I don't really see how your explana
Em qui., 15 de jul. de 2021 às 09:27, Ronan Dunklau
escreveu:
> Le jeudi 15 juillet 2021, 14:09:26 CEST Ranier Vilela a écrit :
> > Is there a special reason to not share v7b tests and results?
> >
>
> The v7b patch is wrong, as it loses the type of tuplesort being used
I don't see 'node->datumS
Le jeudi 15 juillet 2021, 14:09:26 CEST Ranier Vilela a écrit :
> Is there a special reason to not share v7b tests and results?
>
The v7b patch is wrong, as it loses the type of tuplesort being used and as
such always tries to fetch results using tuplesort_gettupleslot after the first
tuple is
Em qui., 15 de jul. de 2021 às 07:18, Ronan Dunklau
escreveu:
> Le jeudi 15 juillet 2021, 01:30:26 CEST John Naylor a écrit :
> > On Wed, Jul 14, 2021 at 6:14 AM David Rowley
> wrote:
> > > It would be good to get a 2nd opinion about this idea. Also, more
> > > benchmark results with v6 and v8
Le jeudi 15 juillet 2021, 01:30:26 CEST John Naylor a écrit :
> On Wed, Jul 14, 2021 at 6:14 AM David Rowley wrote:
> > It would be good to get a 2nd opinion about this idea. Also, more
> > benchmark results with v6 and v8 would be good too.
>
Hello,
Thank you for trying this approach in v8 Da
Em qua., 14 de jul. de 2021 às 22:22, David Rowley
escreveu:
> On Thu, 15 Jul 2021 at 12:30, Ranier Vilela wrote:
> >
> > Em qua., 14 de jul. de 2021 às 21:21, David Rowley
> escreveu:
> >> But, in v8 there is no additional branch, so no branch to mispredict.
> >> I don't really see how your ex
On Thu, 15 Jul 2021 at 12:30, Ranier Vilela wrote:
>
> Em qua., 14 de jul. de 2021 às 21:21, David Rowley
> escreveu:
>> But, in v8 there is no additional branch, so no branch to mispredict.
>> I don't really see how your explanation fits.
>
> In v8 the branch occurs at :
> + if (ExecGetResultTy
Em qua., 14 de jul. de 2021 às 21:21, David Rowley
escreveu:
> On Thu, 15 Jul 2021 at 12:10, Ranier Vilela wrote:
> >
> > Em qua., 14 de jul. de 2021 às 20:43, David Rowley
> escreveu:
> >>
> >> On Thu, 15 Jul 2021 at 05:55, Ranier Vilela
> wrote:
> >> > I repeated (3 times) the benchmark with
On Thu, 15 Jul 2021 at 12:10, Ranier Vilela wrote:
>
> Em qua., 14 de jul. de 2021 às 20:43, David Rowley
> escreveu:
>>
>> On Thu, 15 Jul 2021 at 05:55, Ranier Vilela wrote:
>> > I repeated (3 times) the benchmark with v8 here,
>> > and the results were not good.
>>
>> Do you have any good the
Em qua., 14 de jul. de 2021 às 20:43, David Rowley
escreveu:
> On Thu, 15 Jul 2021 at 05:55, Ranier Vilela wrote:
> > I repeated (3 times) the benchmark with v8 here,
> > and the results were not good.
>
> Do you have any good theories on why the additional branching that's
> done in v7b vs v8 m
On Thu, 15 Jul 2021 at 05:55, Ranier Vilela wrote:
> I repeated (3 times) the benchmark with v8 here,
> and the results were not good.
Do you have any good theories on why the additional branching that's
done in v7b vs v8 might cause it to run faster?
David
On Wed, Jul 14, 2021 at 6:14 AM David Rowley wrote:
> It would be good to get a 2nd opinion about this idea. Also, more
> benchmark results with v6 and v8 would be good too.
I tested this on an older Xeon, gcc 8.4 (here median of each test, full
results attached):
test HEAD v6 v8
Test1 588 10
Em qua., 14 de jul. de 2021 às 07:14, David Rowley
escreveu:
> On Tue, 13 Jul 2021 at 15:15, David Rowley wrote:
> > In theory, we likely could get rid of the small regression by having
> > two versions of ExecSort() and setting the correct one during
> > ExecInitSort() by setting the function p
Em ter., 13 de jul. de 2021 às 14:42, Ranier Vilela
escreveu:
> Em ter., 13 de jul. de 2021 às 09:44, Ranier Vilela
> escreveu:
>
>> Em ter., 13 de jul. de 2021 às 09:24, David Rowley
>> escreveu:
>>
>>> On Wed, 14 Jul 2021 at 00:06, Ranier Vilela wrote:
>>> >
>>> > Em ter., 13 de jul. de 2021
Em ter., 13 de jul. de 2021 às 09:44, Ranier Vilela
escreveu:
> Em ter., 13 de jul. de 2021 às 09:24, David Rowley
> escreveu:
>
>> On Wed, 14 Jul 2021 at 00:06, Ranier Vilela wrote:
>> >
>> > Em ter., 13 de jul. de 2021 às 04:19, Ronan Dunklau <
>> ronan.dunk...@aiven.io> escreveu:
>> >> I wou
Em ter., 13 de jul. de 2021 às 09:24, David Rowley
escreveu:
> On Wed, 14 Jul 2021 at 00:06, Ranier Vilela wrote:
> >
> > Em ter., 13 de jul. de 2021 às 04:19, Ronan Dunklau <
> ronan.dunk...@aiven.io> escreveu:
> >> I would be
> >> surprised the check adds that much to the whole execution thoug
On Wed, 14 Jul 2021 at 00:06, Ranier Vilela wrote:
>
> Em ter., 13 de jul. de 2021 às 04:19, Ronan Dunklau
> escreveu:
>> I would be
>> surprised the check adds that much to the whole execution though.
>
> I think this branch is a misprediction.
It could be. I wondered that myself when I saw R
Em ter., 13 de jul. de 2021 às 04:19, Ronan Dunklau
escreveu:
> > I've now pushed that bug fix so it's fine to remove the change to
>
>
> I would be
> surprised the check adds that much to the whole execution though.
>
I think this branch is a misprediction.
In most cases is it not datumSort?
Th
> I've now pushed that bug fix so it's fine to remove the change to
> tuplesort.c now.
Thanks, I've rebased the patch, please find attached the v6.
>
> I also did a round of benchmarking on this patch using the attached
> script. Anyone wanting to run it will need to run make installcheck
> firs
On Tue, 13 Jul 2021 at 01:59, Ronan Dunklau wrote:
> > 3. This seems to be a bug fix where byval datum sorts do not properly
> > handle bounded sorts. I think that maybe that should be fixed and
> > backpatched. I don't see anything that says Datum sorts can't be
> > bounded and if there were so
Le lundi 12 juillet 2021, 15:11:17 CEST David Rowley a écrit :
> On Wed, 7 Jul 2021 at 21:32, Ronan Dunklau wrote:
> > In the meantime I fixed some formatting issues, please find attached a new
> > patch.
>
> I started to look at this.
Thank you ! I'm attaching a new version of the patch taking
On Wed, 7 Jul 2021 at 21:32, Ronan Dunklau wrote:
> In the meantime I fixed some formatting issues, please find attached a new
> patch.
I started to look at this.
First I wondered how often we might be able to apply this
optimisation, so I ran make check after adding some elog(NOTICE) calls
to o
Le mardi 6 juillet 2021, 17:37:53 CEST James Coleman a écrit :
> Yes and no. When incremental sort has to do a full sort there will
> always be at least 2 attributes. But in prefix sort mode (see
> prefixsort_state) only non-presorted columns are sorted (i.e., if
> given a,b already sorted by a, th
On Tue, Jul 6, 2021 at 11:03 AM Ronan Dunklau wrote:
>
> Thank you for the review, I will address those shortly, but will answer some
> questions in the meantime.
>
> > > First, the changes are lacking any explanatory comments. Probably we
> > > should follow how nodeAgg does this and add both com
Thank you for the review, I will address those shortly, but will answer some
questions in the meantime.
> > First, the changes are lacking any explanatory comments. Probably we
> > should follow how nodeAgg does this and add both comments to the
> > ExecSort function header as well as specific co
On Tue, Jul 6, 2021 at 6:49 PM James Coleman wrote:
>
> Adding David since this patch is likely a precondition for [1].
>
> On Tue, Jul 6, 2021 at 2:15 AM Ronan Dunklau wrote:
> >
> > Hello,
> >
> > While testing the patch "Add proper planner support for ORDER BY / DISTINCT
> > aggregates" [0] I
Em ter., 6 de jul. de 2021 às 10:19, James Coleman
escreveu:
> Adding David since this patch is likely a precondition for [1].
>
> On Tue, Jul 6, 2021 at 2:15 AM Ronan Dunklau
> wrote:
> >
> > Hello,
> >
> > While testing the patch "Add proper planner support for ORDER BY /
> DISTINCT
> > aggreg
Adding David since this patch is likely a precondition for [1].
On Tue, Jul 6, 2021 at 2:15 AM Ronan Dunklau wrote:
>
> Hello,
>
> While testing the patch "Add proper planner support for ORDER BY / DISTINCT
> aggregates" [0] I discovered the performance penalty from adding a sort node
> essential
Em ter., 6 de jul. de 2021 às 08:25, Ranier Vilela
escreveu:
> Em ter., 6 de jul. de 2021 às 03:15, Ronan Dunklau
> escreveu:
>
>> Hello,
>>
>> While testing the patch "Add proper planner support for ORDER BY /
>> DISTINCT
>> aggregates" [0] I discovered the performance penalty from adding a sor
Em ter., 6 de jul. de 2021 às 03:15, Ronan Dunklau
escreveu:
> Hello,
>
> While testing the patch "Add proper planner support for ORDER BY /
> DISTINCT
> aggregates" [0] I discovered the performance penalty from adding a sort
> node
> essentially came from not using the single-datum tuplesort opt
Hello,
While testing the patch "Add proper planner support for ORDER BY / DISTINCT
aggregates" [0] I discovered the performance penalty from adding a sort node
essentially came from not using the single-datum tuplesort optimization in
ExecSort (contrary to the sorting done in ExecAgg).
I origi
53 matches
Mail list logo
