Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.94.2) (envelope-from ) id 1rPMtx-00B5ND-JT for pgsql-hackers@arkaria.postgresql.org; Mon, 15 Jan 2024 13:21:19 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.94.2) (envelope-from ) id 1rPMtu-00FWHF-Ar for pgsql-hackers@arkaria.postgresql.org; Mon, 15 Jan 2024 13:21:14 +0000 Received: from makus.postgresql.org ([2001:4800:3e1:1::229]) by malur.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.94.2) (envelope-from ) id 1rPMtt-00FWH7-Pc for pgsql-hackers@lists.postgresql.org; Mon, 15 Jan 2024 13:21:14 +0000 Received: from mail.postgrespro.ru ([93.174.131.139]) by makus.postgresql.org with esmtps (TLS1.2) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.94.2) (envelope-from ) id 1rPMtp-001eWN-Gb for pgsql-hackers@lists.postgresql.org; Mon, 15 Jan 2024 13:21:12 +0000 Received: from [10.4.13.208] (unknown [93.174.131.141]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (Client did not present a certificate) (Authenticated sender: a.rybakina@postgrespro.ru) by mail.postgrespro.ru (Postfix/587) with ESMTPSA id CB71AE203EB; Mon, 15 Jan 2024 16:21:05 +0300 (MSK) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=postgrespro.ru; s=mx2023; t=1705324865; bh=UzpyhHB/O0pjqZNhRn20l8t99OBWDcBzed/b2w1ZblY=; h=Message-ID:Date:User-Agent:Subject:To:Cc:References:From: In-Reply-To:From; b=Oir6BsTRcfvhonWkFxmqQOm5mLPTgL9UfNiJWAGZ/ZTFr29NCBY5pRT198vBhRxoq wGJ31it5WZQWAvndQIiqQPrXl3JxeXjdQtDIKgIY/Nln8vurvATIdhmErRmjYWi3W4 eMNvm6pFk9IV4ZJ0fmprlQfT+zOQQ1UFrf4Da8AUa7DD4Lupt/Khxj9qmWDWAj0bmz jmn2/dRhY1TnWgmlYi/gbqLFwU9dkFRWpjnglOd4nDKh9vSoeOUtfhavpqvVMEBCI9 mECh+Ub9EqeBBbSXo76hLnAF1wEAaAu9nMR4Vsv9f/Wp/G09VTOEh8ExE8PU0w9vbU ojWhhZxLeUEvA== Content-Type: multipart/mixed; boundary="------------d3qZmSbptV1Nl70B4UKGSICT" Message-ID: Date: Mon, 15 Jan 2024 16:21:05 +0300 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: POC: GROUP BY optimization Content-Language: en-US To: Andrei Lepikhov , Richard Guo , Alexander Korotkov Cc: Pavel Borisov , vignesh C , PostgreSQL Developers , Tomas Vondra , Teodor Sigaev , David Rowley , Tom Lane References: <60610df1-c32f-ebdf-e58c-7a664431f452@enterprisedb.com> <9acd27e9-3372-4d78-b9bc-73e407b8a007@postgrespro.ru> <138cb2df-1a0d-48c0-be80-48aaf62e8aeb@postgrespro.ru> <57478722-f891-46cb-b3fe-d140fedd4ebe@postgrespro.ru> From: Alena Rybakina In-Reply-To: <57478722-f891-46cb-b3fe-d140fedd4ebe@postgrespro.ru> List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Archived-At: Precedence: bulk This is a multi-part message in MIME format. --------------d3qZmSbptV1Nl70B4UKGSICT Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit On 15.01.2024 12:46, Andrei Lepikhov wrote: > On 15/1/2024 13:42, Richard Guo wrote: >> >> On Mon, Jan 15, 2024 at 8:20 AM Alexander Korotkov >> > wrote: >> >>     Thank you for providing the test case relevant for this code change. >>     The revised patch incorporating this change is attached. Now the >>     patchset looks good to me.  I'm going to push it if there are no >>     objections. >> >> >> Seems I'm late for the party.  Can we hold for several more days?  I'd >> like to have a review on this patch. > Get on board! It looks like this feature needs as much review as > possible (likewise SJE). > Hi! Thank you for your work on this issue! I believe that this will help the scheduler to make a more optimal query plan here and therefore speed up their execution. I have reviewed patches and noticed that we can add some code refactoring. I have attached a diff file (group_by.diff) to this email. The changes involve spelling corrections, renaming variables and porting some common parts. In addition, I have a few questions, since some points in the code remained unclear to me. 1.  I didn't understand why we have a question in the comment next to the enable_group_by_reordering variable in src/backend/optimizer/path/pathkeys.c file, I assumed it was spelling and fixed it in the diff file. 2. Why do we set the variable (path = path_save) here (add_paths_to_grouping_rel function) if we change its variable below and we can pass path_save as a parameter? foreach(lc2, pathkey_orderings) {     PathKeyInfo *info = (PathKeyInfo *) lfirst(lc2);     /* restore the path (we replace it in the loop) */     path = path_save;     path = make_ordered_path(root,                              grouped_rel,                              path,                              cheapest_path,                              info->pathkeys);     if (path == NULL)         continue; -- Regards, Alena Rybakina Postgres Professional: http://www.postgrespro.com The Russian Postgres Company --------------d3qZmSbptV1Nl70B4UKGSICT Content-Type: text/x-patch; charset=UTF-8; name="group_by.diff" Content-Disposition: attachment; filename="group_by.diff" Content-Transfer-Encoding: base64 ZGlmZiAtLWdpdCBhL3NyYy9iYWNrZW5kL29wdGltaXplci9wYXRoL3BhdGhrZXlzLmMgYi9z cmMvYmFja2VuZC9vcHRpbWl6ZXIvcGF0aC9wYXRoa2V5cy5jCmluZGV4IDVhYWM2ZDY2Nzc2 Li44YmU1OGZhMmIwZSAxMDA2NDQKLS0tIGEvc3JjL2JhY2tlbmQvb3B0aW1pemVyL3BhdGgv cGF0aGtleXMuYworKysgYi9zcmMvYmFja2VuZC9vcHRpbWl6ZXIvcGF0aC9wYXRoa2V5cy5j CkBAIC0yOSw3ICsyOSw3IEBACiAjaW5jbHVkZSAicGFydGl0aW9uaW5nL3BhcnRib3VuZHMu aCIKICNpbmNsdWRlICJ1dGlscy9sc3lzY2FjaGUuaCIKIAotLyogQ29uc2lkZXIgcmVvcmRl cmluZyBvZiBHUk9VUCBCWSBrZXlzPyAqLworLyogQ29uc2lkZXIgcmVvcmRlcmluZyBvZiBH Uk9VUCBCWSBrZXlzICovCiBib29sCQllbmFibGVfZ3JvdXBfYnlfcmVvcmRlcmluZyA9IHRy dWU7CiAKIHN0YXRpYyBib29sIHBhdGhrZXlfaXNfcmVkdW5kYW50KFBhdGhLZXkgKm5ld19w YXRoa2V5LCBMaXN0ICpwYXRoa2V5cyk7CkBAIC0zNjIsNyArMzYyLDcgQEAgcGF0aGtleXNf Y29udGFpbmVkX2luKExpc3QgKmtleXMxLCBMaXN0ICprZXlzMikKICAqCiAgKiBSZXR1cm5z IHRoZSBudW1iZXIgb2YgR1JPVVAgQlkga2V5cyB3aXRoIGEgbWF0Y2hpbmcgcGF0aGtleS4K ICAqLwotc3RhdGljIGludAorc3RhdGljIFBhdGhLZXlJbmZvICoKIGdyb3VwX2tleXNfcmVv cmRlcl9ieV9wYXRoa2V5cyhMaXN0ICpwYXRoa2V5cywgTGlzdCAqKmdyb3VwX3BhdGhrZXlz LAogCQkJCQkJCSAgIExpc3QgKipncm91cF9jbGF1c2VzLAogCQkJCQkJCSAgIGludCBudW1f Z3JvdXBieV9wYXRoa2V5cykKQEAgLTQyMSw3ICs0MjEsMTYgQEAgZ3JvdXBfa2V5c19yZW9y ZGVyX2J5X3BhdGhrZXlzKExpc3QgKnBhdGhrZXlzLCBMaXN0ICoqZ3JvdXBfcGF0aGtleXMs CiAJKmdyb3VwX2NsYXVzZXMgPSBsaXN0X2NvbmNhdF91bmlxdWVfcHRyKG5ld19ncm91cF9j bGF1c2VzLAogCQkJCQkJCQkJCQkqZ3JvdXBfY2xhdXNlcyk7CiAKLQlyZXR1cm4gbjsKKwlp ZiAobiA+IDAgJiYKKwkJKGVuYWJsZV9pbmNyZW1lbnRhbF9zb3J0IHx8IG4gPT0gbGlzdF9s ZW5ndGgoKmdyb3VwX3BhdGhrZXlzKSkpCisJeworCQlQYXRoS2V5SW5mbyAqaW5mbyA9IG1h a2VOb2RlKFBhdGhLZXlJbmZvKTsKKwkJaW5mby0+cGF0aGtleXMgPSAqZ3JvdXBfcGF0aGtl eXM7CisJCWluZm8tPmNsYXVzZXMgPSAqZ3JvdXBfY2xhdXNlczsKKwkJcmV0dXJuIGluZm87 CisJfQorCisJcmV0dXJuIE5VTEw7CiB9CiAKIC8qCkBAIC00MzYsNyArNDQ1LDcgQEAgZ3Jv dXBfa2V5c19yZW9yZGVyX2J5X3BhdGhrZXlzKExpc3QgKnBhdGhrZXlzLCBMaXN0ICoqZ3Jv dXBfcGF0aGtleXMsCiAgKgogICogLSB0aGUgb3JpZ2luYWwgb3JkZXJpbmcsIGFzIHNwZWNp ZmllZCBieSB0aGUgR1JPVVAgQlkgY2xhdXNlLAogICogLSBHUk9VUCBCWSBrZXlzIHJlb3Jk ZXJlZCB0byBtYXRjaCAncGF0aCcgb3JkZXJpbmcgKGFzIG11Y2ggYXMgcG9zc2libGUpLAot ICogLSBHUk9VUCBCWSBrZXlzIHRvIG1hdGNoIHRhcmdldCBPUkRFUiBCWSBjbGF1c2UgKGFz IG11Y2ggYXMgcG9zc2libGUpLgorICogLSBHUk9VUCBCWSBrZXlzIHNob3VsZCBtYXRjaCB0 aGUgdGFyZ2V0IE9SREVSIEJZIGNsYXVzZSAoYXMgbXVjaCBhcyBwb3NzaWJsZSkuCiAgKi8K IExpc3QgKgogZ2V0X3VzZWZ1bF9ncm91cF9rZXlzX29yZGVyaW5ncyhQbGFubmVySW5mbyAq cm9vdCwgUGF0aCAqcGF0aCkKQEAgLTQ3NSwyMCArNDg0LDExIEBAIGdldF91c2VmdWxfZ3Jv dXBfa2V5c19vcmRlcmluZ3MoUGxhbm5lckluZm8gKnJvb3QsIFBhdGggKnBhdGgpCiAJICov CiAJaWYgKHBhdGgtPnBhdGhrZXlzKQogCXsKLQkJaW50CQkJbjsKLQotCQluID0gZ3JvdXBf a2V5c19yZW9yZGVyX2J5X3BhdGhrZXlzKHBhdGgtPnBhdGhrZXlzLCAmcGF0aGtleXMsICZj bGF1c2VzLAorCQlpbmZvID0gZ3JvdXBfa2V5c19yZW9yZGVyX2J5X3BhdGhrZXlzKHBhdGgt PnBhdGhrZXlzLCAmcGF0aGtleXMsICZjbGF1c2VzLAogCQkJCQkJCQkJCSAgIHJvb3QtPm51 bV9ncm91cGJ5X3BhdGhrZXlzKTsKIAotCQlpZiAobiA+IDAgJiYKLQkJCShlbmFibGVfaW5j cmVtZW50YWxfc29ydCB8fCBuID09IGxpc3RfbGVuZ3RoKHBhdGgtPnBhdGhrZXlzKSkpCi0J CXsKLQkJCWluZm8gPSBtYWtlTm9kZShQYXRoS2V5SW5mbyk7Ci0JCQlpbmZvLT5wYXRoa2V5 cyA9IHBhdGhrZXlzOwotCQkJaW5mby0+Y2xhdXNlcyA9IGNsYXVzZXM7Ci0KKwkJaWYgKGlu Zm8pCiAJCQlpbmZvcyA9IGxhcHBlbmQoaW5mb3MsIGluZm8pOwotCQl9CiAJfQogCiAJLyoK QEAgLTQ5NywyMSArNDk3LDEyIEBAIGdldF91c2VmdWxfZ3JvdXBfa2V5c19vcmRlcmluZ3Mo UGxhbm5lckluZm8gKnJvb3QsIFBhdGggKnBhdGgpCiAJICovCiAJaWYgKHJvb3QtPnNvcnRf cGF0aGtleXMpCiAJewotCQlpbnQJCQluOwotCi0JCW4gPSBncm91cF9rZXlzX3Jlb3JkZXJf YnlfcGF0aGtleXMocm9vdC0+c29ydF9wYXRoa2V5cywgJnBhdGhrZXlzLAorCQlpbmZvID0g Z3JvdXBfa2V5c19yZW9yZGVyX2J5X3BhdGhrZXlzKHJvb3QtPnNvcnRfcGF0aGtleXMsICZw YXRoa2V5cywKIAkJCQkJCQkJCQkgICAmY2xhdXNlcywKIAkJCQkJCQkJCQkgICByb290LT5u dW1fZ3JvdXBieV9wYXRoa2V5cyk7CiAKLQkJaWYgKG4gPiAwICYmCi0JCQkoZW5hYmxlX2lu Y3JlbWVudGFsX3NvcnQgfHwgbiA9PSBsaXN0X2xlbmd0aChwYXRoLT5wYXRoa2V5cykpKQot CQl7Ci0JCQlpbmZvID0gbWFrZU5vZGUoUGF0aEtleUluZm8pOwotCQkJaW5mby0+cGF0aGtl eXMgPSBwYXRoa2V5czsKLQkJCWluZm8tPmNsYXVzZXMgPSBjbGF1c2VzOwotCisJCWlmIChp bmZvKQogCQkJaW5mb3MgPSBsYXBwZW5kKGluZm9zLCBpbmZvKTsKLQkJfQogCX0KIAogCXJl dHVybiBpbmZvczsKQEAgLTIxNjMsMjcgKzIxNTQsMjYgQEAgdHJ1bmNhdGVfdXNlbGVzc19w YXRoa2V5cyhQbGFubmVySW5mbyAqcm9vdCwKIAkJCQkJCSAgUmVsT3B0SW5mbyAqcmVsLAog CQkJCQkJICBMaXN0ICpwYXRoa2V5cykKIHsKLQlpbnQJCQludXNlZnVsOwotCWludAkJCW51 c2VmdWwyOworCWludAkJCW51c2VmdWxfcGF0aGtleXM7CisJaW50CQkJbnVzZWZ1bF9hbHRl cm5hdGl2ZTsKKworCW51c2VmdWxfcGF0aGtleXMgPSBwYXRoa2V5c191c2VmdWxfZm9yX21l cmdpbmcocm9vdCwgcmVsLCBwYXRoa2V5cyk7CiAKLQludXNlZnVsID0gcGF0aGtleXNfdXNl ZnVsX2Zvcl9tZXJnaW5nKHJvb3QsIHJlbCwgcGF0aGtleXMpOwotCW51c2VmdWwyID0gcGF0 aGtleXNfdXNlZnVsX2Zvcl9vcmRlcmluZyhyb290LCBwYXRoa2V5cyk7Ci0JaWYgKG51c2Vm dWwyID4gbnVzZWZ1bCkKLQkJbnVzZWZ1bCA9IG51c2VmdWwyOwotCW51c2VmdWwyID0gcGF0 aGtleXNfdXNlZnVsX2Zvcl9ncm91cGluZyhyb290LCBwYXRoa2V5cyk7Ci0JaWYgKG51c2Vm dWwyID4gbnVzZWZ1bCkKLQkJbnVzZWZ1bCA9IG51c2VmdWwyOworCWlmKChudXNlZnVsX2Fs dGVybmF0aXZlID0gcGF0aGtleXNfdXNlZnVsX2Zvcl9vcmRlcmluZyhyb290LCBwYXRoa2V5 cykpID4gbnVzZWZ1bF9wYXRoa2V5cykKKwkJbnVzZWZ1bF9wYXRoa2V5cyA9IG51c2VmdWxf YWx0ZXJuYXRpdmU7CisJaWYgKChudXNlZnVsX2FsdGVybmF0aXZlID0gcGF0aGtleXNfdXNl ZnVsX2Zvcl9ncm91cGluZyhyb290LCBwYXRoa2V5cykpID4gbnVzZWZ1bF9wYXRoa2V5cykK KwkJbnVzZWZ1bF9wYXRoa2V5cyA9IG51c2VmdWxfYWx0ZXJuYXRpdmU7CiAKIAkvKgogCSAq IE5vdGU6IG5vdCBzYWZlIHRvIG1vZGlmeSBpbnB1dCBsaXN0IGRlc3RydWN0aXZlbHksIGJ1 dCB3ZSBjYW4gYXZvaWQKIAkgKiBjb3B5aW5nIHRoZSBsaXN0IGlmIHdlJ3JlIG5vdCBhY3R1 YWxseSBnb2luZyB0byBjaGFuZ2UgaXQKIAkgKi8KLQlpZiAobnVzZWZ1bCA9PSAwKQorCWlm IChudXNlZnVsX3BhdGhrZXlzID09IDApCiAJCXJldHVybiBOSUw7Ci0JZWxzZSBpZiAobnVz ZWZ1bCA9PSBsaXN0X2xlbmd0aChwYXRoa2V5cykpCisJZWxzZSBpZiAobnVzZWZ1bF9wYXRo a2V5cyA9PSBsaXN0X2xlbmd0aChwYXRoa2V5cykpCiAJCXJldHVybiBwYXRoa2V5czsKIAll bHNlCi0JCXJldHVybiBsaXN0X2NvcHlfaGVhZChwYXRoa2V5cywgbnVzZWZ1bCk7CisJCXJl dHVybiBsaXN0X2NvcHlfaGVhZChwYXRoa2V5cywgbnVzZWZ1bF9wYXRoa2V5cyk7CiB9CiAK IC8qCg== --------------d3qZmSbptV1Nl70B4UKGSICT--