Thread
Commits
Same data as JSON:
GET /api/v1/messages/:b64id/commits
the thread's linked commits as JSON, with link sources.
API reference →
-
Fix poorly written regression test
- 593509202f66 18.0 landed
-
Modify an incorrect regression test case in the group by key value elimination function
songjinzhou <tsinghualucky912@foxmail.com> — 2025-02-18T07:40:35Z
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!
-
Re: Modify an incorrect regression test case in the group by key value elimination function
Japin Li <japinli@hotmail.com> — 2025-02-18T08:17:25Z
On Tue, 18 Feb 2025 at 15:40, "songjinzhou" <tsinghualucky912@foxmail.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
-
Re: Modify an incorrect regression test case in the group by key value elimination function
Zhang Mingli <zmlpostgres@gmail.com> — 2025-02-18T11:09:08Z
On Feb 18, 2025 at 16:17 +0800, Japin Li <japinli@hotmail.com>, wrote: > On Tue, 18 Feb 2025 at 15:40, "songjinzhou" <tsinghualucky912@foxmail.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
-
Re: Modify an incorrect regression test case in the group by key value elimination function
David Rowley <dgrowleyml@gmail.com> — 2025-02-18T11:45:23Z
On Tue, 18 Feb 2025 at 20:40, songjinzhou <tsinghualucky912@foxmail.com> wrote: > 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: > Because the table structure of t3 is defined as follows(Its PK is deferrable): > 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