Content-Type: multipart/mixed; boundary="------------jKg8Rdd8p6XF2jpLBiq8ofhG"
Message-ID: <50c1c1dc-057c-421b-a1f4-62622c7afe61@postgrespro.ru>
Date: Thu, 18 Apr 2024 12:01:13 +0700
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: POC: GROUP BY optimization
To: Tom Lane <tgl@sss.pgh.pa.us>, Alexander Korotkov <aekorotkov@gmail.com>
Cc: Richard Guo <guofenglinux@gmail.com>,
 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>
 <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>
 <890ed877-e2c0-448a-93b8-b07ccf3a2b37@postgrespro.ru>
 <CAMbWs4-wAwJgxAnUNjCz8Qgzgjwn9t_NN9=i46hVPdaOT6Jmmg@mail.gmail.com> <d0265!
 80a-15a0-4734-9e69-cc6ebb70da1d@postgrespro.ru>
 <266850.1712879082@sss.pgh.pa.us>
Content-Language: en-AU
From: Andrei Lepikhov <a.lepikhov@postgrespro.ru>
Organization: Postgres Professional
In-Reply-To: <266850.1712879082@sss.pgh.pa.us>
Archived-At: <https://www.postgresql.org/message-id/50c1c1dc-057c-421b-a1f4-62622c7afe61%40postgrespro.ru>
Precedence: bulk

This is a multi-part message in MIME format.
--------------jKg8Rdd8p6XF2jpLBiq8ofhG
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit

On 4/12/24 06:44, Tom Lane wrote:
> * It seems like root->processed_groupClause no longer has much to do
> with the grouping behavior, which is scary given how much code still
> believes that it does.  I suspect there are bugs lurking there, and
1. Analysing the code, processed_groupClause is used in:
- adjust_foreign_grouping_path_cost - to decide, do we need to add cost 
of sort to the foreign grouping.
- just for replacing relids during SJE process.
- create_groupingsets_plan(), preprocess_grouping_sets, 
remove_useless_groupby_columns - we don't apply this feature in the case 
of grouping sets.
- get_number_of_groups - estimation shouldn't depend on the order of 
clauses.
- create_grouping_paths - to analyse hashability of grouping clause
- make_group_input_target, make_partial_grouping_target - doesn't depend 
on the order of clauses
planner.c: add_paths_to_grouping_rel, create_partial_grouping_paths - in 
the case of hash grouping.

So, the only case we can impact current behavior lies in the 
postgres_fdw. But here we don't really know which plan will be chosen 
during planning on foreign node and stay the same behavior is legal for me.
> am not comforted by the fact that the patch changed exactly nothing
> in the pathnodes.h documentation of that field.  This comment looks
> pretty silly now too:
> 
>              /* Preprocess regular GROUP BY clause, if any */
>              root->processed_groupClause = list_copy(parse->groupClause);
> 
> What "preprocessing" is going on there?  This comment was adequate
> before, when the code line invoked preprocess_groupclause which had
> a bunch of commentary; but now it has to stand on its own and it's
> not doing a great job of that.
Thanks for picking this place. I fixed it.
More interesting here is that we move decisions on the order of group-by 
clauses to the stage, where we already have underlying subtree and 
incoming path keys. In principle, we could return the preprocessing 
routine and arrange GROUP-BY with the ORDER-BY clause as it was before. 
But looking into the code, I found only one reason to do this: 
adjust_group_pathkeys_for_groupagg. Curious about how much profit we get 
here, I think we can discover it later with no hurry. A good outcome 
here will be a test case that can show the importance of arranging 
GROUP-BY and ORDER-BY at an early stage.

-- 
regards,
Andrei Lepikhov
Postgres Professional

--------------jKg8Rdd8p6XF2jpLBiq8ofhG
Content-Type: text/x-patch; charset=UTF-8; name="minor_comment.patch"
Content-Disposition: attachment; filename="minor_comment.patch"
Content-Transfer-Encoding: base64

ZGlmZiAtLWdpdCBhL3NyYy9iYWNrZW5kL29wdGltaXplci9wbGFuL3BsYW5uZXIuYyBiL3Ny
Yy9iYWNrZW5kL29wdGltaXplci9wbGFuL3BsYW5uZXIuYwppbmRleCA1ZWEzNzA1Nzk2Li44
NjE2NTZhMTkyIDEwMDY0NAotLS0gYS9zcmMvYmFja2VuZC9vcHRpbWl6ZXIvcGxhbi9wbGFu
bmVyLmMKKysrIGIvc3JjL2JhY2tlbmQvb3B0aW1pemVyL3BsYW4vcGxhbm5lci5jCkBAIC0x
NDI0LDcgKzE0MjQsMTAgQEAgZ3JvdXBpbmdfcGxhbm5lcihQbGFubmVySW5mbyAqcm9vdCwg
ZG91YmxlIHR1cGxlX2ZyYWN0aW9uLAogCQl9CiAJCWVsc2UgaWYgKHBhcnNlLT5ncm91cENs
YXVzZSkKIAkJewotCQkJLyogUHJlcHJvY2VzcyByZWd1bGFyIEdST1VQIEJZIGNsYXVzZSwg
aWYgYW55ICovCisJCQkvKgorCQkJICogTWFrZSBhIGNvcHkgb2Ygb3JpZ2luIGdyb3VwQ2xh
dXNlIGJlY2F1c2Ugd2UgYXJlIGdvaW5nIHRvCisJCQkgKiByZW1vdmUgcmVkdW5kYW50IGNs
YXVzZXMuCisJCQkgKi8KIAkJCXJvb3QtPnByb2Nlc3NlZF9ncm91cENsYXVzZSA9IGxpc3Rf
Y29weShwYXJzZS0+Z3JvdXBDbGF1c2UpOwogCQkJLyogUmVtb3ZlIGFueSByZWR1bmRhbnQg
R1JPVVAgQlkgY29sdW1ucyAqLwogCQkJcmVtb3ZlX3VzZWxlc3NfZ3JvdXBieV9jb2x1bW5z
KHJvb3QpOwo=

--------------jKg8Rdd8p6XF2jpLBiq8ofhG--