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

On Feb 18, 2025 at 16:17 +0800, Japin Li <japinli(at)hotmail(dot)com>, wrote:
> 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!
> >
Hi,

Good catch.
>
> 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?
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