Date: Tue, 18 Feb 2025 19:09:08 +0800
From: Zhang Mingli <zmlpostgres@gmail.com>
To: songjinzhou <tsinghualucky912@foxmail.com>, Japin Li
 <japinli@hotmail.com>
Cc: pgsql-hackers <pgsql-hackers@lists.postgresql.org>, David Rowley
 <dgrowleyml@gmail.com>
Message-ID: <c9b3ac67-bb2e-4dd8-ab63-8b343af5ec38@Spark>
In-Reply-To: <ME0P300MB04455DD194CE6F5A8CAFAD85B6FA2@ME0P300MB0445.AUSP300.PROD.OUTLOOK.COM>
References: <tencent_CD414C79D39668455DF80D35143B87634C08@qq.com>
 <ME0P300MB04455DD194CE6F5A8CAFAD85B6FA2@ME0P300MB0445.AUSP300.PROD.OUTLOOK.COM>
Subject: Re: Modify an incorrect regression test case in the group by
 key value elimination function
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="67b46a59_7cfb422_7ffc"
Archived-At: <https://www.postgresql.org/message-id/c9b3ac67-bb2e-4dd8-ab63-8b343af5ec38%40Spark>
Precedence: bulk

--67b46a59_7cfb422_7ffc
Content-Type: multipart/alternative; boundary="67b46a59_349be1a0_7ffc"

--67b46a59_349be1a0_7ffc
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable
Content-Disposition: inline

On =46eb 18, 2025 at 16:17 +0800, Japin Li <japinli=40hotmail.com>, wrote=
:
> On Tue, 18 =46eb 2025 at 15:40, =22songjinzhou=22 <tsinghualucky912=40f=
oxmail.com> wrote:
> > Dear hackers:
> >
> > Two months ago, we enhanced the group by key value elimination functi=
on. This is a very useful function. When I was
> > learning, I found a regression test case that was not suitable, as fo=
llows:
> >
> > -- When there are multiple supporting unique indexes and the GROUP BY=
 contains
> > -- columns to cover all of those, ensure we pick the index with the l=
east
> > -- number of columns so that we can remove more columns from the GROU=
P BY.
> > explain (costs off) select a,b,c from t3 group by a,b,c;
> > QUERY PLAN
> > ----------------------
> > HashAggregate
> > Group Key: c
> > -> Seq Scan on t3
> > (3 rows)
> >
> > -- As above but try ordering the columns differently to ensure we get=
 the
> > -- same result.
> > explain (costs off) select a,b,c from t3 group by c,a,b;
> > QUERY PLAN
> > ----------------------
> > HashAggregate
> > Group Key: c
> > -> Seq Scan on t3
> > (3 rows)
> >
> > Because the table structure of t3 is defined as follows(Its PK is def=
errable):
> >
> > postgres=3D=23 =5Cd+ t3
> > Table =22pg=5Ftemp=5F1.t3=22
> > Column =7C Type =7C Collation =7C Nullable =7C Default =7C Storage =7C=
 Compression =7C Stats target =7C Description
> > --------+---------+-----------+----------+---------+---------+-------=
------+--------------+-------------
> > a =7C integer =7C =7C not null =7C =7C plain =7C =7C =7C
> > b =7C integer =7C =7C not null =7C =7C plain =7C =7C =7C
> > c =7C integer =7C =7C not null =7C =7C plain =7C =7C =7C
> > Indexes:
> > =22t3=5Fpkey=22 PRIMARY KEY, btree (a, b) DE=46ERRABLE
> > =22t3=5Fc=5Fuidx=22 UNIQUE, btree (c)
> > Not-null constraints:
> > =22t3=5Fa=5Fnot=5Fnull=22 NOT NULL =22a=22
> > =22t3=5Fb=5Fnot=5Fnull=22 NOT NULL =22b=22
> > =22t3=5Fc=5Fnot=5Fnull=22 NOT NULL =22c=22
> > Access method: heap
> >
> > postgres=3D=23
> >
> > I think this test case does not fully reflect the original meaning. S=
o I made a small change to this and look forward to
> > your feedback. Thanks=21
> >
Hi,

Good catch.
>
> Yeah, the primary key of t3 is deferred, so only the t3=5Fc=5Fuidx can =
be used.
> The test case is inconsistent with comments.
>
> It seems that the t2 does not have a unique index on z, do you forget t=
o
> create it in the patch=3F
While there isn't an unique index, an unique constraint with a NOT NULL o=
n that column should serve the same purpose.

=C2=A0=C2=A0+alter table t2 alter column z set not null, add constraint t=
2=5Fz=5Fuidx unique (z);

In patch v2, I made a slight adjustment: creating a unique index on t3 wi=
ll handle multiple choice scenarios.

Thanks for the patch.

--
Zhang Mingli
HashData

--67b46a59_349be1a0_7ffc
Content-Type: text/html; charset="utf-8"
Content-Transfer-Encoding: quoted-printable
Content-Disposition: inline

<html xmlns=3D=22http://www.w3.org/1999/xhtml=22>
<head>
<title></title>
</head>
<body>
<div name=3D=22messageReplySection=22>
<div dir=3D=22auto=22>On =46eb 18, 2025 at 16:17 +0800, Japin Li &lt;japi=
nli=40hotmail.com&gt;, wrote:</div>
<blockquote style=3D=22border-left-color: rgb(26, 188, 156); margin: 5px;=
 padding-left: 10px; border-left-width: thin; border-left-style: solid;=22=
>On Tue, 18 =46eb 2025 at 15:40, =22songjinzhou=22 &lt;tsinghualucky912=40=
foxmail.com&gt; wrote:<br />
<blockquote style=3D=22border-left-color: rgb(230, 126, 34); margin: 5px;=
 padding-left: 10px; border-left-width: thin; border-left-style: solid;=22=
>Dear hackers:<br />
<br />
Two months ago, we enhanced the group by key value elimination function. =
This is a very useful function. When I was<br />
learning, I found a regression test case that was not suitable, as follow=
s:<br />
<br />
-- When there are multiple supporting unique indexes and the GROUP BY con=
tains<br />
-- columns to cover all of those, ensure we pick the index with the least=
<br />
-- number of columns so that we can remove more columns from the GROUP BY=
.<br />
explain (costs off) select a,b,c from t3 group by a,b,c;<br />
QUERY PLAN<br />
----------------------<br />
HashAggregate<br />
Group Key: c<br />
-&gt; Seq Scan on t3<br />
(3 rows)<br />
<br />
-- As above but try ordering the columns differently to ensure we get the=
<br />
-- same result.<br />
explain (costs off) select a,b,c from t3 group by c,a,b;<br />
QUERY PLAN<br />
----------------------<br />
HashAggregate<br />
Group Key: c<br />
-&gt; Seq Scan on t3<br />
(3 rows)<br />
<br />
Because the table structure of t3 is defined as follows(Its PK is deferra=
ble):<br />
<br />
postgres=3D=23 =5Cd+ t3<br />
Table =22pg=5Ftemp=5F1.t3=22<br />
Column =7C Type =7C Collation =7C Nullable =7C Default =7C Storage =7C Co=
mpression =7C Stats target =7C Description<br />
--------+---------+-----------+----------+---------+---------+-----------=
--+--------------+-------------<br />
a =7C integer =7C =7C not null =7C =7C plain =7C =7C =7C<br />
b =7C integer =7C =7C not null =7C =7C plain =7C =7C =7C<br />
c =7C integer =7C =7C not null =7C =7C plain =7C =7C =7C<br />
Indexes:<br />
=22t3=5Fpkey=22 PRIMARY KEY, btree (a, b) DE=46ERRABLE<br />
=22t3=5Fc=5Fuidx=22 UNIQUE, btree (c)<br />
Not-null constraints:<br />
=22t3=5Fa=5Fnot=5Fnull=22 NOT NULL =22a=22<br />
=22t3=5Fb=5Fnot=5Fnull=22 NOT NULL =22b=22<br />
=22t3=5Fc=5Fnot=5Fnull=22 NOT NULL =22c=22<br />
Access method: heap<br />
<br />
postgres=3D=23<br />
<br />
I think this test case does not fully reflect the original meaning. So I =
made a small change to this and look forward to<br />
your feedback. Thanks=21<br />
<br /></blockquote>
</blockquote>
<div dir=3D=22auto=22>Hi,<br />
<br />
Good catch.</div>
<blockquote style=3D=22border-left-color: rgb(26, 188, 156); margin: 5px;=
 padding-left: 10px; border-left-width: thin; border-left-style: solid;=22=
><br />
Yeah, the primary key of t3 is deferred, so only the t3=5Fc=5Fuidx can be=
 used.<br />
The test case is inconsistent with comments.<br />
<br />
It seems that the t2 does not have a unique index on z, do you forget to<=
br />
create it in the patch=3F</blockquote>
<div dir=3D=22auto=22>While there isn't an unique index, an unique constr=
aint with a NOT NULL on that column should serve the same purpose.<br />
&=23160;<br />
&=23160;&=23160;+alter table t2 alter column z set not null, add constrai=
nt t2=5Fz=5Fuidx unique (z);<br />
<br />
In patch v2, I made a slight adjustment: creating a unique index on t3 wi=
ll handle multiple choice scenarios.<br />
<br />
Thanks for the patch.<br />
<br />
--<br />
Zhang Mingli<br />
HashData</div>
</div>
</body>
</html>

--67b46a59_349be1a0_7ffc--

--67b46a59_7cfb422_7ffc
Content-Type: application/octet-stream
Content-Transfer-Encoding: base64
Content-Disposition: attachment; 
 filename="v2-0001-Modify-an-incorrect-regression-test-case-in-the-grou.patch"

RnJvbSBjOTJlYmE4OTRiZTNmNGQ2YjIzODkxN2U2MTc0MzczZjE0MDRhMGZjIE1vbiBTZXAgMTcg
MDA6MDA6MDAgMjAwMQpGcm9tOiBzb25namluemhvdSA8dHNpbmdodWFsdWNreTkxMkBmb3htYWls
LmNvbT4KRGF0ZTogVHVlLCAxOCBGZWIgMjAyNSAxOTowMjozNiArMDgwMApTdWJqZWN0OiBbUEFU
Q0hdIE1vZGlmeSBhbiBpbmNvcnJlY3QgcmVncmVzc2lvbiB0ZXN0IGNhc2UgaW4gdGhlIGdyb3Vw
IGJ5IGtleQogdmFsdWUgZWxpbWluYXRpb24gZnVuY3Rpb24KCi0tLQogc3JjL3Rlc3QvcmVncmVz
cy9leHBlY3RlZC9hZ2dyZWdhdGVzLm91dCB8IDIgKysKIHNyYy90ZXN0L3JlZ3Jlc3Mvc3FsL2Fn
Z3JlZ2F0ZXMuc3FsICAgICAgfCAyICsrCiAyIGZpbGVzIGNoYW5nZWQsIDQgaW5zZXJ0aW9ucygr
KQoKZGlmZiAtLWdpdCBhL3NyYy90ZXN0L3JlZ3Jlc3MvZXhwZWN0ZWQvYWdncmVnYXRlcy5vdXQg
Yi9zcmMvdGVzdC9yZWdyZXNzL2V4cGVjdGVkL2FnZ3JlZ2F0ZXMub3V0CmluZGV4IGYyZmI2NjM4
OGMuLmNhOTViYTEzNDMgMTAwNjQ0Ci0tLSBhL3NyYy90ZXN0L3JlZ3Jlc3MvZXhwZWN0ZWQvYWdn
cmVnYXRlcy5vdXQKKysrIGIvc3JjL3Rlc3QvcmVncmVzcy9leHBlY3RlZC9hZ2dyZWdhdGVzLm91
dApAQCAtMTQ3Miw2ICsxNDcyLDcgQEAgZXhwbGFpbiAoY29zdHMgb2ZmKSBzZWxlY3QgYixjIGZy
b20gdDMgZ3JvdXAgYnkgYixjOwogLS0gV2hlbiB0aGVyZSBhcmUgbXVsdGlwbGUgc3VwcG9ydGlu
ZyB1bmlxdWUgaW5kZXhlcyBhbmQgdGhlIEdST1VQIEJZIGNvbnRhaW5zCiAtLSBjb2x1bW5zIHRv
IGNvdmVyIGFsbCBvZiB0aG9zZSwgZW5zdXJlIHdlIHBpY2sgdGhlIGluZGV4IHdpdGggdGhlIGxl
YXN0CiAtLSBudW1iZXIgb2YgY29sdW1ucyBzbyB0aGF0IHdlIGNhbiByZW1vdmUgbW9yZSBjb2x1
bW5zIGZyb20gdGhlIEdST1VQIEJZLgorY3JlYXRlIHVuaXF1ZSBpbmRleCB0M19iX2NfdWlkeCBv
biB0MyhiLCBjKTsKIGV4cGxhaW4gKGNvc3RzIG9mZikgc2VsZWN0IGEsYixjIGZyb20gdDMgZ3Jv
dXAgYnkgYSxiLGM7CiAgICAgICBRVUVSWSBQTEFOICAgICAgCiAtLS0tLS0tLS0tLS0tLS0tLS0t
LS0tCkBAIC0xNDkwLDYgKzE0OTEsNyBAQCBleHBsYWluIChjb3N0cyBvZmYpIHNlbGVjdCBhLGIs
YyBmcm9tIHQzIGdyb3VwIGJ5IGMsYSxiOwogICAgLT4gIFNlcSBTY2FuIG9uIHQzCiAoMyByb3dz
KQogCitkcm9wIGluZGV4IHQzX2JfY191aWR4OwogLS0gRW5zdXJlIHdlIGRvbid0IHVzZSBhIHBh
cnRpYWwgaW5kZXggYXMgcHJvb2Ygb2YgZnVuY3Rpb25hbCBkZXBlbmRlbmN5CiBkcm9wIGluZGV4
IHQzX2NfdWlkeDsKIGNyZWF0ZSBpbmRleCB0M19jX3VpZHggb24gdDMgKGMpIHdoZXJlIGMgPiAw
OwpkaWZmIC0tZ2l0IGEvc3JjL3Rlc3QvcmVncmVzcy9zcWwvYWdncmVnYXRlcy5zcWwgYi9zcmMv
dGVzdC9yZWdyZXNzL3NxbC9hZ2dyZWdhdGVzLnNxbAppbmRleCA3NzE2OGJjYzc0Li44YWNkNjRk
MzRlIDEwMDY0NAotLS0gYS9zcmMvdGVzdC9yZWdyZXNzL3NxbC9hZ2dyZWdhdGVzLnNxbAorKysg
Yi9zcmMvdGVzdC9yZWdyZXNzL3NxbC9hZ2dyZWdhdGVzLnNxbApAQCAtNTIwLDEyICs1MjAsMTQg
QEAgZXhwbGFpbiAoY29zdHMgb2ZmKSBzZWxlY3QgYixjIGZyb20gdDMgZ3JvdXAgYnkgYixjOwog
LS0gV2hlbiB0aGVyZSBhcmUgbXVsdGlwbGUgc3VwcG9ydGluZyB1bmlxdWUgaW5kZXhlcyBhbmQg
dGhlIEdST1VQIEJZIGNvbnRhaW5zCiAtLSBjb2x1bW5zIHRvIGNvdmVyIGFsbCBvZiB0aG9zZSwg
ZW5zdXJlIHdlIHBpY2sgdGhlIGluZGV4IHdpdGggdGhlIGxlYXN0CiAtLSBudW1iZXIgb2YgY29s
dW1ucyBzbyB0aGF0IHdlIGNhbiByZW1vdmUgbW9yZSBjb2x1bW5zIGZyb20gdGhlIEdST1VQIEJZ
LgorY3JlYXRlIHVuaXF1ZSBpbmRleCB0M19iX2NfdWlkeCBvbiB0MyhiLCBjKTsKIGV4cGxhaW4g
KGNvc3RzIG9mZikgc2VsZWN0IGEsYixjIGZyb20gdDMgZ3JvdXAgYnkgYSxiLGM7CiAKIC0tIEFz
IGFib3ZlIGJ1dCB0cnkgb3JkZXJpbmcgdGhlIGNvbHVtbnMgZGlmZmVyZW50bHkgdG8gZW5zdXJl
IHdlIGdldCB0aGUKIC0tIHNhbWUgcmVzdWx0LgogZXhwbGFpbiAoY29zdHMgb2ZmKSBzZWxlY3Qg
YSxiLGMgZnJvbSB0MyBncm91cCBieSBjLGEsYjsKIAorZHJvcCBpbmRleCB0M19iX2NfdWlkeDsK
IC0tIEVuc3VyZSB3ZSBkb24ndCB1c2UgYSBwYXJ0aWFsIGluZGV4IGFzIHByb29mIG9mIGZ1bmN0
aW9uYWwgZGVwZW5kZW5jeQogZHJvcCBpbmRleCB0M19jX3VpZHg7CiBjcmVhdGUgaW5kZXggdDNf
Y191aWR4IG9uIHQzIChjKSB3aGVyZSBjID4gMDsKLS0gCjIuMzQuMQoK

--67b46a59_7cfb422_7ffc--