Subject: Modify an incorrect regression test case in the group by key value elimination function

From: "songjinzhou" <[email-masked]>
Date: Tue, 18 Feb 2025 15:40:35 +0800

Dear hackers:&nbsp;

Two months ago, we enhanced the group by key value elimination function. This is a very useful function. When I was learning, I found a regression test case that was not suitable, as follows:

-- 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 least
-- number of columns so that we can remove more columns from the GROUP BY.
explain (costs off) select a,b,c from t3 group by a,b,c;
&nbsp; &nbsp; &nbsp; QUERY PLAN &nbsp; &nbsp; &nbsp;
----------------------
&nbsp;HashAggregate
&nbsp; &nbsp;Group Key: c
&nbsp; &nbsp;-&gt; &nbsp;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;
&nbsp; &nbsp; &nbsp; QUERY PLAN &nbsp; &nbsp; &nbsp;
----------------------
&nbsp;HashAggregate
&nbsp; &nbsp;Group Key: c
&nbsp; &nbsp;-&gt; &nbsp;Seq Scan on t3
(3 rows)

Because the table structure of t3 is defined as follows(Its PK is deferrable):

postgres=# \d+ t3
&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; Table "pg_temp_1.t3"
&nbsp;Column | &nbsp;Type &nbsp; | Collation | Nullable | Default | Storage | Compression | Stats target | Description
--------+---------+-----------+----------+---------+---------+-------------+--------------+-------------
&nbsp;a &nbsp; &nbsp; &nbsp;| integer | &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; | not null | &nbsp; &nbsp; &nbsp; &nbsp; | plain &nbsp; | &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; | &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp;|
&nbsp;b &nbsp; &nbsp; &nbsp;| integer | &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; | not null | &nbsp; &nbsp; &nbsp; &nbsp; | plain &nbsp; | &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; | &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp;|
&nbsp;c &nbsp; &nbsp; &nbsp;| integer | &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; | not null | &nbsp; &nbsp; &nbsp; &nbsp; | plain &nbsp; | &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; | &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp;|
Indexes:
&nbsp; &nbsp; "t3_pkey" PRIMARY KEY, btree (a, b) DEFERRABLE
&nbsp; &nbsp; "t3_c_uidx" UNIQUE, btree (c)
Not-null constraints:
&nbsp; &nbsp; "t3_a_not_null" NOT NULL "a"
&nbsp; &nbsp; "t3_b_not_null" NOT NULL "b"
&nbsp; &nbsp; "t3_c_not_null" NOT NULL "c"
Access method: heap

postgres=#

I think this test case does not fully reflect the original meaning. So I made a small change to this and look forward to your feedback. Thanks!

---

From: Japin Li <[email-masked]>
Date: Tue, 18 Feb 2025 16:17:25 +0800

On Tue, 18 Feb 2025 at 15:40, "songjinzhou" <[email-masked]> wrote:
> Dear hackers: 
>
> Two months ago, we enhanced the group by key value elimination function. This is a very useful function. When I was
> learning, I found a regression test case that was not suitable, as follows:
>
>       QUERY PLAN      
>  HashAggregate
>    Group Key: c
>    ->  Seq Scan on t3
>       QUERY PLAN      
>  HashAggregate
>    Group Key: c
>    ->  Seq Scan on t3
>                                           Table "pg_temp_1.t3"
>  Column |  Type   | Collation | Nullable | Default | Storage | Compression | Stats target | Description
>  a      | integer |           | not null |         | plain   |             |              |
>  b      | integer |           | not null |         | plain   |             |              |
>  c      | integer |           | not null |         | plain   |             |              |
> Indexes:
>     "t3_pkey" PRIMARY KEY, btree (a, b) DEFERRABLE
>     "t3_c_uidx" UNIQUE, btree (c)
> Not-null constraints:
>     "t3_a_not_null" NOT NULL "a"
>     "t3_b_not_null" NOT NULL "b"
>     "t3_c_not_null" NOT NULL "c"
> Access method: heap
>
> postgres=#
>

Yeah, the primary key of t3 is deferred, so only the t3_c_uidx 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 to
create it in the patch?

-- 
Regrads,
Japin Li

---

From: Zhang Mingli <[email-masked]>
Date: Tue, 18 Feb 2025 19:09:08 +0800

> > explain (costs off) select a,b,c from t3 group by a,b,c;
Hi,

Good catch.
> It seems that the t2 does not have a unique index on z, do you forget to
While there isn't an unique index, an unique constraint with a NOT NULL on that column should serve the same purpose.

  +alter table t2 alter column z set not null, add constraint t2_z_uidx unique (z);

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

Thanks for the patch.

--
Zhang Mingli
HashData

---

From: David Rowley <[email-masked]>
Date: Wed, 19 Feb 2025 00:45:23 +1300

> I think this test case does not fully reflect the original meaning. So I made a small change to this and look forward to your feedback. Thanks!

That's my mistake. Thanks for spotting it.

I looked at your patch and felt that the flow would be better if all
those tests used t2 rather than t3.  That way we don't need to add the
NOT NULL constraint to both tables, just t2. I wrote the patch to do
it that way and pushed it.

David