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

On Tue, 18 Feb 2025 at 15:40, "songjinzhou" <tsinghualucky912(at)foxmail(dot)com> 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:
>
> -- 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;
> 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 deferrable):
>
> postgres=# \d+ 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=#
>
> 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!
>

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