Message-ID: <52a3def2-4d2b-fb76-e334-5079ac67d7c7@gmail.com>
Date: Sat, 28 Jan 2023 13:51:09 +0530
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.4.2
Subject: Re: Todo: Teach planner to evaluate multiple windows in the optimal
 order
To: David Rowley <dgrowleyml@gmail.com>
Cc: pghackers <pgsql-hackers@lists.postgresql.org>,
 Vik Fearing <vik@postgresfriends.org>
References: <83d80853-a45c-d85c-68eb-59acfe7fb5fb@gmail.com>
 <c1d1aace-ce9f-9ec3-1984-5319920b1594@gmail.com>
 <CAApHDvp8BpUEo_kQdGHWNPCjcmRWCdiy5p26SoQA4R6rinkaLA@mail.gmail.com>
 <2040c902-32b1-46c8-0b26-d3f43a16b9bb@gmail.com>
 <12f6ef19-e06e-8ea9-ad98-3fd92afb7f0b@gmail.com>
 <CAApHDvp=YnmEQ8Ceu5g4wPL-b5YKmhQ7A3Lya6x=6ga=LLHMFA@mail.gmail.com>
 <8d0b24ce-f8ba-cd63-82ae-be853b4067e8@gmail.com>
 <CAApHDvpigtPVJOJ4EmWOAODtPTccz=L_gdPXDVgG2Z=iSPfNmQ@mail.gmail.com>
 <b1df021b-e6c0-f26a-1d5a-bd07f765db62@gmail.com>
 <CAApHDvqVjeHg6UgeCvFOa-uCaEFix9C_3YaT2qxsMtjN4oKbNQ@mail.gmail.com>
 <4ef7f947-526b-c4ec-f7a0-8b4586e693e4@gmail.com>
 <CAApHDvra7JeUm+wRpS+XoYXun_9b601s6ujWVG+gOd_AP5cADw@mail.gmail.com>
 <8599211e-1ea9-8827-4b9b-adeb865656bd@gmail.com>
 <CAApHDvpAO5H_L84kn9gCJ_hihOavtmDjimKYyftjWtF69BJ=8Q@mail.gmail.com>
 <a1c1f93a-16f7-4f8a-4562-4b89dd2e371c@gmail.com>
 <5314d2fc-76b6-c4ea-c083-1ef11411e5be@gmail.com>
 <CAApHDvqh+qOHk4sbvvy=Qr2NjPqAAVYf82oXY0g=Z2hRpC2Vmg@mail.gmail.com>
Content-Language: en-US
From: Ankit Kumar Pandey <itsankitkp@gmail.com>
In-Reply-To: <CAApHDvqh+qOHk4sbvvy=Qr2NjPqAAVYf82oXY0g=Z2hRpC2Vmg@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Archived-At: <https://www.postgresql.org/message-id/52a3def2-4d2b-fb76-e334-5079ac67d7c7%40gmail.com>
Precedence: bulk


> On 26/01/23 07:40, David Rowley wrote:

> You can see from the results that the patched version is not looking
> particularly great. I did a 1 million row test and a 10 million row
> test. work_mem was 4GB for each, so the sorts never went to disk.

Yes, its lackluster gains are very limited (pretty much when data gets
pushed to disk).


> I'm unsure if 69749243 might be partially to blame here as it favours
> single-key sorts.  If you look at qsort_tuple_signed_compare(), you'll
> see that the tiebreak function will be called only when it's needed
> and there are > 1 sort keys. The comparetup function will re-compare
> the first key all over again. If I get some more time I'll run the
> tests again with the sort specialisation code disabled to see if the
> situation is the same or not.
> Another way to test that would be to have a table with 3 columns and
> always sort by at least 2.

I will need to go through this.

> I've attached the benchmark script that I used and also a copy of the
> patch with a GUC added solely to allow easier benchmarking of patched
> vs unpatched.

This is much relief, will be easier to repro and create more cases. Thanks.

> I think we need to park this patch until we can figure out what can be
> done to stop these regressions. 

Makes sense.

> We might want to look at 1) Expanding
> on what 69749243 did and considering if we want sort specialisations
> that are specifically for 1 column and another set for multi-columns.
> The multi-column ones don't need to re-compare key[0] again. 2)
> Sorting in smaller batches that better fit into CPU cache. Both of
> these ideas would require a large amount of testing and discussion.
> For #1 we're considering other specialisations, for example NOT NULL,
> and we don't want to explode the number of specialisations we have to
> compile into the binary.

Yes, 1 & 2 needs to be addressed before going ahead with this patch.
Do we any have ongoing thread with #1 and #2?

Also, we have seen an issue with cost ( 1 sort vs 2 sorts, both having same cost)
https://www.postgresql.org/message-id/CAApHDvo2y9S2AO-BPYo7gMPYD0XE2Lo-KFLnqX80fcftqBCcyw@mail.gmail.com
This needs to be investigated too (although might not be related but need to check at least)

I will do some study around things mentioned here and will setup new threads (if they don't exists)
  based on discussions we had here .

Thanks,
Ankit