Content-Type: multipart/mixed; boundary="------------K00fQCkeJqxcZ1weUHMDxrka"
Message-ID: <208b2729-1d8e-48d2-8a4d-ce592be0bde4@gmail.com>
Date: Thu, 18 Jul 2024 12:18:24 +0700
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: Expand applicability of aggregate's sortop optimization
To: Matthias van de Meent <boekewurm+postgres@gmail.com>
Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>,
 David Rowley <dgrowleyml@gmail.com>, ilmari@ilmari.org
References: <CAEze2Wg-+EV4HdbQiut7X3KQd39xwmrpV4CeCmoJFFjH8cGdhw@mail.gmail.com>
 <24c11397-d9a1-4aac-a999-a79352a6a35b@gmail.com>
 <CAEze2Wj44i8eiLFCpv2wW+u_3af-wGcWnA1iuPaMVy9LOmSKFw@mail.gmail.com>
Content-Language: en-US
From: Andrei Lepikhov <lepihov@gmail.com>
In-Reply-To: <CAEze2Wj44i8eiLFCpv2wW+u_3af-wGcWnA1iuPaMVy9LOmSKFw@mail.gmail.com>
Archived-At: <https://www.postgresql.org/message-id/208b2729-1d8e-48d2-8a4d-ce592be0bde4%40gmail.com>
Precedence: bulk

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

On 17/7/2024 16:33, Matthias van de Meent wrote:
> On Wed, 17 Jul 2024 at 05:29, Andrei Lepikhov <lepihov@gmail.com> wrote:
>> As I see, the code:
>> aggsortop = fetch_agg_sort_op(aggref->aggfnoid);
>> if (!OidIsValid(aggsortop))
>>     return false;                /* not a MIN/MAX aggregate */
>>
>> used twice and can be evaluated earlier to avoid duplicated code.
> 
> The code is structured like this to make sure we only start accessing
> catalogs once we know that all other reasons to bail out from this
> optimization indicate we can apply the opimization. You'll notice that
> I've tried to put the cheapest checks that only use caller-supplied
> information first, and catalog accesses only after that.
After additional research I think I get the key misunderstanding why you 
did so:
As I see, the checks:
if (list_length(aggref->aggorder) > 1)
   return false;
if (orderClause->tleSortGroupRef != curTarget->ressortgroupref)
   return false;

not needed at all. You already have check:
if (list_length(aggref->args) != 1)
and this tells us, that if we have ordering like MIN(x ORDER BY <smth>), 
this <smth> ordering contains only aggregate argument x. Because if it 
contained some expression, the transformAggregateCall() would add this 
expression to agg->args by calling the transformSortClause() routine.
The tleSortGroupRef is just exactly ressortgroupref - no need to recheck 
it one more time. Of course, it is suitable only for MIN/MAX aggregates, 
but we discuss only them right now. Am I wrong?
If you want, you can place it as assertions (see the diff in attachment).

-- 
regards, Andrei Lepikhov

--------------K00fQCkeJqxcZ1weUHMDxrka
Content-Type: text/plain; charset=UTF-8; name="rewritten-patch-v2.diff"
Content-Disposition: attachment; filename="rewritten-patch-v2.diff"
Content-Transfer-Encoding: base64

ZGlmZiAtLWdpdCBhL3NyYy9iYWNrZW5kL29wdGltaXplci9wbGFuL3BsYW5hZ2cuYyBiL3Ny
Yy9iYWNrZW5kL29wdGltaXplci9wbGFuL3BsYW5hZ2cuYwppbmRleCBhZmI1NDQ1Yjc3Li5l
MGNiZTVjOTIzIDEwMDY0NAotLS0gYS9zcmMvYmFja2VuZC9vcHRpbWl6ZXIvcGxhbi9wbGFu
YWdnLmMKKysrIGIvc3JjL2JhY2tlbmQvb3B0aW1pemVyL3BsYW4vcGxhbmFnZy5jCkBAIC0y
NTMsNiArMjUzLDIwIEBAIGNhbl9taW5tYXhfYWdncyhQbGFubmVySW5mbyAqcm9vdCwgTGlz
dCAqKmNvbnRleHQpCiAJCWlmIChsaXN0X2xlbmd0aChhZ2dyZWYtPmFyZ3MpICE9IDEpCiAJ
CQlyZXR1cm4gZmFsc2U7CQkvKiBpdCBjb3VsZG4ndCBiZSBNSU4vTUFYICovCiAKKwkJLyoK
KwkJICogV2UgbWlnaHQgaW1wbGVtZW50IHRoZSBvcHRpbWl6YXRpb24gd2hlbiBhIEZJTFRF
UiBjbGF1c2UgaXMgcHJlc2VudAorCQkgKiBieSBhZGRpbmcgdGhlIGZpbHRlciB0byB0aGUg
cXVhbHMgb2YgdGhlIGdlbmVyYXRlZCBzdWJxdWVyeS4gIEZvcgorCQkgKiBub3csIGp1c3Qg
cHVudC4KKwkJICovCisJCWlmIChhZ2dyZWYtPmFnZ2ZpbHRlciAhPSBOVUxMKQorCQkJcmV0
dXJuIGZhbHNlOworCisJCWN1clRhcmdldCA9IChUYXJnZXRFbnRyeSAqKSBsaW5pdGlhbChh
Z2dyZWYtPmFyZ3MpOworCisJCWFnZ3NvcnRvcCA9IGZldGNoX2FnZ19zb3J0X29wKGFnZ3Jl
Zi0+YWdnZm5vaWQpOworCQlpZiAoIU9pZElzVmFsaWQoYWdnc29ydG9wKSkKKwkJCXJldHVy
biBmYWxzZTsJCS8qIG5vdCBhIE1JTi9NQVggYWdncmVnYXRlICovCisKIAkJLyoKIAkJICog
T1JERVIgQlkgaXMgdXN1YWxseSBpcnJlbGV2YW50IGZvciBNSU4vTUFYLCBidXQgaXQgY2Fu
IGNoYW5nZSB0aGUKIAkJICogb3V0Y29tZSBpZiB0aGUgYWdnc29ydG9wJ3Mgb3BlcmF0b3Ig
Y2xhc3MgcmVjb2duaXplcyBub24taWRlbnRpY2FsCkBAIC0yNjcsMjIgKzI4MSwzNSBAQCBj
YW5fbWlubWF4X2FnZ3MoUGxhbm5lckluZm8gKnJvb3QsIExpc3QgKipjb250ZXh0KQogCQkg
KiBxdWlja2x5LgogCQkgKi8KIAkJaWYgKGFnZ3JlZi0+YWdnb3JkZXIgIT0gTklMKQotCQkJ
cmV0dXJuIGZhbHNlOwotCQkvKiBub3RlOiB3ZSBkbyBub3QgY2FyZSBpZiBESVNUSU5DVCBp
cyBtZW50aW9uZWQgLi4uICovCisJCXsKKwkJCVNvcnRHcm91cENsYXVzZSAqb3JkZXJDbGF1
c2U7CiAKLQkJLyoKLQkJICogV2UgbWlnaHQgaW1wbGVtZW50IHRoZSBvcHRpbWl6YXRpb24g
d2hlbiBhIEZJTFRFUiBjbGF1c2UgaXMgcHJlc2VudAotCQkgKiBieSBhZGRpbmcgdGhlIGZp
bHRlciB0byB0aGUgcXVhbHMgb2YgdGhlIGdlbmVyYXRlZCBzdWJxdWVyeS4gIEZvcgotCQkg
KiBub3csIGp1c3QgcHVudC4KLQkJICovCi0JCWlmIChhZ2dyZWYtPmFnZ2ZpbHRlciAhPSBO
VUxMKQotCQkJcmV0dXJuIGZhbHNlOworCQkJLyoKKwkJCSAqIElmIHRoZSBvcmRlciBjbGF1
c2UgaXMgdGhlIHNhbWUgY29sdW1uIGFzIHRoZSBvbmUgd2UncmUKKwkJCSAqIGFnZ3JlZ2F0
aW5nLCB3ZSBjYW4gc3RpbGwgdXNlIHRoZSBpbmRleDogSXQgaXMgdW5kZWZpbmVkIHdoaWNo
CisJCQkgKiB2YWx1ZSBpcyBNSU4oKSBvciBNQVgoKSwgYXMgd2VsbCBhcyB3aGljaCB2YWx1
ZSBpcyBmaXJzdCBvcgorCQkJICogbGFzdCB3aGVuIHNvcnRlZC4gU28sIHdlIGNhbiBzdGls
bCB1c2UgdGhlIGluZGV4IElGRiB0aGUKKwkJCSAqIGFnZ3JlZ2F0ZWQgZXhwcmVzc2lvbiBl
cXVhbHMgdGhlIGV4cHJlc3Npb24gdXNlZCBpbiB0aGUKKwkJCSAqIG9yZGVyaW5nIG9wZXJh
dGlvbi4KKwkJCSAqLwogCi0JCWFnZ3NvcnRvcCA9IGZldGNoX2FnZ19zb3J0X29wKGFnZ3Jl
Zi0+YWdnZm5vaWQpOwotCQlpZiAoIU9pZElzVmFsaWQoYWdnc29ydG9wKSkKLQkJCXJldHVy
biBmYWxzZTsJCS8qIG5vdCBhIE1JTi9NQVggYWdncmVnYXRlICovCisJCQkvKgorCQkJICog
V2Ugb25seSBhY2NlcHQgYSBzaW5nbGUgYXJndW1lbnQgdG8gbWluL21heCBhZ2dyZWdhdGVz
LAorCQkJICogb3JkZXJpbmdzIHRoYXQgaGF2ZSBtb3JlIGNsYXVzZXMgd29uJ3QgcHJvdmlk
ZSBjb3JyZWN0IHJlc3VsdHMuCisJCQkgKi8KKwkJCUFzc2VydChsaXN0X2xlbmd0aChhZ2dy
ZWYtPmFnZ29yZGVyKSA9PSAxKTsKKworCQkJb3JkZXJDbGF1c2UgPSBjYXN0Tm9kZShTb3J0
R3JvdXBDbGF1c2UsIGxpbml0aWFsKGFnZ3JlZi0+YWdnb3JkZXIpKTsKKworCQkJQXNzZXJ0
KG9yZGVyQ2xhdXNlLT50bGVTb3J0R3JvdXBSZWYgPT0gY3VyVGFyZ2V0LT5yZXNzb3J0Z3Jv
dXByZWYpOworCisJCQlpZiAob3JkZXJDbGF1c2UtPnNvcnRvcCAhPSBhZ2dzb3J0b3AgJiYK
KwkJCQlvcmRlckNsYXVzZS0+c29ydG9wICE9IGdldF9jb21tdXRhdG9yKGFnZ3NvcnRvcCkp
CisJCQkJcmV0dXJuIGZhbHNlOworCQl9CisKKwkJLyogbm90ZTogd2UgZG8gbm90IGNhcmUg
aWYgRElTVElOQ1QgaXMgbWVudGlvbmVkIC4uLiAqLwogCi0JCWN1clRhcmdldCA9IChUYXJn
ZXRFbnRyeSAqKSBsaW5pdGlhbChhZ2dyZWYtPmFyZ3MpOwogCiAJCWlmIChjb250YWluX211
dGFibGVfZnVuY3Rpb25zKChOb2RlICopIGN1clRhcmdldC0+ZXhwcikpCiAJCQlyZXR1cm4g
ZmFsc2U7CQkvKiBub3QgcG90ZW50aWFsbHkgaW5kZXhhYmxlICovCmRpZmYgLS1naXQgYS9z
cmMvdGVzdC9yZWdyZXNzL2V4cGVjdGVkL2FnZ3JlZ2F0ZXMub3V0IGIvc3JjL3Rlc3QvcmVn
cmVzcy9leHBlY3RlZC9hZ2dyZWdhdGVzLm91dAppbmRleCBhNTU5NmFiMjEwLi41ZDM3NTEw
ZDY0IDEwMDY0NAotLS0gYS9zcmMvdGVzdC9yZWdyZXNzL2V4cGVjdGVkL2FnZ3JlZ2F0ZXMu
b3V0CisrKyBiL3NyYy90ZXN0L3JlZ3Jlc3MvZXhwZWN0ZWQvYWdncmVnYXRlcy5vdXQKQEAg
LTEwMDMsNiArMTAwMyw3MSBAQCBzZWxlY3QgbWF4KHVuaXF1ZTEpIGZyb20gdGVuazEgd2hl
cmUgdW5pcXVlMSA+IDQyOwogIDk5OTkKICgxIHJvdykKIAorLS0gV2hlbiBzb3J0aW5nIG9u
IHRoZSBjb2x1bW4gdGhhdCdzIGJlaW5nIGFnZ3JlZ2F0ZWQsIGluZGV4ZXMgY2FuIGFsc28g
YmUKKy0tIHVzZWQsIGJ1dCBvbmx5IHdoZW4gdGhlIGFnZ3JlZ2F0ZSdzIG9wZXJhdG9yIGhh
cyB0aGUgc2FtZSBvcmRlcmluZyBiZWhhdmlvcgorLS0gYXMgdGhlIE9SREVSIEJZLWNsYXVz
ZSwgaS5lLiBpZiBpdCBpcyBpbiB0aGUgc2FtZSBidHJlZSBvcGNsYXNzIGFzIHRoZSBvbmUK
Ky0tIGNob3NlbiBmb3IgdGhlIE9SREVSIEJZIGNsYXVzZS4KK2V4cGxhaW4gKGNvc3RzIG9m
ZikKKyAgc2VsZWN0IG1pbih1bmlxdWUxIE9SREVSIEJZIHVuaXF1ZTEgQVNDIE5VTExTIExB
U1QpIGZyb20gdGVuazE7CisgICAgICAgICAgICAgICAgICAgICAgICAgUVVFUlkgUExBTiAg
ICAgICAgICAgICAgICAgICAgICAgICAKKy0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0t
LS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLQorIFJlc3VsdAorICAgSW5pdFBsYW4g
MQorICAgICAtPiAgTGltaXQKKyAgICAgICAgICAgLT4gIEluZGV4IE9ubHkgU2NhbiB1c2lu
ZyB0ZW5rMV91bmlxdWUxIG9uIHRlbmsxCisgICAgICAgICAgICAgICAgIEluZGV4IENvbmQ6
ICh1bmlxdWUxIElTIE5PVCBOVUxMKQorKDUgcm93cykKKworc2VsZWN0IG1pbih1bmlxdWUx
IE9SREVSIEJZIHVuaXF1ZTEgQVNDIE5VTExTIExBU1QpIGZyb20gdGVuazE7CisgbWluIAor
LS0tLS0KKyAgIDAKKygxIHJvdykKKworZXhwbGFpbiAoY29zdHMgb2ZmKQorICBzZWxlY3Qg
bWF4KHVuaXF1ZTEgT1JERVIgQlkgdW5pcXVlMSBVU0lORyA8KSBmcm9tIHRlbmsxOworICAg
ICAgICAgICAgICAgICAgICAgICAgICAgICBRVUVSWSBQTEFOICAgICAgICAgICAgICAgICAg
ICAgICAgICAgICAgCistLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0t
LS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0KKyBSZXN1bHQKKyAgIEluaXRQbGFuIDEK
KyAgICAgLT4gIExpbWl0CisgICAgICAgICAgIC0+ICBJbmRleCBPbmx5IFNjYW4gQmFja3dh
cmQgdXNpbmcgdGVuazFfdW5pcXVlMSBvbiB0ZW5rMQorICAgICAgICAgICAgICAgICBJbmRl
eCBDb25kOiAodW5pcXVlMSBJUyBOT1QgTlVMTCkKKyg1IHJvd3MpCisKK3NlbGVjdCBtYXgo
dW5pcXVlMSBPUkRFUiBCWSB1bmlxdWUxIFVTSU5HIDwpIGZyb20gdGVuazE7CisgbWF4ICAK
Ky0tLS0tLQorIDk5OTkKKygxIHJvdykKKworZXhwbGFpbiAoY29zdHMgb2ZmKQorICBzZWxl
Y3QgbWF4KHVuaXF1ZTEgT1JERVIgQlkgdGVudGhvdXMpIGZyb20gdGVuazE7CisgICAgICAg
ICAgUVVFUlkgUExBTiAgICAgICAgICAgCistLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0t
LS0tCisgQWdncmVnYXRlCisgICAtPiAgU29ydAorICAgICAgICAgU29ydCBLZXk6IHRlbnRo
b3VzCisgICAgICAgICAtPiAgU2VxIFNjYW4gb24gdGVuazEKKyg0IHJvd3MpCisKK3NlbGVj
dCBtYXgodW5pcXVlMSBPUkRFUiBCWSB0ZW50aG91cykgZnJvbSB0ZW5rMTsKKyBtYXggIAor
LS0tLS0tCisgOTk5OQorKDEgcm93KQorCistLSAgQnV0IGV2ZW4gdGhlbiwgdGhlIGluZGV4
IGNhbid0IGJlIHVzZWQgaWYgd2Ugb3JkZXIgYnkgbXVsdGlwbGUgY29sdW1ucy4KK2V4cGxh
aW4gKGNvc3RzIG9mZikKKyAgc2VsZWN0IG1heCh1bmlxdWUxIE9SREVSIEJZIHVuaXF1ZTEs
IHRlbnRob3VzKSBmcm9tIHRlbmsxOworICAgICAgICAgICAgIFFVRVJZIFBMQU4gICAgICAg
ICAgICAgIAorLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLQorIEFnZ3Jl
Z2F0ZQorICAgLT4gIFNvcnQKKyAgICAgICAgIFNvcnQgS2V5OiB1bmlxdWUxLCB0ZW50aG91
cworICAgICAgICAgLT4gIFNlcSBTY2FuIG9uIHRlbmsxCisoNCByb3dzKQorCiAtLSB0aGUg
cGxhbm5lciBtYXkgY2hvb3NlIGEgZ2VuZXJpYyBhZ2dyZWdhdGUgaGVyZSBpZiBwYXJhbGxl
bCBxdWVyeSBpcwogLS0gZW5hYmxlZCwgc2luY2UgdGhhdCBwbGFuIHdpbGwgYmUgcGFyYWxs
ZWwgc2FmZSBhbmQgdGhlICJvcHRpbWl6ZWQiCiAtLSBwbGFuLCB3aGljaCBoYXMgYWxtb3N0
IGlkZW50aWNhbCBjb3N0LCB3aWxsIG5vdCBiZS4gIHdlIHdhbnQgdG8gdGVzdApkaWZmIC0t
Z2l0IGEvc3JjL3Rlc3QvcmVncmVzcy9zcWwvYWdncmVnYXRlcy5zcWwgYi9zcmMvdGVzdC9y
ZWdyZXNzL3NxbC9hZ2dyZWdhdGVzLnNxbAppbmRleCBjYTZkMWJjZmI3Li41Y2RmMzM2ZmMw
IDEwMDY0NAotLS0gYS9zcmMvdGVzdC9yZWdyZXNzL3NxbC9hZ2dyZWdhdGVzLnNxbAorKysg
Yi9zcmMvdGVzdC9yZWdyZXNzL3NxbC9hZ2dyZWdhdGVzLnNxbApAQCAtMzY4LDYgKzM2OCwy
NCBAQCBleHBsYWluIChjb3N0cyBvZmYpCiAgIHNlbGVjdCBtYXgodW5pcXVlMSkgZnJvbSB0
ZW5rMSB3aGVyZSB1bmlxdWUxID4gNDI7CiBzZWxlY3QgbWF4KHVuaXF1ZTEpIGZyb20gdGVu
azEgd2hlcmUgdW5pcXVlMSA+IDQyOwogCistLSBXaGVuIHNvcnRpbmcgb24gdGhlIGNvbHVt
biB0aGF0J3MgYmVpbmcgYWdncmVnYXRlZCwgaW5kZXhlcyBjYW4gYWxzbyBiZQorLS0gdXNl
ZCwgYnV0IG9ubHkgd2hlbiB0aGUgYWdncmVnYXRlJ3Mgb3BlcmF0b3IgaGFzIHRoZSBzYW1l
IG9yZGVyaW5nIGJlaGF2aW9yCistLSBhcyB0aGUgT1JERVIgQlktY2xhdXNlLCBpLmUuIGlm
IGl0IGlzIGluIHRoZSBzYW1lIGJ0cmVlIG9wY2xhc3MgYXMgdGhlIG9uZQorLS0gY2hvc2Vu
IGZvciB0aGUgT1JERVIgQlkgY2xhdXNlLgorZXhwbGFpbiAoY29zdHMgb2ZmKQorICBzZWxl
Y3QgbWluKHVuaXF1ZTEgT1JERVIgQlkgdW5pcXVlMSBBU0MgTlVMTFMgTEFTVCkgZnJvbSB0
ZW5rMTsKK3NlbGVjdCBtaW4odW5pcXVlMSBPUkRFUiBCWSB1bmlxdWUxIEFTQyBOVUxMUyBM
QVNUKSBmcm9tIHRlbmsxOworZXhwbGFpbiAoY29zdHMgb2ZmKQorICBzZWxlY3QgbWF4KHVu
aXF1ZTEgT1JERVIgQlkgdW5pcXVlMSBVU0lORyA8KSBmcm9tIHRlbmsxOworc2VsZWN0IG1h
eCh1bmlxdWUxIE9SREVSIEJZIHVuaXF1ZTEgVVNJTkcgPCkgZnJvbSB0ZW5rMTsKK2V4cGxh
aW4gKGNvc3RzIG9mZikKKyAgc2VsZWN0IG1heCh1bmlxdWUxIE9SREVSIEJZIHRlbnRob3Vz
KSBmcm9tIHRlbmsxOworc2VsZWN0IG1heCh1bmlxdWUxIE9SREVSIEJZIHRlbnRob3VzKSBm
cm9tIHRlbmsxOworCistLSAgQnV0IGV2ZW4gdGhlbiwgdGhlIGluZGV4IGNhbid0IGJlIHVz
ZWQgaWYgd2Ugb3JkZXIgYnkgbXVsdGlwbGUgY29sdW1ucy4KK2V4cGxhaW4gKGNvc3RzIG9m
ZikKKyAgc2VsZWN0IG1heCh1bmlxdWUxIE9SREVSIEJZIHVuaXF1ZTEsIHRlbnRob3VzKSBm
cm9tIHRlbmsxOworCiAtLSB0aGUgcGxhbm5lciBtYXkgY2hvb3NlIGEgZ2VuZXJpYyBhZ2dy
ZWdhdGUgaGVyZSBpZiBwYXJhbGxlbCBxdWVyeSBpcwogLS0gZW5hYmxlZCwgc2luY2UgdGhh
dCBwbGFuIHdpbGwgYmUgcGFyYWxsZWwgc2FmZSBhbmQgdGhlICJvcHRpbWl6ZWQiCiAtLSBw
bGFuLCB3aGljaCBoYXMgYWxtb3N0IGlkZW50aWNhbCBjb3N0LCB3aWxsIG5vdCBiZS4gIHdl
IHdhbnQgdG8gdGVzdAo=

--------------K00fQCkeJqxcZ1weUHMDxrka--