Message-ID: <890ed877-e2c0-448a-93b8-b07ccf3a2b37@postgrespro.ru>
Date: Thu, 22 Feb 2024 11:18:42 +0700
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: POC: GROUP BY optimization
Content-Language: en-US
To: Richard Guo <guofenglinux@gmail.com>,
 Alexander Korotkov <aekorotkov@gmail.com>
Cc: Tom Lane <tgl@sss.pgh.pa.us>, Pavel Borisov <pashkin.elfe@gmail.com>,
 vignesh C <vignesh21@gmail.com>,
 PostgreSQL Developers <pgsql-hackers@lists.postgresql.org>,
 Tomas Vondra <tomas.vondra@enterprisedb.com>,
 Teodor Sigaev <teodor@sigaev.ru>, David Rowley <dgrowleyml@gmail.com>,
 "a.rybakina" <a.rybakina@postgrespro.ru>
References: <CA+q6zcVRrd-z4YZ4M43ccst7aGL9==w5r1fionRWhP9ot6mybQ@mail.gmail.com>
 <f0ea5a6e-3e9b-48b2-b122-9196ee450cbd@postgrespro.ru>
 <CAPpHfdtWiQk587H3F-SHm4VUrb26rU7kBF1GtzV1CKOzz+GcOw@mail.gmail.com>
 <CAMbWs48jZKgqnCuWKzaxrzgthDW5MfJTfesnTGUOs+oWCn+uEw@mail.gmail.com>
 <CAPpHfdtqS1LXRSPNKjuG-gCxDdYaRk3YUisNgQco4yR3KmD0Zw@mail.gmail.com>
 <CAMbWs4-7oUGax4qWUwdiuyPKYKDO-DtKrcPedmjbPZyC-uYYrg@mail.gmail.com>
 <CAPpHfdsfHn89cAFEa6-6Xm1sB_xf8Rf2WNab6DQzb8Rvs0yD8g@mail.gmail.com>
 <CAPpHfdtNnX9PYmgEsh9DXYws2Px6UJhH5kYUxsg3hu-zJTYuWw@mail.gmail.com>
 <3046975.1706839355@sss.pgh.pa.us>
 <CAMbWs4_NF0stupM8guasC_yCJxdTvRqm3a+JLarjvAxcH314TQ@mail.gmail.com>
 <CAMbWs4-NKLa+Ss+X=WR6h0x=T07YBJoAz70ZGHzc-2zcHUHb0A@mail.gmail.com>
 <f5dae142-d351-4991-a2aa-92d12dcea004@postgrespro.ru>
 <CAMbWs49oMEFUEy6RRaenx5ak4AuZzuDEDzi0otEBYwJzBBYz7Q@mail.gmail.com>
 <CAPpHfdtMD=RhmB80DMRejs8s72Nr_y0OprUD9YiJFGgPVMdwXw@mail.gmail.com>
 <CAMbWs48MSejYST6pX8q-rEJMmRwo+fycDpc5vkkDusc=KF5y7Q@mail.gmail.com>
From: Andrei Lepikhov <a.lepikhov@postgrespro.ru>
Organization: Postgres Professional
In-Reply-To: <CAMbWs48MSejYST6pX8q-rEJMmRwo+fycDpc5vkkDusc=KF5y7Q@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Archived-At: <https://www.postgresql.org/message-id/890ed877-e2c0-448a-93b8-b07ccf3a2b37%40postgrespro.ru>
Precedence: bulk

On 22/2/2024 09:09, Richard Guo wrote:
> 
> On Wed, Feb 21, 2024 at 6:20 PM Alexander Korotkov <aekorotkov@gmail.com 
> <mailto:aekorotkov@gmail.com>> wrote:
> 
>     Hi, Richard!
> 
>      > What do you think about the revisions for the test cases?
> 
>     I've rebased your patch upthread.  Did some minor beautifications.
> 
>      > * The table 'btg' is inserted with 10000 tuples, which seems a bit
>      > expensive for a test.  I don't think we need such a big table to test
>      > what we want.
> 
>     Your patch reduces the number of rows to 1000 tuples.  I found it
>     possible to further reduce it to 100 tuples.  That also allowed me to
>     save the plan in the test case introduced by e1b7fde418.
> 
>     Please check if you're OK with the patch attached.
> 
> 
> I looked through the v2 patch and have two comments.
> 
> * The test case under "Check we don't pick aggregate path key instead of
> grouping path key" does not have EXPLAIN to show the plan.  So how can
> we ensure we do not mistakenly select the aggregate pathkeys instead of
> the grouping pathkeys?
I confirm it works correctly. I am not sure about the stability of the 
zeros number in the output on different platforms here:
        avg
--------------------
  4.0000000000000000
  5.0000000000000000
It was why I'd used the format() function before. So, may we elaborate 
on the test and restrict the output?
> 
> * I don't think the test case introduced by e1b7fde418 is still needed,
> because we already have one under "Utilize the ordering of merge join to
> avoid a full Sort operation".  This kind of test case is just to ensure
> that we are able to utilize the ordering of the subplans underneath.  So
> it should be parallel to the test cases for utilize the ordering of
> index scan and subquery scan.
I confirm, this version also checks ec_sortref initialization in the 
case when ec are contructed from WHERE clause. Generally, I like more 
one test for one issue instead of one test for all at once. But it works 
and I don't have any reason to dispute it.

Also, I'm unsure about removing the disabling of the 
max_parallel_workers_per_gather parameter. Have you discovered the 
domination of the current plan over the partial one? Do the cost 
fluctuations across platforms not trigger a parallel plan?

What's more, I suggest to address here the complaint from [1]. As I see, 
cost difference between Sort and IncrementalSort strategies in that case 
is around 0.5. To make the test more stable I propose to change it a bit 
and add a limit:
SELECT count(*) FROM btg GROUP BY z, y, w, x LIMIT 10;
It makes efficacy of IncrementalSort more obvious difference around 10 
cost points.

[1] 
https://www.postgresql.org/message-id/CACG=ezaYM1tr6Lmp8PRH1aeZq=rBKXEoTwgzMcLaD5MPhfW0Lg@mail.gmail.com

-- 
regards,
Andrei Lepikhov
Postgres Professional