Subject: Re: POC: GROUP BY optimization
To: Teodor Sigaev <teodor@sigaev.ru>,
 Pgsql Hackers <pgsql-hackers@postgresql.org>
References: <7c79e6a5-8597-74e8-0671-1c39d124c9d6@sigaev.ru>
 <97d148b7-f3e7-1c39-07fd-74cbc66f6ec9@sigaev.ru>
 <6d1e0cdb-dde3-f62a-43e2-e90bbd9b0f42@2ndquadrant.com>
 <b2c0e7bf-0d38-0042-dbbe-7e50b6a3454f@sigaev.ru>
 <7edddb4f-9ddd-561d-2bde-47340f91d2a0@2ndquadrant.com>
 <2cb99655-7ae7-ec8d-2c74-3fea58987104@sigaev.ru>
 <6dddcce6-0d91-1868-794a-4d61dc5516f0@2ndquadrant.com>
 <a02225c6-96d3-2e85-9907-9459105d1244@sigaev.ru>
 <e33558a8-6528-f0c4-8f59-04ff7e2e529c@2ndquadrant.com>
 <523d1f60-c60c-5c8c-8cc5-c6c078970da6@2ndquadrant.com>
 <24c773b8-3a9a-8fe9-610d-01145758931b@sigaev.ru>
From: Tomas Vondra <tomas.vondra@2ndquadrant.com>
Message-ID: <623f5e50-eeb2-1279-d30b-dc43611fb063@2ndquadrant.com>
Date: Sat, 16 Jun 2018 17:59:24 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101
 Thunderbird/52.5.2
MIME-Version: 1.0
In-Reply-To: <24c773b8-3a9a-8fe9-610d-01145758931b@sigaev.ru>
Content-Type: text/plain; charset=koi8-r
Content-Language: en-US
Content-Transfer-Encoding: 8bit
Precedence: bulk


On 06/13/2018 06:56 PM, Teodor Sigaev wrote:
>> I.e. we already do reorder the group clauses to match ORDER BY, to only
>> require a single sort. This happens in preprocess_groupclause(), which
>> also explains the reasoning behind that.
> Huh. I missed that. That means group_keys_reorder_by_pathkeys() call
> inside get_cheapest_group_keys_order() duplicates work of
> preprocess_groupclause().
> šAnd so it's possible to replace it to simple counting number of the
> same pathkeys in beginning of lists. Or remove most part of
> preprocess_groupclause() - but it seems to me we should use first
> option, preprocess_groupclause() is simpler, it doesn't operate with
> pathkeys only withš SortGroupClause which is simpler.
> 
> BTW, incremental sort path provides pathkeys_common(), exactly what we
> need.
> 
>> I wonder if some of the new code reordering group pathkeys could/should
>> be moved here (not sure, maybe it's too early for those decisions). In
>> any case, it might be appropriate to update some of the comments before
>> preprocess_groupclause() which claim we don't do certain things added by
>> the proposed patches.
> 
> preprocess_groupclause() is too early step to use something like
> group_keys_reorder_by_pathkeys() because pathkeys is not known here yet.
> 
> 
>> This probably also somewhat refutes my claim that the order of
>> grouping keys is currently fully determined by users (and so they
>> may pick the most efficient order), while the reorder-by-ndistinct
>> patch would make that impossible. Apparently when there's ORDER BY,
>> we already mess with the order of group clauses - there are ways to
>> get around it (subquery with OFFSET 0) but it's much less clear.
> 
> I like a moment when objections go away :)
> 

I'm afraid that's a misunderstanding - my objections did not really go
away. I'm just saying my claim that we're not messing with order of
grouping keys is not entirely accurate, because we do that in one
particular case.

I still think we need to be careful when introducing new optimizations
in this area - reordering the grouping keys by ndistinct, ORDER BY or
whatever. In particular I don't think we should commit these patches
that may quite easily cause regressions, and then hope some hypothetical
future patch fixes the costing. Those objections still stand.

regards

-- 
Tomas Vondra                  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services