pg.ddx.io pgsql-bugs@postgresql.org mailing list archive
help / color / mirror / Atom feedBUG #18568: BUG: Result wrong when do group by on partition table!
30+ messages / 5 participants
[nested] [flat]
* BUG #18568: BUG: Result wrong when do group by on partition table!
@ 2024-08-06 06:23 PG Bug reporting form <noreply@postgresql.org>
0 siblings, 2 replies; 30+ messages in thread
From: PG Bug reporting form @ 2024-08-06 06:23 UTC (permalink / raw)
To: pgsql-bugs@lists.postgresql.org; +Cc: 1105066510@qq.com
The following bug has been logged on the website:
Bug reference: 18568
Logged by: Webbo Han
Email address: 1105066510@qq.com
PostgreSQL version: 16.3
Operating system: centos 7.6
Description:
First, we create one case-insensitive collation use ICU:
```sql
CREATE COLLATION case_insensitive (
provider = icu,
locale = 'und-u-ks-level2',
deterministic = false
);
```
Then, we create the partition table, meanwhile we set the collation of
column c to `case_insensitive`,
and set partkey's collation to 'C'.
```sql
SET enable_partitionwise_aggregate TO true;
SET enable_partitionwise_join TO true;
SET max_parallel_workers_per_gather TO 0;
SET enable_incremental_sort TO off;
CREATE TABLE pagg_tab (c text collate case_insensitive) PARTITION BY LIST(c
collate "C");
CREATE TABLE pagg_tab_p1 PARTITION OF pagg_tab FOR VALUES IN ('a', 'b',
'c', 'd');
CREATE TABLE pagg_tab_p2 PARTITION OF pagg_tab FOR VALUES IN ('e', 'f',
'A');
CREATE TABLE pagg_tab_p3 PARTITION OF pagg_tab FOR VALUES IN ('B', 'C',
'D', 'E');
INSERT INTO pagg_tab SELECT substr('abcdeABCDE', (i % 10) +1 , 1) FROM
generate_series(0, 2999) i;
ANALYZE pagg_tab;
```
We do group by on the table pagg_tab use `case_insensitive` collation, we
hope group key is case-insensitive.
but we find the execution result is not what we expected.
```shell
postgres=# SELECT c collate case_insensitive, count(c) FROM pagg_tab GROUP
BY c collate case_insensitive;
c | count
---+-------
A | 300
e | 300
E | 300
D | 300
C | 300
B | 300
d | 300
c | 300
b | 300
a | 300
(10 rows)
```
The reason is the function group_by_has_partkey() do not check partkey's
collation, that lead to explain error.
```shell
postgres=# EXPLAIN SELECT c collate case_insensitive, count(c) FROM
pagg_tab GROUP BY c collate case_insensitive ;
QUERY PLAN
--------------------------------------------------------------------------------------
Append (cost=12.00..60.15 rows=10 width=10)
-> HashAggregate (cost=12.00..12.02 rows=2 width=10)
Group Key: pagg_tab.c
-> Seq Scan on pagg_tab_p2 pagg_tab (cost=0.00..9.00 rows=600
width=2)
-> HashAggregate (cost=24.00..24.04 rows=4 width=10)
Group Key: pagg_tab_1.c
-> Seq Scan on pagg_tab_p3 pagg_tab_1 (cost=0.00..18.00
rows=1200 width=2)
-> HashAggregate (cost=24.00..24.04 rows=4 width=10)
Group Key: pagg_tab_2.c
-> Seq Scan on pagg_tab_p1 pagg_tab_2 (cost=0.00..18.00
rows=1200 width=2)
(10 rows)
```
So, group_by_has_partkey() need to verify if the partkey's collation matches
the groupkey,
meanwhile, if groupkey is RelabelType node and it's collation equal to
partkey's, it should
also set variable `found` to true.
^ permalink raw reply [nested|flat] 30+ messages in thread
* Re: BUG #18568: BUG: Result wrong when do group by on partition table!
@ 2024-08-06 08:42 Tender Wang <tndrwang@gmail.com>
parent: PG Bug reporting form <noreply@postgresql.org>
1 sibling, 4 replies; 30+ messages in thread
From: Tender Wang @ 2024-08-06 08:42 UTC (permalink / raw)
To: 1105066510@qq.com; pgsql-bugs@lists.postgresql.org
PG Bug reporting form <noreply@postgresql.org> 于2024年8月6日周二 15:01写道：
> The following bug has been logged on the website:
>
> Bug reference: 18568
> Logged by: Webbo Han
> Email address: 1105066510@qq.com
> PostgreSQL version: 16.3
> Operating system: centos 7.6
> Description:
>
> First, we create one case-insensitive collation use ICU:
> ```sql
> CREATE COLLATION case_insensitive (
> provider = icu,
> locale = 'und-u-ks-level2',
> deterministic = false
> );
> ```
>
> Then, we create the partition table, meanwhile we set the collation of
> column c to `case_insensitive`,
> and set partkey's collation to 'C'.
> ```sql
> SET enable_partitionwise_aggregate TO true;
> SET enable_partitionwise_join TO true;
> SET max_parallel_workers_per_gather TO 0;
> SET enable_incremental_sort TO off;
> CREATE TABLE pagg_tab (c text collate case_insensitive) PARTITION
> BY LIST(c
> collate "C");
> CREATE TABLE pagg_tab_p1 PARTITION OF pagg_tab FOR VALUES IN ('a',
> 'b',
> 'c', 'd');
> CREATE TABLE pagg_tab_p2 PARTITION OF pagg_tab FOR VALUES IN ('e',
> 'f',
> 'A');
> CREATE TABLE pagg_tab_p3 PARTITION OF pagg_tab FOR VALUES IN ('B',
> 'C',
> 'D', 'E');
> INSERT INTO pagg_tab SELECT substr('abcdeABCDE', (i % 10) +1 , 1)
> FROM
> generate_series(0, 2999) i;
> ANALYZE pagg_tab;
> ```
>
> We do group by on the table pagg_tab use `case_insensitive` collation, we
> hope group key is case-insensitive.
> but we find the execution result is not what we expected.
> ```shell
> postgres=# SELECT c collate case_insensitive, count(c) FROM
> pagg_tab GROUP
> BY c collate case_insensitive;
> c | count
> ---+-------
> A | 300
> e | 300
> E | 300
> D | 300
> C | 300
> B | 300
> d | 300
> c | 300
> b | 300
> a | 300
> (10 rows)
> ```
>
> The reason is the function group_by_has_partkey() do not check partkey's
> collation, that lead to explain error.
> ```shell
> postgres=# EXPLAIN SELECT c collate case_insensitive, count(c) FROM
> pagg_tab GROUP BY c collate case_insensitive ;
> QUERY PLAN
>
> --------------------------------------------------------------------------------------
> Append (cost=12.00..60.15 rows=10 width=10)
> -> HashAggregate (cost=12.00..12.02 rows=2 width=10)
> Group Key: pagg_tab.c
> -> Seq Scan on pagg_tab_p2 pagg_tab (cost=0.00..9.00
> rows=600
> width=2)
> -> HashAggregate (cost=24.00..24.04 rows=4 width=10)
> Group Key: pagg_tab_1.c
> -> Seq Scan on pagg_tab_p3 pagg_tab_1 (cost=0.00..18.00
> rows=1200 width=2)
> -> HashAggregate (cost=24.00..24.04 rows=4 width=10)
> Group Key: pagg_tab_2.c
> -> Seq Scan on pagg_tab_p1 pagg_tab_2 (cost=0.00..18.00
> rows=1200 width=2)
> (10 rows)
> ```
>
> So, group_by_has_partkey() need to verify if the partkey's collation
> matches
> the groupkey,
> meanwhile, if groupkey is RelabelType node and it's collation equal to
> partkey's, it should
> also set variable `found` to true.
>
>
Yeah, I can reproduce $subject on HEAD.
But I found this when debug into group_by_has_partkey(), as below:
call nodeToString(groupexprs):
VAR : varcollid 16384
call nodeToString(partexpr):
VAR: varcollid 16384
So the collid of partkey and groupexpr is same, so add check here may not
fix this issue.
I continue to find out why the collation id of partkey is 16384(e.g.
case_insensitive). The partkey expr info is
set in set_baserel_partition_key_exprs(), which it uses
partkey->parttypcoll[cnt] value not partkey->partcollation value.
And partkey->parttypcoll[cnt] is assigned from pg_attribute , which is the
column c meta data.
Should we use partkey->partcollation value? I try to fix that in the
attached patch. I add your case in the test, and I don't find
failed regress.
--
Tender Wang
Attachments:
[application/octet-stream] v1-0001-Fix-collation-different-between-columan-collation.patch (4.2K, ../../CAHewXNnKLrZYG4iqaYw=uB3XWRrYRZHo7VtcMsbUEbdbajQg2Q@mail.gmail.com/3-v1-0001-Fix-collation-different-between-columan-collation.patch)
download | inline diff:
From f47375da9849ecd6e6f8179d6b28d75764280be0 Mon Sep 17 00:00:00 2001
From: Tender Wang <tndrwang@gmail.com>
Date: Tue, 6 Aug 2024 16:38:03 +0800
Subject: [PATCH v1] Fix collation different between columan collation and
partkey collation
---
src/backend/optimizer/util/plancat.c | 2 +-
src/test/regress/expected/partition_info.out | 34 ++++++++++++++++++++
src/test/regress/sql/partition_info.sql | 19 +++++++++++
3 files changed, 54 insertions(+), 1 deletion(-)
diff --git a/src/backend/optimizer/util/plancat.c b/src/backend/optimizer/util/plancat.c
index 78a3cfafde..29e33150f1 100644
--- a/src/backend/optimizer/util/plancat.c
+++ b/src/backend/optimizer/util/plancat.c
@@ -2531,7 +2531,7 @@ set_baserel_partition_key_exprs(Relation relation,
partexpr = (Expr *) makeVar(varno, attno,
partkey->parttypid[cnt],
partkey->parttypmod[cnt],
- partkey->parttypcoll[cnt], 0);
+ partkey->partcollation[cnt], 0);
}
else
{
diff --git a/src/test/regress/expected/partition_info.out b/src/test/regress/expected/partition_info.out
index 42b6bc77ca..13b096307e 100644
--- a/src/test/regress/expected/partition_info.out
+++ b/src/test/regress/expected/partition_info.out
@@ -349,3 +349,37 @@ SELECT pg_partition_root('ptif_li_child');
DROP VIEW ptif_test_view;
DROP MATERIALIZED VIEW ptif_test_matview;
DROP TABLE ptif_li_parent, ptif_li_child;
+-- Test collation
+CREATE COLLATION case_insensitive (
+ provider = icu,
+ locale = 'und-u-ks-level2',
+ deterministic = false);
+CREATE TABLE pagg_tab_col (c text collate case_insensitive) PARTITION BY LIST(c collate "C");
+CREATE TABLE pagg_tab_col_p1 PARTITION OF pagg_tab_col FOR VALUES IN ('a', 'b', 'c', 'd');
+CREATE TABLE pagg_tab_col_p2 PARTITION OF pagg_tab_col FOR VALUES IN ('e', 'f','A');
+CREATE TABLE pagg_tab_col_p3 PARTITION OF pagg_tab_col FOR VALUES IN ('B', 'C', 'D', 'E');
+INSERT INTO pagg_tab_col SELECT substr('abcdeABCDE', (i % 10) +1 , 1) FROM generate_series(0, 2999) i;
+ANALYZE pagg_tab_col;
+EXPLAIN SELECT c collate case_insensitive, count(c) FROM
+pagg_tab_col GROUP BY c collate case_insensitive;
+ QUERY PLAN
+----------------------------------------------------------------------------------------------
+ HashAggregate (cost=75.00..75.05 rows=5 width=10)
+ Group Key: pagg_tab_col.c
+ -> Append (cost=0.00..60.00 rows=3000 width=2)
+ -> Seq Scan on pagg_tab_col_p2 pagg_tab_col_1 (cost=0.00..9.00 rows=600 width=2)
+ -> Seq Scan on pagg_tab_col_p3 pagg_tab_col_2 (cost=0.00..18.00 rows=1200 width=2)
+ -> Seq Scan on pagg_tab_col_p1 pagg_tab_col_3 (cost=0.00..18.00 rows=1200 width=2)
+(6 rows)
+
+SELECT c collate case_insensitive, count(c) FROM
+pagg_tab_col GROUP BY c collate case_insensitive;
+ c | count
+---+-------
+ e | 600
+ D | 600
+ C | 600
+ B | 600
+ A | 600
+(5 rows)
+
diff --git a/src/test/regress/sql/partition_info.sql b/src/test/regress/sql/partition_info.sql
index b5060bec7f..5c27c82217 100644
--- a/src/test/regress/sql/partition_info.sql
+++ b/src/test/regress/sql/partition_info.sql
@@ -127,3 +127,22 @@ SELECT pg_partition_root('ptif_li_child');
DROP VIEW ptif_test_view;
DROP MATERIALIZED VIEW ptif_test_matview;
DROP TABLE ptif_li_parent, ptif_li_child;
+
+-- Test collation
+CREATE COLLATION case_insensitive (
+ provider = icu,
+ locale = 'und-u-ks-level2',
+ deterministic = false);
+
+CREATE TABLE pagg_tab_col (c text collate case_insensitive) PARTITION BY LIST(c collate "C");
+CREATE TABLE pagg_tab_col_p1 PARTITION OF pagg_tab_col FOR VALUES IN ('a', 'b', 'c', 'd');
+CREATE TABLE pagg_tab_col_p2 PARTITION OF pagg_tab_col FOR VALUES IN ('e', 'f','A');
+CREATE TABLE pagg_tab_col_p3 PARTITION OF pagg_tab_col FOR VALUES IN ('B', 'C', 'D', 'E');
+INSERT INTO pagg_tab_col SELECT substr('abcdeABCDE', (i % 10) +1 , 1) FROM generate_series(0, 2999) i;
+ANALYZE pagg_tab_col;
+
+EXPLAIN SELECT c collate case_insensitive, count(c) FROM
+pagg_tab_col GROUP BY c collate case_insensitive;
+
+SELECT c collate case_insensitive, count(c) FROM
+pagg_tab_col GROUP BY c collate case_insensitive;
--
2.34.1
^ permalink raw reply [nested|flat] 30+ messages in thread
* Re: BUG #18568: BUG: Result wrong when do group by on partition table!
@ 2024-08-06 09:09 Aleksander Alekseev <aleksander@timescale.com>
parent: Tender Wang <tndrwang@gmail.com>
3 siblings, 0 replies; 30+ messages in thread
From: Aleksander Alekseev @ 2024-08-06 09:09 UTC (permalink / raw)
To: pgsql-bugs@lists.postgresql.org; +Cc: Tender Wang <tndrwang@gmail.com>; 1105066510@qq.com
Hi,
> [...]
> I continue to find out why the collation id of partkey is 16384(e.g. case_insensitive). The partkey expr info is
> set in set_baserel_partition_key_exprs(), which it uses partkey->parttypcoll[cnt] value not partkey->partcollation value.
>
> And partkey->parttypcoll[cnt] is assigned from pg_attribute , which is the column c meta data.
> Should we use partkey->partcollation value? I try to fix that in the attached patch. I add your case in the test, and I don't find
> failed regress.
```
+SELECT c collate case_insensitive, count(c) FROM
+pagg_tab_col GROUP BY c collate case_insensitive;
+ c | count
+---+-------
+ e | 600
+ D | 600
+ C | 600
+ B | 600
+ A | 600
+(5 rows)
```
Shouldn't we use UPPER(c) and ORDER BY in the test case to make the
results deterministic?
--
Best regards,
Aleksander Alekseev
^ permalink raw reply [nested|flat] 30+ messages in thread
* Re: BUG #18568: BUG: Result wrong when do group by on partition table!
@ 2024-08-06 09:27 Tender Wang <tndrwang@gmail.com>
parent: Tender Wang <tndrwang@gmail.com>
3 siblings, 0 replies; 30+ messages in thread
From: Tender Wang @ 2024-08-06 09:27 UTC (permalink / raw)
To: 1105066510@qq.com; pgsql-bugs@lists.postgresql.org
Tender Wang <tndrwang@gmail.com> 于2024年8月6日周二 16:42写道：
> Yeah, I can reproduce $subject on HEAD.
> But I found this when debug into group_by_has_partkey(), as below:
> call nodeToString(groupexprs):
> VAR : varcollid 16384
> call nodeToString(partexpr):
> VAR: varcollid 16384
>
> So the collid of partkey and groupexpr is same, so add check here may not
> fix this issue.
>
> I continue to find out why the collation id of partkey is 16384(e.g.
> case_insensitive). The partkey expr info is
> set in set_baserel_partition_key_exprs(), which it uses
> partkey->parttypcoll[cnt] value not partkey->partcollation value.
>
> And partkey->parttypcoll[cnt] is assigned from pg_attribute , which is the
> column c meta data.
> Should we use partkey->partcollation value? I try to fix that in the
> attached patch. I add your case in the test, and I don't find
> failed regress.
>
The plan seems not what we want, because I forgot to
set enable_partitionwise_aggregate to true.
--
Tender Wang
Attachments:
[application/octet-stream] v2-0001-Fix-collation-different-between-columan-collation.patch (5.1K, ../../CAHewXNngCmzj8ObeYhYzTj56rDb2HAa-xv-Ekt=BNGJ-9rysGg@mail.gmail.com/3-v2-0001-Fix-collation-different-between-columan-collation.patch)
download | inline diff:
From 802f231501cdf11361a0d1e10adbb84fd9261ff1 Mon Sep 17 00:00:00 2001
From: Tender Wang <tndrwang@gmail.com>
Date: Tue, 6 Aug 2024 16:38:03 +0800
Subject: [PATCH v2] Fix collation different between columan collation and
partkey collation
---
src/backend/optimizer/util/plancat.c | 2 +-
src/test/regress/expected/partition_info.out | 52 ++++++++++++++++++++
src/test/regress/sql/partition_info.sql | 22 +++++++++
3 files changed, 75 insertions(+), 1 deletion(-)
diff --git a/src/backend/optimizer/util/plancat.c b/src/backend/optimizer/util/plancat.c
index 78a3cfafde..29e33150f1 100644
--- a/src/backend/optimizer/util/plancat.c
+++ b/src/backend/optimizer/util/plancat.c
@@ -2531,7 +2531,7 @@ set_baserel_partition_key_exprs(Relation relation,
partexpr = (Expr *) makeVar(varno, attno,
partkey->parttypid[cnt],
partkey->parttypmod[cnt],
- partkey->parttypcoll[cnt], 0);
+ partkey->partcollation[cnt], 0);
}
else
{
diff --git a/src/test/regress/expected/partition_info.out b/src/test/regress/expected/partition_info.out
index 42b6bc77ca..e68ff1fbe8 100644
--- a/src/test/regress/expected/partition_info.out
+++ b/src/test/regress/expected/partition_info.out
@@ -349,3 +349,55 @@ SELECT pg_partition_root('ptif_li_child');
DROP VIEW ptif_test_view;
DROP MATERIALIZED VIEW ptif_test_matview;
DROP TABLE ptif_li_parent, ptif_li_child;
+-- Test collation
+SET enable_partitionwise_aggregate TO true;
+CREATE COLLATION case_insensitive (
+ provider = icu,
+ locale = 'und-u-ks-level2',
+ deterministic = false);
+CREATE TABLE pagg_tab_col (c text collate case_insensitive) PARTITION BY LIST(c collate "C");
+CREATE TABLE pagg_tab_col_p1 PARTITION OF pagg_tab_col FOR VALUES IN ('a', 'b', 'c', 'd');
+CREATE TABLE pagg_tab_col_p2 PARTITION OF pagg_tab_col FOR VALUES IN ('e', 'f','A');
+CREATE TABLE pagg_tab_col_p3 PARTITION OF pagg_tab_col FOR VALUES IN ('B', 'C', 'D', 'E');
+INSERT INTO pagg_tab_col SELECT substr('abcdeABCDE', (i % 10) +1 , 1) FROM generate_series(0, 2999) i;
+ANALYZE pagg_tab_col;
+EXPLAIN(VERBOSE, COSTS OFF) SELECT c collate case_insensitive, count(c) FROM
+pagg_tab_col GROUP BY c collate case_insensitive;
+ QUERY PLAN
+-------------------------------------------------------------------------------
+ Finalize GroupAggregate
+ Output: pagg_tab_col.c, count(pagg_tab_col.c)
+ Group Key: pagg_tab_col.c
+ -> Sort
+ Output: pagg_tab_col.c, (PARTIAL count(pagg_tab_col.c))
+ Sort Key: pagg_tab_col.c COLLATE case_insensitive
+ -> Append
+ -> Partial HashAggregate
+ Output: pagg_tab_col.c, PARTIAL count(pagg_tab_col.c)
+ Group Key: pagg_tab_col.c
+ -> Seq Scan on public.pagg_tab_col_p2 pagg_tab_col
+ Output: pagg_tab_col.c
+ -> Partial HashAggregate
+ Output: pagg_tab_col_1.c, PARTIAL count(pagg_tab_col_1.c)
+ Group Key: pagg_tab_col_1.c
+ -> Seq Scan on public.pagg_tab_col_p3 pagg_tab_col_1
+ Output: pagg_tab_col_1.c
+ -> Partial HashAggregate
+ Output: pagg_tab_col_2.c, PARTIAL count(pagg_tab_col_2.c)
+ Group Key: pagg_tab_col_2.c
+ -> Seq Scan on public.pagg_tab_col_p1 pagg_tab_col_2
+ Output: pagg_tab_col_2.c
+(22 rows)
+
+SELECT c collate case_insensitive, count(c) FROM
+pagg_tab_col GROUP BY c collate case_insensitive;
+ c | count
+---+-------
+ a | 600
+ b | 600
+ C | 600
+ d | 600
+ E | 600
+(5 rows)
+
+RESET enable_partitionwise_aggregate;
diff --git a/src/test/regress/sql/partition_info.sql b/src/test/regress/sql/partition_info.sql
index b5060bec7f..5c1a7bab5d 100644
--- a/src/test/regress/sql/partition_info.sql
+++ b/src/test/regress/sql/partition_info.sql
@@ -127,3 +127,25 @@ SELECT pg_partition_root('ptif_li_child');
DROP VIEW ptif_test_view;
DROP MATERIALIZED VIEW ptif_test_matview;
DROP TABLE ptif_li_parent, ptif_li_child;
+
+-- Test collation
+SET enable_partitionwise_aggregate TO true;
+CREATE COLLATION case_insensitive (
+ provider = icu,
+ locale = 'und-u-ks-level2',
+ deterministic = false);
+
+CREATE TABLE pagg_tab_col (c text collate case_insensitive) PARTITION BY LIST(c collate "C");
+CREATE TABLE pagg_tab_col_p1 PARTITION OF pagg_tab_col FOR VALUES IN ('a', 'b', 'c', 'd');
+CREATE TABLE pagg_tab_col_p2 PARTITION OF pagg_tab_col FOR VALUES IN ('e', 'f','A');
+CREATE TABLE pagg_tab_col_p3 PARTITION OF pagg_tab_col FOR VALUES IN ('B', 'C', 'D', 'E');
+INSERT INTO pagg_tab_col SELECT substr('abcdeABCDE', (i % 10) +1 , 1) FROM generate_series(0, 2999) i;
+ANALYZE pagg_tab_col;
+
+EXPLAIN(VERBOSE, COSTS OFF) SELECT c collate case_insensitive, count(c) FROM
+pagg_tab_col GROUP BY c collate case_insensitive;
+
+SELECT c collate case_insensitive, count(c) FROM
+pagg_tab_col GROUP BY c collate case_insensitive;
+
+RESET enable_partitionwise_aggregate;
--
2.34.1
^ permalink raw reply [nested|flat] 30+ messages in thread
* Re: BUG #18568: BUG: Result wrong when do group by on partition table!
@ 2024-08-06 13:23 Tender Wang <tndrwang@gmail.com>
parent: PG Bug reporting form <noreply@postgresql.org>
1 sibling, 1 reply; 30+ messages in thread
From: Tender Wang @ 2024-08-06 13:23 UTC (permalink / raw)
To: 1105066510@qq.com; pgsql-bugs@lists.postgresql.org
PG Bug reporting form <noreply@postgresql.org> 于2024年8月6日周二 15:01写道：
> The following bug has been logged on the website:
>
> Bug reference: 18568
> Logged by: Webbo Han
> Email address: 1105066510@qq.com
> PostgreSQL version: 16.3
> Operating system: centos 7.6
> Description:
>
> First, we create one case-insensitive collation use ICU:
> ```sql
> CREATE COLLATION case_insensitive (
> provider = icu,
> locale = 'und-u-ks-level2',
> deterministic = false
> );
> ```
>
> Then, we create the partition table, meanwhile we set the collation of
> column c to `case_insensitive`,
> and set partkey's collation to 'C'.
> ```sql
> SET enable_partitionwise_aggregate TO true;
> SET enable_partitionwise_join TO true;
> SET max_parallel_workers_per_gather TO 0;
> SET enable_incremental_sort TO off;
> CREATE TABLE pagg_tab (c text collate case_insensitive) PARTITION
> BY LIST(c
> collate "C");
>
I think above create table again. Should we allow this CREATE TABLE? The
partition key
definition are not same with column definiton. Is it better to report
error for users?
--
Tender Wang
^ permalink raw reply [nested|flat] 30+ messages in thread
* Re: BUG #18568: BUG: Result wrong when do group by on partition table!
@ 2024-08-06 14:37 Tender Wang <tndrwang@gmail.com>
parent: Tender Wang <tndrwang@gmail.com>
3 siblings, 0 replies; 30+ messages in thread
From: Tender Wang @ 2024-08-06 14:37 UTC (permalink / raw)
To: 狂奔的蜗牛 <1105066510@qq.com>; +Cc: pgsql-bugs <pgsql-bugs@lists.postgresql.org>
狂奔的蜗牛 <1105066510@qq.com> 于2024年8月6日周二 19:33写道：
> Partkey's collation stored in RelOptInfo->part_scheme, and I use it to fix
> the bug.
> The attachment is my solution!
>
I look through your patch. Some code is not native for PG. For example:
if (nodeTag(groupexpr) == T_RelabelType)
usually, we use IsA() func.
--
Tender Wang
^ permalink raw reply [nested|flat] 30+ messages in thread
* Re: BUG #18568: BUG: Result wrong when do group by on partition table!
@ 2024-08-07 04:01 Tender Wang <tndrwang@gmail.com>
parent: Tender Wang <tndrwang@gmail.com>
3 siblings, 1 reply; 30+ messages in thread
From: Tender Wang @ 2024-08-07 04:01 UTC (permalink / raw)
To: 狂奔的蜗牛 <1105066510@qq.com>; +Cc: pgsql-bugs <pgsql-bugs@lists.postgresql.org>
I think what I have done in v2 is not the right way. Because partition
prune logic first checks whether it is equal
or not, then it will check the collation match or not. So I should not
change the set_baserel_partition_key_exprs() logic.
I look through your patch and make some changes.
1.
git am your patch, report warnings, some code format issue.
2.
I removed this branch: if (IsA(groupexpr, RelabelType)).
Because in your added test case, it didn't enter this branch. I didn't
figure out what kind of group by clause is RelableType.
You can provide a test case based on the v3 patch.
3. Tweek a little about the test case.
By the way, your last two emails only sent to me, please cc the pgsql-bugs.
狂奔的蜗牛 <1105066510@qq.com> 于2024年8月6日周二 19:33写道：
> Partkey's collation stored in RelOptInfo->part_scheme, and I use it to fix
> the bug.
> The attachment is my solution!
> ------------------------------
> 狂奔的蜗牛
> 1105066510@qq.com
>
> <https://wx.mail.qq.com/home/index?t=readmail_businesscard_midpage&nocheck=true&name=%E7%8B%8...;
>
>
>
> ------------------ 原始邮件 ------------------
> *发件人:* "Tender Wang" <tndrwang@gmail.com>;
> *发送时间:* 2024年8月6日(星期二) 下午4:42
> *收件人:* "狂奔的蜗牛"<1105066510@qq.com>;"pgsql-bugs"<
> pgsql-bugs@lists.postgresql.org>;
> *主题:* Re: BUG #18568: BUG: Result wrong when do group by on partition
> table!
>
>
>
> PG Bug reporting form <noreply@postgresql.org> 于2024年8月6日周二 15:01写道：
>
>> The following bug has been logged on the website:
>>
>> Bug reference: 18568
>> Logged by: Webbo Han
>> Email address: 1105066510@qq.com
>> PostgreSQL version: 16.3
>> Operating system: centos 7.6
>> Description:
>>
>> First, we create one case-insensitive collation use ICU:
>> ```sql
>> CREATE COLLATION case_insensitive (
>> provider = icu,
>> locale = 'und-u-ks-level2',
>> deterministic = false
>> );
>> ```
>>
>> Then, we create the partition table, meanwhile we set the collation of
>> column c to `case_insensitive`,
>> and set partkey's collation to 'C'.
>> ```sql
>> SET enable_partitionwise_aggregate TO true;
>> SET enable_partitionwise_join TO true;
>> SET max_parallel_workers_per_gather TO 0;
>> SET enable_incremental_sort TO off;
>> CREATE TABLE pagg_tab (c text collate case_insensitive) PARTITION
>> BY LIST(c
>> collate "C");
>> CREATE TABLE pagg_tab_p1 PARTITION OF pagg_tab FOR VALUES IN
>> ('a', 'b',
>> 'c', 'd');
>> CREATE TABLE pagg_tab_p2 PARTITION OF pagg_tab FOR VALUES IN
>> ('e', 'f',
>> 'A');
>> CREATE TABLE pagg_tab_p3 PARTITION OF pagg_tab FOR VALUES IN
>> ('B', 'C',
>> 'D', 'E');
>> INSERT INTO pagg_tab SELECT substr('abcdeABCDE', (i % 10) +1 , 1)
>> FROM
>> generate_series(0, 2999) i;
>> ANALYZE pagg_tab;
>> ```
>>
>> We do group by on the table pagg_tab use `case_insensitive` collation, we
>> hope group key is case-insensitive.
>> but we find the execution result is not what we expected.
>> ```shell
>> postgres=# SELECT c collate case_insensitive, count(c) FROM
>> pagg_tab GROUP
>> BY c collate case_insensitive;
>> c | count
>> ---+-------
>> A | 300
>> e | 300
>> E | 300
>> D | 300
>> C | 300
>> B | 300
>> d | 300
>> c | 300
>> b | 300
>> a | 300
>> (10 rows)
>> ```
>>
>> The reason is the function group_by_has_partkey() do not check partkey's
>> collation, that lead to explain error.
>> ```shell
>> postgres=# EXPLAIN SELECT c collate case_insensitive, count(c)
>> FROM
>> pagg_tab GROUP BY c collate case_insensitive ;
>> QUERY PLAN
>>
>> --------------------------------------------------------------------------------------
>> Append (cost=12.00..60.15 rows=10 width=10)
>> -> HashAggregate (cost=12.00..12.02 rows=2 width=10)
>> Group Key: pagg_tab.c
>> -> Seq Scan on pagg_tab_p2 pagg_tab (cost=0.00..9.00
>> rows=600
>> width=2)
>> -> HashAggregate (cost=24.00..24.04 rows=4 width=10)
>> Group Key: pagg_tab_1.c
>> -> Seq Scan on pagg_tab_p3 pagg_tab_1 (cost=0.00..18.00
>> rows=1200 width=2)
>> -> HashAggregate (cost=24.00..24.04 rows=4 width=10)
>> Group Key: pagg_tab_2.c
>> -> Seq Scan on pagg_tab_p1 pagg_tab_2 (cost=0.00..18.00
>> rows=1200 width=2)
>> (10 rows)
>> ```
>>
>> So, group_by_has_partkey() need to verify if the partkey's collation
>> matches
>> the groupkey,
>> meanwhile, if groupkey is RelabelType node and it's collation equal to
>> partkey's, it should
>> also set variable `found` to true.
>>
>>
> Yeah, I can reproduce $subject on HEAD.
> But I found this when debug into group_by_has_partkey(), as below:
> call nodeToString(groupexprs):
> VAR : varcollid 16384
> call nodeToString(partexpr):
> VAR: varcollid 16384
>
> So the collid of partkey and groupexpr is same, so add check here may not
> fix this issue.
>
> I continue to find out why the collation id of partkey is 16384(e.g.
> case_insensitive). The partkey expr info is
> set in set_baserel_partition_key_exprs(), which it uses
> partkey->parttypcoll[cnt] value not partkey->partcollation value.
>
> And partkey->parttypcoll[cnt] is assigned from pg_attribute , which is the
> column c meta data.
> Should we use partkey->partcollation value? I try to fix that in the
> attached patch. I add your case in the test, and I don't find
> failed regress.
>
> --
> Tender Wang
>
--
Tender Wang
Attachments:
[application/octet-stream] v3-0001-fix-group_by_has_partkey-bug-and-add-regress-test.patch (5.3K, ../../CAHewXNn7LoMiwAPXJvSVkU_W8cu=nj4ec9RaqGn2QrTEr5QBrw@mail.gmail.com/3-v3-0001-fix-group_by_has_partkey-bug-and-add-regress-test.patch)
download | inline diff:
From 35822d7cdeae12945e09bd79b293b301e96594d8 Mon Sep 17 00:00:00 2001
From: hanweibo <hanweibo@highgo.com>
Date: Wed, 7 Aug 2024 09:25:34 +0800
Subject: [PATCH v3] fix group_by_has_partkey bug and add regress test case
---
src/backend/optimizer/plan/planner.c | 24 +++++++++++--
.../regress/expected/partition_aggregate.out | 35 +++++++++++++++++++
src/test/regress/sql/partition_aggregate.sql | 21 +++++++++++
3 files changed, 77 insertions(+), 3 deletions(-)
diff --git a/src/backend/optimizer/plan/planner.c b/src/backend/optimizer/plan/planner.c
index 948afd9094..88a8937b43 100644
--- a/src/backend/optimizer/plan/planner.c
+++ b/src/backend/optimizer/plan/planner.c
@@ -7993,6 +7993,10 @@ create_partitionwise_grouping_paths(PlannerInfo *root,
}
}
+/* XXX see PartCollMatchesExprColl */
+#define PartKeyCollMatchesExprColl(partcoll, exprcoll) \
+ ((partcoll) == InvalidOid || (partcoll) == (exprcoll))
+
/*
* group_by_has_partkey
*
@@ -8025,13 +8029,27 @@ group_by_has_partkey(RelOptInfo *input_rel,
foreach(lc, partexprs)
{
+ ListCell *lg;
Expr *partexpr = lfirst(lc);
+ Oid partcoll = input_rel->part_scheme->partcollation[cnt];
- if (list_member(groupexprs, partexpr))
+ foreach (lg, groupexprs)
{
- found = true;
- break;
+ Expr *groupexpr = lfirst(lg);
+ Oid groupexpr_coll = exprCollation((Node *) groupexpr);
+
+ /*
+ * Partition key match also requires collation match.
+ */
+ if (equal(groupexpr, partexpr) &&
+ PartKeyCollMatchesExprColl(partcoll, groupexpr_coll))
+ {
+ found = true;
+ break;
+ }
}
+ if (found)
+ break;
}
/*
diff --git a/src/test/regress/expected/partition_aggregate.out b/src/test/regress/expected/partition_aggregate.out
index 5f2c0cf578..a512d6291f 100644
--- a/src/test/regress/expected/partition_aggregate.out
+++ b/src/test/regress/expected/partition_aggregate.out
@@ -1518,3 +1518,38 @@ SELECT x, sum(y), avg(y), count(*) FROM pagg_tab_para GROUP BY x HAVING avg(y) <
21 | 6000 | 6.0000000000000000 | 1000
(6 rows)
+-- test for group_by_has_partkey(). Add collation check between groupkey and partkey.
+-- If groupkey's collation not equal to partkey's, we cannot use PARTITIONWISE_AGGREGATE_FULL.
+SET enable_partitionwise_aggregate TO true;
+SET enable_partitionwise_join TO true;
+SET max_parallel_workers_per_gather TO 0;
+SET enable_incremental_sort TO off;
+CREATE TABLE pagg_tab3 (c text collate "POSIX") PARTITION BY LIST(c collate "C");
+CREATE TABLE pagg_tab3_p1 PARTITION OF pagg_tab3 FOR VALUES IN ('a', 'b', 'c', 'd');
+CREATE TABLE pagg_tab3_p2 PARTITION OF pagg_tab3 FOR VALUES IN ('e', 'f', 'A');
+CREATE TABLE pagg_tab3_p3 PARTITION OF pagg_tab3 FOR VALUES IN ('B', 'C', 'D', 'E');
+INSERT INTO pagg_tab3 SELECT substr('abcdeABCDE', (i % 10) +1 , 1) FROM generate_series(0, 2999) i;
+ANALYZE pagg_tab3;
+EXPLAIN (COSTS OFF) SELECT c collate "POSIX", count(c) FROM pagg_tab3 GROUP BY c collate "POSIX" ;
+ QUERY PLAN
+--------------------------------------------------------------
+ Finalize GroupAggregate
+ Group Key: pagg_tab3.c
+ -> Sort
+ Sort Key: pagg_tab3.c COLLATE "POSIX"
+ -> Append
+ -> Partial HashAggregate
+ Group Key: pagg_tab3.c
+ -> Seq Scan on pagg_tab3_p2 pagg_tab3
+ -> Partial HashAggregate
+ Group Key: pagg_tab3_1.c
+ -> Seq Scan on pagg_tab3_p3 pagg_tab3_1
+ -> Partial HashAggregate
+ Group Key: pagg_tab3_2.c
+ -> Seq Scan on pagg_tab3_p1 pagg_tab3_2
+(14 rows)
+
+RESET enable_partitionwise_aggregate;
+RESET enable_partitionwise_join;
+RESET max_parallel_workers_per_gather;
+RESET enable_incremental_sort;
diff --git a/src/test/regress/sql/partition_aggregate.sql b/src/test/regress/sql/partition_aggregate.sql
index ab070fee24..5b219e835f 100644
--- a/src/test/regress/sql/partition_aggregate.sql
+++ b/src/test/regress/sql/partition_aggregate.sql
@@ -334,3 +334,24 @@ RESET parallel_setup_cost;
EXPLAIN (COSTS OFF)
SELECT x, sum(y), avg(y), count(*) FROM pagg_tab_para GROUP BY x HAVING avg(y) < 7 ORDER BY 1, 2, 3;
SELECT x, sum(y), avg(y), count(*) FROM pagg_tab_para GROUP BY x HAVING avg(y) < 7 ORDER BY 1, 2, 3;
+
+
+-- test for group_by_has_partkey(). Add collation check between groupkey and partkey.
+-- If groupkey's collation not equal to partkey's, we cannot use PARTITIONWISE_AGGREGATE_FULL.
+SET enable_partitionwise_aggregate TO true;
+SET enable_partitionwise_join TO true;
+SET max_parallel_workers_per_gather TO 0;
+SET enable_incremental_sort TO off;
+
+CREATE TABLE pagg_tab3 (c text collate "POSIX") PARTITION BY LIST(c collate "C");
+CREATE TABLE pagg_tab3_p1 PARTITION OF pagg_tab3 FOR VALUES IN ('a', 'b', 'c', 'd');
+CREATE TABLE pagg_tab3_p2 PARTITION OF pagg_tab3 FOR VALUES IN ('e', 'f', 'A');
+CREATE TABLE pagg_tab3_p3 PARTITION OF pagg_tab3 FOR VALUES IN ('B', 'C', 'D', 'E');
+INSERT INTO pagg_tab3 SELECT substr('abcdeABCDE', (i % 10) +1 , 1) FROM generate_series(0, 2999) i;
+ANALYZE pagg_tab3;
+EXPLAIN (COSTS OFF) SELECT c collate "POSIX", count(c) FROM pagg_tab3 GROUP BY c collate "POSIX" ;
+
+RESET enable_partitionwise_aggregate;
+RESET enable_partitionwise_join;
+RESET max_parallel_workers_per_gather;
+RESET enable_incremental_sort;
--
2.34.1
^ permalink raw reply [nested|flat] 30+ messages in thread
* Re: BUG #18568: BUG: Result wrong when do group by on partition table!
@ 2024-08-07 06:57 Tender Wang <tndrwang@gmail.com>
parent: Tender Wang <tndrwang@gmail.com>
0 siblings, 1 reply; 30+ messages in thread
From: Tender Wang @ 2024-08-07 06:57 UTC (permalink / raw)
To: 狂奔的蜗牛 <1105066510@qq.com>; +Cc: pgsql-bugs <pgsql-bugs@lists.postgresql.org>; aleksander@timescale.com
狂奔的蜗牛 <1105066510@qq.com> 于2024年8月7日周三 13:35写道：
> this case will enter `if (IsA(groupexpr, RelabelType))` branch.
>
> We set "C" as groupkey's collation, and it's not equal to column's.
> In transformGroupClause(), syntax `COLLATE xxx` will create CollateExpr
> node, and CollateExpr's arg is Var node.
> And then, planner will call eval_const_expressions_mutator() to transform
> CollateExpr to RelableType if CollateExpr->collOid not equal to
> Collate->arg's collation.
>
Yech. Thanks for the explanation.
Because exprCollation() return the resultcollid if the nodetag is
Relabletype, the expr() && PartKeyCollMatchesExprColl() can handle this
situation.
So we don't need "if (IsA(groupexpr, RelableTyple) " this branch.
>
>
> About V3 patch, PartCollMatchesExprColl() may be not suitable, because
> collation of partkey must be equal to groupkey's, even though they are all
> InvalidOid.
>
I'm not sure about what you said. I uesd it because partition prune do
this way. I keep it temporarily.
The test case in v3 patch only has EXPLAIN statement. I add SELECT
statement and make the result more stable according to Aleksander advices.
--
Tender Wang
Attachments:
[application/octet-stream] v4-0001-fix-group_by_has_partkey-bug-and-add-regress-test.patch (6.1K, ../../CAHewXNnzROMZRt=GTmEw1araiAsbS68Bs+Lf3d0s=o01E5B6WQ@mail.gmail.com/3-v4-0001-fix-group_by_has_partkey-bug-and-add-regress-test.patch)
download | inline diff:
From 1a7fd763d5863be392b8b73e932c0145714cdbe7 Mon Sep 17 00:00:00 2001
From: hanweibo <hanweibo@highgo.com>
Date: Wed, 7 Aug 2024 09:25:34 +0800
Subject: [PATCH v4] fix group_by_has_partkey bug and add regress test case
---
src/backend/optimizer/plan/planner.c | 24 +++++++--
.../regress/expected/partition_aggregate.out | 53 +++++++++++++++++++
src/test/regress/sql/partition_aggregate.sql | 29 ++++++++++
3 files changed, 103 insertions(+), 3 deletions(-)
diff --git a/src/backend/optimizer/plan/planner.c b/src/backend/optimizer/plan/planner.c
index 948afd9094..88a8937b43 100644
--- a/src/backend/optimizer/plan/planner.c
+++ b/src/backend/optimizer/plan/planner.c
@@ -7993,6 +7993,10 @@ create_partitionwise_grouping_paths(PlannerInfo *root,
}
}
+/* XXX see PartCollMatchesExprColl */
+#define PartKeyCollMatchesExprColl(partcoll, exprcoll) \
+ ((partcoll) == InvalidOid || (partcoll) == (exprcoll))
+
/*
* group_by_has_partkey
*
@@ -8025,13 +8029,27 @@ group_by_has_partkey(RelOptInfo *input_rel,
foreach(lc, partexprs)
{
+ ListCell *lg;
Expr *partexpr = lfirst(lc);
+ Oid partcoll = input_rel->part_scheme->partcollation[cnt];
- if (list_member(groupexprs, partexpr))
+ foreach (lg, groupexprs)
{
- found = true;
- break;
+ Expr *groupexpr = lfirst(lg);
+ Oid groupexpr_coll = exprCollation((Node *) groupexpr);
+
+ /*
+ * Partition key match also requires collation match.
+ */
+ if (equal(groupexpr, partexpr) &&
+ PartKeyCollMatchesExprColl(partcoll, groupexpr_coll))
+ {
+ found = true;
+ break;
+ }
}
+ if (found)
+ break;
}
/*
diff --git a/src/test/regress/expected/partition_aggregate.out b/src/test/regress/expected/partition_aggregate.out
index 5f2c0cf578..bc4a1987a1 100644
--- a/src/test/regress/expected/partition_aggregate.out
+++ b/src/test/regress/expected/partition_aggregate.out
@@ -1518,3 +1518,56 @@ SELECT x, sum(y), avg(y), count(*) FROM pagg_tab_para GROUP BY x HAVING avg(y) <
21 | 6000 | 6.0000000000000000 | 1000
(6 rows)
+-- Add collation check between groupkey and partkey.
+-- If groupkey's collation is not equal to partkey's, we cannot use PARTITIONWISE_AGGREGATE_FULL.
+SET enable_partitionwise_aggregate TO true;
+SET enable_partitionwise_join TO true;
+SET max_parallel_workers_per_gather TO 0;
+SET enable_incremental_sort TO off;
+CREATE COLLATION case_insensitive (
+ provider = icu,
+ locale = 'und-u-ks-level2',
+ deterministic = false
+);
+CREATE TABLE pagg_tab3 (c text collate case_insensitive) PARTITION BY LIST(c collate "C");
+CREATE TABLE pagg_tab3_p1 PARTITION OF pagg_tab3 FOR VALUES IN ('a', 'b', 'c', 'd');
+CREATE TABLE pagg_tab3_p2 PARTITION OF pagg_tab3 FOR VALUES IN ('e', 'f', 'A');
+CREATE TABLE pagg_tab3_p3 PARTITION OF pagg_tab3 FOR VALUES IN ('B', 'C', 'D', 'E');
+INSERT INTO pagg_tab3 SELECT substr('abcdeABCDE', (i % 10) +1 , 1) FROM generate_series(0, 2999) i;
+ANALYZE pagg_tab3;
+EXPLAIN (COSTS OFF)
+SELECT upper(c collate case_insensitive), count(c) FROM pagg_tab3 GROUP BY c collate case_insensitive ORDER BY 1;
+ QUERY PLAN
+--------------------------------------------------------------------
+ Sort
+ Sort Key: (upper(pagg_tab3.c)) COLLATE case_insensitive
+ -> Finalize GroupAggregate
+ Group Key: pagg_tab3.c
+ -> Sort
+ Sort Key: pagg_tab3.c COLLATE case_insensitive
+ -> Append
+ -> Partial HashAggregate
+ Group Key: pagg_tab3.c
+ -> Seq Scan on pagg_tab3_p2 pagg_tab3
+ -> Partial HashAggregate
+ Group Key: pagg_tab3_1.c
+ -> Seq Scan on pagg_tab3_p3 pagg_tab3_1
+ -> Partial HashAggregate
+ Group Key: pagg_tab3_2.c
+ -> Seq Scan on pagg_tab3_p1 pagg_tab3_2
+(16 rows)
+
+SELECT upper(c collate case_insensitive), count(c) FROM pagg_tab3 GROUP BY c collate case_insensitive ORDER BY 1;
+ upper | count
+-------+-------
+ A | 600
+ B | 600
+ C | 600
+ D | 600
+ E | 600
+(5 rows)
+
+RESET enable_partitionwise_aggregate;
+RESET enable_partitionwise_join;
+RESET max_parallel_workers_per_gather;
+RESET enable_incremental_sort;
diff --git a/src/test/regress/sql/partition_aggregate.sql b/src/test/regress/sql/partition_aggregate.sql
index ab070fee24..8f2de9ee96 100644
--- a/src/test/regress/sql/partition_aggregate.sql
+++ b/src/test/regress/sql/partition_aggregate.sql
@@ -334,3 +334,32 @@ RESET parallel_setup_cost;
EXPLAIN (COSTS OFF)
SELECT x, sum(y), avg(y), count(*) FROM pagg_tab_para GROUP BY x HAVING avg(y) < 7 ORDER BY 1, 2, 3;
SELECT x, sum(y), avg(y), count(*) FROM pagg_tab_para GROUP BY x HAVING avg(y) < 7 ORDER BY 1, 2, 3;
+
+
+-- Add collation check between groupkey and partkey.
+-- If groupkey's collation is not equal to partkey's, we cannot use PARTITIONWISE_AGGREGATE_FULL.
+SET enable_partitionwise_aggregate TO true;
+SET enable_partitionwise_join TO true;
+SET max_parallel_workers_per_gather TO 0;
+SET enable_incremental_sort TO off;
+
+CREATE COLLATION case_insensitive (
+ provider = icu,
+ locale = 'und-u-ks-level2',
+ deterministic = false
+);
+
+CREATE TABLE pagg_tab3 (c text collate case_insensitive) PARTITION BY LIST(c collate "C");
+CREATE TABLE pagg_tab3_p1 PARTITION OF pagg_tab3 FOR VALUES IN ('a', 'b', 'c', 'd');
+CREATE TABLE pagg_tab3_p2 PARTITION OF pagg_tab3 FOR VALUES IN ('e', 'f', 'A');
+CREATE TABLE pagg_tab3_p3 PARTITION OF pagg_tab3 FOR VALUES IN ('B', 'C', 'D', 'E');
+INSERT INTO pagg_tab3 SELECT substr('abcdeABCDE', (i % 10) +1 , 1) FROM generate_series(0, 2999) i;
+ANALYZE pagg_tab3;
+EXPLAIN (COSTS OFF)
+SELECT upper(c collate case_insensitive), count(c) FROM pagg_tab3 GROUP BY c collate case_insensitive ORDER BY 1;
+SELECT upper(c collate case_insensitive), count(c) FROM pagg_tab3 GROUP BY c collate case_insensitive ORDER BY 1;
+
+RESET enable_partitionwise_aggregate;
+RESET enable_partitionwise_join;
+RESET max_parallel_workers_per_gather;
+RESET enable_incremental_sort;
--
2.34.1
^ permalink raw reply [nested|flat] 30+ messages in thread
* Re: BUG #18568: BUG: Result wrong when do group by on partition table!
@ 2024-08-07 09:23 Tender Wang <tndrwang@gmail.com>
parent: Tender Wang <tndrwang@gmail.com>
0 siblings, 0 replies; 30+ messages in thread
From: Tender Wang @ 2024-08-07 09:23 UTC (permalink / raw)
To: 狂奔的蜗牛 <1105066510@qq.com>; +Cc: pgsql-bugs <pgsql-bugs@lists.postgresql.org>; aleksander <aleksander@timescale.com>
狂奔的蜗牛 <1105066510@qq.com> 于2024年8月7日周三 16:06写道：
> PartCollMatchesExprColl() is not strict enough.
> If partcoll == InvalidOid and groupcoll != InvalidOid,
> PartCollMatchesExprColl() return true always.
> Just determine whether groupcoll is equal to partcoll, like this "partcoll
> == groupcoll".
>
> I understand what you said. Actually, I keep it just curious when partcoll
is InvalidOid. Why it works for partition prune.
Is it not same between check partkey is equal to groupexpr and check
partkey is equal to qualclause?
> We cannot delete "if (IsA(groupexpr, RelableTyple) " branch,
> becasuse if groupexpr is RelabelType and partcoll equal to groupcoll,
> the "equal(groupexpr, partexpr) && PartKeyCollMatchesExprColl(partcoll,
> groupexpr_coll)" condition return false!!!
> This is not what we expect.
>
" if groupexpr is RelabelType and partcoll equal to groupcoll ", according
to original logic, will return false in this situation.
Now you think we can support above situation. Am I understand correctly?
We're better to add more test case to cover the code if we're going to
support this. The test cases now seem not going
into the RelableTyple branch.
> We could rename "groupexpr_coll" to groupcoll, it looks more elegant.
>
> No objection. You can continue to support RelableType situation and add
more test cases based on V4.
--
Tender Wang
^ permalink raw reply [nested|flat] 30+ messages in thread
* Re: BUG #18568: BUG: Result wrong when do group by on partition table!
@ 2024-08-07 15:04 jian he <jian.universality@gmail.com>
0 siblings, 1 reply; 30+ messages in thread
From: jian he @ 2024-08-07 15:04 UTC (permalink / raw)
To: 狂奔的蜗牛 <1105066510@qq.com>; +Cc: Tender Wang <tndrwang@gmail.com>; pgsql-bugs <pgsql-bugs@lists.postgresql.org>; aleksander <aleksander@timescale.com>
On Wed, Aug 7, 2024 at 9:08 PM 狂奔的蜗牛 <1105066510@qq.com> wrote:
>
> I readd RelabelType to branch in v5 patch, and add case to cover the code.
>
> In new adding case: `SELECT c collate "C", count(c) FROM pagg_tab3 GROUP BY c collate "C" HAVING count(c) > 1 ORDER BY 1;`
> According to original logic, group_by_has_partkey() will return false, it will lead to cannot do having filter in sub table, the result is correct but the performance is poor.
> Our logic will get better performance.
>
>
> About InvalidOid collation, non-character type's collation is 0(InvalidOid) always.
>
src/test/regress/sql/partition_aggregate.sql
suppose to run with or without ICU, your new test requires ICU,
which will make some buildfarm fail?
put the test to src/test/regress/sql/collate.icu.utf8.sql
seems a little bit of strange.
also. see beginning of partition_aggregate.sql
-- Note: to ensure plan stability, it's a good idea to make the partitions of
-- any one partitioned table in this test all have different numbers of rows.
you may want to adjust the pagg_tab3 partitions rows.
some white space error.
git diff --check
src/backend/optimizer/plan/planner.c:8041: trailing whitespace.
+ * Set collation in group by
clause using COLLATE syntax,
src/backend/optimizer/plan/planner.c:8051: trailing whitespace.
+ }
^ permalink raw reply [nested|flat] 30+ messages in thread
* Re: BUG #18568: BUG: Result wrong when do group by on partition table!
@ 2024-08-08 07:51 jian he <jian.universality@gmail.com>
parent: jian he <jian.universality@gmail.com>
0 siblings, 1 reply; 30+ messages in thread
From: jian he @ 2024-08-08 07:51 UTC (permalink / raw)
To: 狂奔的蜗牛 <1105066510@qq.com>; +Cc: Tender Wang <tndrwang@gmail.com>; pgsql-bugs <pgsql-bugs@lists.postgresql.org>; aleksander <aleksander@timescale.com>
On Wed, Aug 7, 2024 at 11:04 PM jian he <jian.universality@gmail.com> wrote:
>
> On Wed, Aug 7, 2024 at 9:08 PM 狂奔的蜗牛 <1105066510@qq.com> wrote:
> >
> > I readd RelabelType to branch in v5 patch, and add case to cover the code.
> >
I did some minor adjustments in group_by_has_partkey, also refactored
the comment.
Attachments:
[text/x-patch] partition_key_collation.diff (1.8K, ../../CACJufxFX4Bw0xh-ex8cNakB1852d5x8kCwsFrkUcW4=3=qJvog@mail.gmail.com/2-partition_key_collation.diff)
download | inline diff:
diff --git a/src/backend/optimizer/plan/planner.c b/src/backend/optimizer/plan/planner.c
index 948afd90..bf96c0ce 100644
--- a/src/backend/optimizer/plan/planner.c
+++ b/src/backend/optimizer/plan/planner.c
@@ -7998,6 +7998,10 @@ create_partitionwise_grouping_paths(PlannerInfo *root,
*
* Returns true, if all the partition keys of the given relation are part of
* the GROUP BY clauses, false otherwise.
+ * NB: partition key's collation is stored at part_scheme->partcollation.
+ * When comparing GROUP BY clauses with partition keys, we need check
+ * collation of part_scheme->partcollation, not the collation of input_rel->partexprs,
+ * since these two maybe different.
*/
static bool
group_by_has_partkey(RelOptInfo *input_rel,
@@ -8007,6 +8011,8 @@ group_by_has_partkey(RelOptInfo *input_rel,
List *groupexprs = get_sortgrouplist_exprs(groupClause, targetList);
int cnt = 0;
int partnatts;
+ Oid groupcoll = InvalidOid;
+ Oid partcoll = InvalidOid;
/* Input relation should be partitioned. */
Assert(input_rel->part_scheme);
@@ -8023,15 +8029,31 @@ group_by_has_partkey(RelOptInfo *input_rel,
ListCell *lc;
bool found = false;
+ partcoll = input_rel->part_scheme->partcollation[cnt];
foreach(lc, partexprs)
{
+ ListCell *lg;
Expr *partexpr = lfirst(lc);
- if (list_member(groupexprs, partexpr))
+ foreach(lg, groupexprs)
{
- found = true;
- break;
+ Expr *groupexpr = lfirst(lg);
+ groupcoll = exprCollation((Node *) groupexpr);
+
+ if (partcoll == groupcoll)
+ {
+ if (IsA(groupexpr, RelabelType))
+ groupexpr = ((RelabelType *) groupexpr)->arg;
+
+ if (equal(groupexpr, partexpr))
+ {
+ found = true;
+ break;
+ }
+ }
}
+ if (found)
+ break;
}
/*
^ permalink raw reply [nested|flat] 30+ messages in thread
* Re: BUG #18568: BUG: Result wrong when do group by on partition table!
@ 2024-08-08 08:20 Tender Wang <tndrwang@gmail.com>
parent: jian he <jian.universality@gmail.com>
0 siblings, 1 reply; 30+ messages in thread
From: Tender Wang @ 2024-08-08 08:20 UTC (permalink / raw)
To: jian he <jian.universality@gmail.com>; +Cc: 狂奔的蜗牛 <1105066510@qq.com>; pgsql-bugs <pgsql-bugs@lists.postgresql.org>; aleksander <aleksander@timescale.com>
jian he <jian.universality@gmail.com> 于2024年8月8日周四 15:51写道：
> On Wed, Aug 7, 2024 at 11:04 PM jian he <jian.universality@gmail.com>
> wrote:
> >
> > On Wed, Aug 7, 2024 at 9:08 PM 狂奔的蜗牛 <1105066510@qq.com> wrote:
> > >
> > > I readd RelabelType to branch in v5 patch, and add case to cover the
> code.
> > >
>
>
> I did some minor adjustments in group_by_has_partkey, also refactored
> the comment.
>
The logic in patch I think no problem. But I don't like the code style.
if (IsA(groupexpr, RelabelType))
groupexpr = ((RelabelType *) groupexpr)->arg;
if (expr(groupexpr, partexr) && partcoll == groupcoll)
{
...
}
This looks more compact.
By the way, I think we'd better to include test cases together in your
patch, then others can continue
to work based on your patch not need to go back to add the test cases.
--
Tender Wang
^ permalink raw reply [nested|flat] 30+ messages in thread
* Re: BUG #18568: BUG: Result wrong when do group by on partition table!
@ 2024-08-08 09:46 Tender Wang <tndrwang@gmail.com>
parent: Tender Wang <tndrwang@gmail.com>
0 siblings, 1 reply; 30+ messages in thread
From: Tender Wang @ 2024-08-08 09:46 UTC (permalink / raw)
To: 狂奔的蜗牛 <1105066510@qq.com>; +Cc: jian he <jian.universality@gmail.com>; pgsql-bugs <pgsql-bugs@lists.postgresql.org>; aleksander <aleksander@timescale.com>
狂奔的蜗牛 <1105066510@qq.com> 于2024年8月8日周四 16:44写道：
> I agree with advice of code style, and I have trim trailing whitespace in
> V6 patch.
>
The v6 patch still has code format issue, and the v6 looks almost same to
v5.
git am v6-0001-fix-group_by_has_partkey-bug-and-add-regress-test.patch
Applying: fix-group_by_has_partkey-bug-and-add-regress-test
.git/rebase-apply/patch:31: trailing whitespace.
* Set collation in group by clause
using COLLATE syntax,
.git/rebase-apply/patch:41: trailing whitespace.
}
--
Tender Wang
^ permalink raw reply [nested|flat] 30+ messages in thread
* Re: BUG #18568: BUG: Result wrong when do group by on partition table!
@ 2024-10-21 08:10 jian he <jian.universality@gmail.com>
parent: Tender Wang <tndrwang@gmail.com>
0 siblings, 1 reply; 30+ messages in thread
From: jian he @ 2024-10-21 08:10 UTC (permalink / raw)
To: 狂奔的蜗牛 <1105066510@qq.com>; +Cc: Tender Wang <tndrwang@gmail.com>; pgsql-bugs <pgsql-bugs@lists.postgresql.org>; aleksander <aleksander@timescale.com>
On Thu, Aug 8, 2024 at 7:21 PM 狂奔的蜗牛 <1105066510@qq.com> wrote:
>
> Oh no!
> Sorry, I made a mistake on patch.
> v7 is new patch.
regress tests will fail for build with.
-Dicu=disabled
So I guess you have to put
src/test/regress/sql/partition_aggregate.sql related tests to
src/test/regress/sql/collate.icu.utf8.sql.
foreach(lc, partexprs)
{
ListCell *lg;
Expr *partexpr = lfirst(lc);
Oid partcoll =
input_rel->part_scheme->partcollation[cnt];
foreach (lg, groupexprs)
{
Expr *groupexpr = lfirst(lg);
Oid groupcoll = exprCollation((Node *) groupexpr);
/*
* GROUP BY xx COLLATE xxx
* Set collation in group by clause using COLLATE syntax,
* AND the collation IS NOT equal to column's default collation.
*/
if (IsA(groupexpr, RelabelType))
groupexpr = ((RelabelType *) groupexpr)->arg;
if (equal(groupexpr, partexpr) && partcoll == groupcoll)
{
found = true;
break;
}
}
if (found)
break;
}
/*
* GROUP BY xx COLLATE xxx
* Set collation in group by clause using COLLATE syntax,
* AND the collation IS NOT equal to column's default collation.
*/
i can understand the first part of the comment.
but here it is not related to column collation,
It's related to collation comparison between groupby expression and
partition key expressions.
so the second part of the comments confused me.
I added a commitfest entry in
https://commitfest.postgresql.org/50/5312/
you can put your name in the Authors field.
Other than that, it looks good to me.
^ permalink raw reply [nested|flat] 30+ messages in thread
* Re: BUG #18568: BUG: Result wrong when do group by on partition table!
@ 2024-10-22 07:33 Amit Langote <amitlangote09@gmail.com>
parent: Tender Wang <tndrwang@gmail.com>
0 siblings, 2 replies; 30+ messages in thread
From: Amit Langote @ 2024-10-22 07:33 UTC (permalink / raw)
To: Tender Wang <tndrwang@gmail.com>; +Cc: 1105066510@qq.com; pgsql-bugs@lists.postgresql.org
Hi,
On Tue, Aug 6, 2024 at 10:24 PM Tender Wang <tndrwang@gmail.com> wrote:
> PG Bug reporting form <noreply@postgresql.org> 于2024年8月6日周二 15:01写道：
>>
>> The following bug has been logged on the website:
>>
>> Bug reference: 18568
>> Logged by: Webbo Han
>> Email address: 1105066510@qq.com
>> PostgreSQL version: 16.3
>> Operating system: centos 7.6
>> Description:
>>
>> First, we create one case-insensitive collation use ICU:
>> ```sql
>> CREATE COLLATION case_insensitive (
>> provider = icu,
>> locale = 'und-u-ks-level2',
>> deterministic = false
>> );
>> ```
>>
>> Then, we create the partition table, meanwhile we set the collation of
>> column c to `case_insensitive`,
>> and set partkey's collation to 'C'.
>> ```sql
>> SET enable_partitionwise_aggregate TO true;
>> SET enable_partitionwise_join TO true;
>> SET max_parallel_workers_per_gather TO 0;
>> SET enable_incremental_sort TO off;
>> CREATE TABLE pagg_tab (c text collate case_insensitive) PARTITION BY LIST(c
>> collate "C");
>
>
> I think above create table again. Should we allow this CREATE TABLE? The partition key
> definition are not same with column definiton. Is it better to report error for users?
Not really. As the documentation says, collation can be specified per
column or per operation:
https://www.postgresql.org/docs/current/collation.html
In this case, the operation is partitioning. When you specify the
COLLATE clause for a partition key, it means that the partitioning
logic, such as partition tuple routing, will use that collation
instead of the column-specified or the column type's collation.
That's similar to how you can create an index on a column using a
different collation than the column's own:
create table foo (a text collate case_insensitive);
create index on foo (a collate "C");
I do notice that the CREATE TABLE documentation does not describe what
the COLLATE clause does when mentioned in a PARTITION BY clause.
We'll need to fix that.
--
Thanks, Amit Langote
^ permalink raw reply [nested|flat] 30+ messages in thread
* Re: BUG #18568: BUG: Result wrong when do group by on partition table!
@ 2024-10-22 07:46 Tender Wang <tndrwang@gmail.com>
parent: Amit Langote <amitlangote09@gmail.com>
1 sibling, 0 replies; 30+ messages in thread
From: Tender Wang @ 2024-10-22 07:46 UTC (permalink / raw)
To: Amit Langote <amitlangote09@gmail.com>; +Cc: 1105066510@qq.com; pgsql-bugs@lists.postgresql.org
Amit Langote <amitlangote09@gmail.com> 于2024年10月22日周二 15:33写道：
> Hi,
>
> On Tue, Aug 6, 2024 at 10:24 PM Tender Wang <tndrwang@gmail.com> wrote:
> > PG Bug reporting form <noreply@postgresql.org> 于2024年8月6日周二 15:01写道：
> >>
> >> The following bug has been logged on the website:
> >>
> >> Bug reference: 18568
> >> Logged by: Webbo Han
> >> Email address: 1105066510@qq.com
> >> PostgreSQL version: 16.3
> >> Operating system: centos 7.6
> >> Description:
> >>
> >> First, we create one case-insensitive collation use ICU:
> >> ```sql
> >> CREATE COLLATION case_insensitive (
> >> provider = icu,
> >> locale = 'und-u-ks-level2',
> >> deterministic = false
> >> );
> >> ```
> >>
> >> Then, we create the partition table, meanwhile we set the collation of
> >> column c to `case_insensitive`,
> >> and set partkey's collation to 'C'.
> >> ```sql
> >> SET enable_partitionwise_aggregate TO true;
> >> SET enable_partitionwise_join TO true;
> >> SET max_parallel_workers_per_gather TO 0;
> >> SET enable_incremental_sort TO off;
> >> CREATE TABLE pagg_tab (c text collate case_insensitive)
> PARTITION BY LIST(c
> >> collate "C");
> >
> >
> > I think above create table again. Should we allow this CREATE TABLE?
> The partition key
> > definition are not same with column definiton. Is it better to report
> error for users?
>
> Not really. As the documentation says, collation can be specified per
> column or per operation:
>
> https://www.postgresql.org/docs/current/collation.html
>
> In this case, the operation is partitioning. When you specify the
> COLLATE clause for a partition key, it means that the partitioning
> logic, such as partition tuple routing, will use that collation
> instead of the column-specified or the column type's collation.
>
> That's similar to how you can create an index on a column using a
> different collation than the column's own:
>
> create table foo (a text collate case_insensitive);
> create index on foo (a collate "C");
>
>
Thanks for your explanation.
> I do notice that the CREATE TABLE documentation does not describe what
> the COLLATE clause does when mentioned in a PARTITION BY clause.
> We'll need to fix that.
>
Agree.
--
Thanks,
Tender Wang
^ permalink raw reply [nested|flat] 30+ messages in thread
* Re: BUG #18568: BUG: Result wrong when do group by on partition table!
@ 2024-10-22 08:30 Tender Wang <tndrwang@gmail.com>
parent: Amit Langote <amitlangote09@gmail.com>
1 sibling, 1 reply; 30+ messages in thread
From: Tender Wang @ 2024-10-22 08:30 UTC (permalink / raw)
To: Amit Langote <amitlangote09@gmail.com>; +Cc: 1105066510@qq.com; pgsql-bugs@lists.postgresql.org
Hi Amit,
Amit Langote <amitlangote09@gmail.com> 于2024年10月22日周二 15:33写道：
>
>
> Not really. As the documentation says, collation can be specified per
> column or per operation:
>
> https://www.postgresql.org/docs/current/collation.html
>
> In this case, the operation is partitioning. When you specify the
> COLLATE clause for a partition key, it means that the partitioning
> logic, such as partition tuple routing, will use that collation
> instead of the column-specified or the column type's collation.
>
Since you said partition key had its own collation, and but we used column
type's collation in
set_baserel_partition_key_exprs() as below:
partexpr = (Expr *) makeVar(varno, attno,
partkey->parttypid[cnt],
partkey->parttypmod[cnt],
partkey->parttypcoll[cnt], 0);
I think why not we directly use the partition key collation(e.g.
partcollation).
--
Thanks,
Tender Wang
^ permalink raw reply [nested|flat] 30+ messages in thread
* Re: BUG #18568: BUG: Result wrong when do group by on partition table!
@ 2024-10-22 09:25 Amit Langote <amitlangote09@gmail.com>
parent: Tender Wang <tndrwang@gmail.com>
0 siblings, 2 replies; 30+ messages in thread
From: Amit Langote @ 2024-10-22 09:25 UTC (permalink / raw)
To: Tender Wang <tndrwang@gmail.com>; +Cc: 1105066510@qq.com; pgsql-bugs@lists.postgresql.org
On Tue, Oct 22, 2024 at 5:30 PM Tender Wang <tndrwang@gmail.com> wrote:
> Amit Langote <amitlangote09@gmail.com> 于2024年10月22日周二 15:33写道：
>> Not really. As the documentation says, collation can be specified per
>> column or per operation:
>>
>> https://www.postgresql.org/docs/current/collation.html
>>
>> In this case, the operation is partitioning. When you specify the
>> COLLATE clause for a partition key, it means that the partitioning
>> logic, such as partition tuple routing, will use that collation
>> instead of the column-specified or the column type's collation.
>
>
> Since you said partition key had its own collation, and but we used column type's collation in
> set_baserel_partition_key_exprs() as below:
>
> partexpr = (Expr *) makeVar(varno, attno,
> partkey->parttypid[cnt],
> partkey->parttypmod[cnt],
> partkey->parttypcoll[cnt], 0);
>
> I think why not we directly use the partition key collation(e.g. partcollation).
That's a good question but I don't immediately know the answer.
It seems like it has been like this since the beginning or since the
commit that added the RelOptInfo.partexprs field (9140cf8269).
--
Thanks, Amit Langote
^ permalink raw reply [nested|flat] 30+ messages in thread
* Re: BUG #18568: BUG: Result wrong when do group by on partition table!
@ 2024-10-22 11:14 Tender Wang <tndrwang@gmail.com>
parent: Amit Langote <amitlangote09@gmail.com>
1 sibling, 1 reply; 30+ messages in thread
From: Tender Wang @ 2024-10-22 11:14 UTC (permalink / raw)
To: Amit Langote <amitlangote09@gmail.com>; +Cc: 1105066510@qq.com; pgsql-bugs@lists.postgresql.org
Amit Langote <amitlangote09@gmail.com> 于2024年10月22日周二 17:25写道：
> On Tue, Oct 22, 2024 at 5:30 PM Tender Wang <tndrwang@gmail.com> wrote:
> > Amit Langote <amitlangote09@gmail.com> 于2024年10月22日周二 15:33写道：
> >> Not really. As the documentation says, collation can be specified per
> >> column or per operation:
> >>
> >> https://www.postgresql.org/docs/current/collation.html
> >>
> >> In this case, the operation is partitioning. When you specify the
> >> COLLATE clause for a partition key, it means that the partitioning
> >> logic, such as partition tuple routing, will use that collation
> >> instead of the column-specified or the column type's collation.
> >
> >
> > Since you said partition key had its own collation, and but we used
> column type's collation in
> > set_baserel_partition_key_exprs() as below:
> >
> > partexpr = (Expr *) makeVar(varno, attno,
> > partkey->parttypid[cnt],
> > partkey->parttypmod[cnt],
> > partkey->parttypcoll[cnt], 0);
> >
> > I think why not we directly use the partition key collation(e.g.
> partcollation).
>
> That's a good question but I don't immediately know the answer.
>
> It seems like it has been like this since the beginning or since the
> commit that added the RelOptInfo.partexprs field (9140cf8269).
>
>
When I looked at the commit(9140cf8269), I found that in 9140cf8269, the
PartitionSchemeData struct had
a filed: Oid *parttypcoll; whose value was equal to column type's
collation. But now HEAD this field has changed to
Oid *partcollation; whose value is equal to partition key collation. This
commit 2af28e6 made above change.
commit 2af28e603319224e87fd35ab62f36ef6de45eaac
Author: Robert Haas <rhaas@postgresql.org>
Date: Wed Feb 28 12:16:09 2018 -0500
For partitionwise join, match on partcollation, not parttypcoll.
The previous code considered two tables to have the partition scheme
if the underlying columns had the same collation, but what we
actually need to compare is not the collations associated with the
column but the collation used for partitioning. Fix that.
Robert Haas and Amit Langote
Discussion:
http://postgr.es/m/0f95f924-0efa-4cf5-eb5f-9a3d1bc3c33d@lab.ntt.co.jp
Now I suspect the commit 2af28e6 forgot to fix RelOptInfo's partexprs
collation.
If we change the partexprs collation to Partition Key collation, as I said
in [1], this bug will not happen.
But I'm not sure this fix will affect other codes that use RelOptInfo's
partexprs.
Any thoughts?
[1]
https://www.postgresql.org/message-id/CAHewXNnKLrZYG4iqaYw%3DuB3XWRrYRZHo7VtcMsbUEbdbajQg2Q%40mail.g...
--
Thanks,
Tender Wang
^ permalink raw reply [nested|flat] 30+ messages in thread
* Re: BUG #18568: BUG: Result wrong when do group by on partition table!
@ 2024-10-22 12:11 Amit Langote <amitlangote09@gmail.com>
parent: Tender Wang <tndrwang@gmail.com>
0 siblings, 1 reply; 30+ messages in thread
From: Amit Langote @ 2024-10-22 12:11 UTC (permalink / raw)
To: Tender Wang <tndrwang@gmail.com>; +Cc: 1105066510@qq.com; pgsql-bugs@lists.postgresql.org
Hi,
On Tue, Oct 22, 2024 at 8:14 PM Tender Wang <tndrwang@gmail.com> wrote:
> Amit Langote <amitlangote09@gmail.com> 于2024年10月22日周二 17:25写道：
>> On Tue, Oct 22, 2024 at 5:30 PM Tender Wang <tndrwang@gmail.com> wrote:
>> > Amit Langote <amitlangote09@gmail.com> 于2024年10月22日周二 15:33写道：
>> >> Not really. As the documentation says, collation can be specified per
>> >> column or per operation:
>> >>
>> >> https://www.postgresql.org/docs/current/collation.html
>> >>
>> >> In this case, the operation is partitioning. When you specify the
>> >> COLLATE clause for a partition key, it means that the partitioning
>> >> logic, such as partition tuple routing, will use that collation
>> >> instead of the column-specified or the column type's collation.
>> >
>> >
>> > Since you said partition key had its own collation, and but we used column type's collation in
>> > set_baserel_partition_key_exprs() as below:
>> >
>> > partexpr = (Expr *) makeVar(varno, attno,
>> > partkey->parttypid[cnt],
>> > partkey->parttypmod[cnt],
>> > partkey->parttypcoll[cnt], 0);
>> >
>> > I think why not we directly use the partition key collation(e.g. partcollation).
>>
>> That's a good question but I don't immediately know the answer.
>>
>> It seems like it has been like this since the beginning or since the
>> commit that added the RelOptInfo.partexprs field (9140cf8269).
>
> When I looked at the commit(9140cf8269), I found that in 9140cf8269, the PartitionSchemeData struct had
> a filed: Oid *parttypcoll; whose value was equal to column type's collation. But now HEAD this field has changed to
> Oid *partcollation; whose value is equal to partition key collation. This commit 2af28e6 made above change.
>
> commit 2af28e603319224e87fd35ab62f36ef6de45eaac
> Author: Robert Haas <rhaas@postgresql.org>
> Date: Wed Feb 28 12:16:09 2018 -0500
>
> For partitionwise join, match on partcollation, not parttypcoll.
>
> The previous code considered two tables to have the partition scheme
> if the underlying columns had the same collation, but what we
> actually need to compare is not the collations associated with the
> column but the collation used for partitioning. Fix that.
>
> Robert Haas and Amit Langote
>
> Discussion: http://postgr.es/m/0f95f924-0efa-4cf5-eb5f-9a3d1bc3c33d@lab.ntt.co.jp
Good find.
> Now I suspect the commit 2af28e6 forgot to fix RelOptInfo's partexprs collation.
> If we change the partexprs collation to Partition Key collation, as I said in [1], this bug will not happen.
> But I'm not sure this fix will affect other codes that use RelOptInfo's partexprs.
Yeah, I suspect we could maybe just put the collation from
partcollation into varcollid of the partition key Var, but what about
partition keys that are not simple column references? Would they
carry the correct collation such that exprCollation() returns the
right collation OID?
Also, we'll need to be sure that the semantic of anything that's using
partexprs is not broken because of this.
--
Thanks, Amit Langote
^ permalink raw reply [nested|flat] 30+ messages in thread
* Re: BUG #18568: BUG: Result wrong when do group by on partition table!
@ 2024-10-22 12:28 Amit Langote <amitlangote09@gmail.com>
parent: jian he <jian.universality@gmail.com>
0 siblings, 1 reply; 30+ messages in thread
From: Amit Langote @ 2024-10-22 12:28 UTC (permalink / raw)
To: jian he <jian.universality@gmail.com>; +Cc: 狂奔的蜗牛 <1105066510@qq.com>; Tender Wang <tndrwang@gmail.com>; pgsql-bugs <pgsql-bugs@lists.postgresql.org>; aleksander <aleksander@timescale.com>
Hi,
On Mon, Oct 21, 2024 at 5:10 PM jian he <jian.universality@gmail.com> wrote:
>
> On Thu, Aug 8, 2024 at 7:21 PM 狂奔的蜗牛 <1105066510@qq.com> wrote:
> >
> > Oh no!
> > Sorry, I made a mistake on patch.
> > v7 is new patch.
>
> regress tests will fail for build with.
> -Dicu=disabled
>
> So I guess you have to put
> src/test/regress/sql/partition_aggregate.sql related tests to
> src/test/regress/sql/collate.icu.utf8.sql.
Yes, I can see a regression diff such as this one when I push the
patch to Cirrus CI:
diff -U3 /tmp/cirrus-ci-build/src/test/regress/expected/partition_aggregate.out
/tmp/cirrus-ci-build/build-32/testrun/regress/regress/results/partition_aggregate.out
--- /tmp/cirrus-ci-build/src/test/regress/expected/partition_aggregate.out
2024-10-22 07:42:10.098181779 +0000
+++ /tmp/cirrus-ci-build/build-32/testrun/regress/regress/results/partition_aggregate.out
2024-10-22 07:51:54.836528856 +0000
@@ -1529,115 +1529,52 @@
locale = 'und-u-ks-level2',
deterministic = false
);
+ERROR: current database's encoding is not supported with this provider
--
Thanks, Amit Langote
^ permalink raw reply [nested|flat] 30+ messages in thread
* Re: BUG #18568: BUG: Result wrong when do group by on partition table!
@ 2024-10-22 14:29 Tender Wang <tndrwang@gmail.com>
parent: Amit Langote <amitlangote09@gmail.com>
0 siblings, 0 replies; 30+ messages in thread
From: Tender Wang @ 2024-10-22 14:29 UTC (permalink / raw)
To: Amit Langote <amitlangote09@gmail.com>; +Cc: 1105066510@qq.com; pgsql-bugs@lists.postgresql.org
Amit Langote <amitlangote09@gmail.com> 于2024年10月22日周二 20:11写道：
> Hi,
>
> On Tue, Oct 22, 2024 at 8:14 PM Tender Wang <tndrwang@gmail.com> wrote:
> > Amit Langote <amitlangote09@gmail.com> 于2024年10月22日周二 17:25写道：
> >> On Tue, Oct 22, 2024 at 5:30 PM Tender Wang <tndrwang@gmail.com> wrote:
> >> > Amit Langote <amitlangote09@gmail.com> 于2024年10月22日周二 15:33写道：
> >> >> Not really. As the documentation says, collation can be specified
> per
> >> >> column or per operation:
> >> >>
> >> >> https://www.postgresql.org/docs/current/collation.html
> >> >>
> >> >> In this case, the operation is partitioning. When you specify the
> >> >> COLLATE clause for a partition key, it means that the partitioning
> >> >> logic, such as partition tuple routing, will use that collation
> >> >> instead of the column-specified or the column type's collation.
> >> >
> >> >
> >> > Since you said partition key had its own collation, and but we used
> column type's collation in
> >> > set_baserel_partition_key_exprs() as below:
> >> >
> >> > partexpr = (Expr *) makeVar(varno, attno,
> >> > partkey->parttypid[cnt],
> >> > partkey->parttypmod[cnt],
> >> > partkey->parttypcoll[cnt], 0);
> >> >
> >> > I think why not we directly use the partition key collation(e.g.
> partcollation).
> >>
> >> That's a good question but I don't immediately know the answer.
> >>
> >> It seems like it has been like this since the beginning or since the
> >> commit that added the RelOptInfo.partexprs field (9140cf8269).
> >
> > When I looked at the commit(9140cf8269), I found that in 9140cf8269,
> the PartitionSchemeData struct had
> > a filed: Oid *parttypcoll; whose value was equal to column type's
> collation. But now HEAD this field has changed to
> > Oid *partcollation; whose value is equal to partition key collation.
> This commit 2af28e6 made above change.
> >
> > commit 2af28e603319224e87fd35ab62f36ef6de45eaac
> > Author: Robert Haas <rhaas@postgresql.org>
> > Date: Wed Feb 28 12:16:09 2018 -0500
> >
> > For partitionwise join, match on partcollation, not parttypcoll.
> >
> > The previous code considered two tables to have the partition scheme
> > if the underlying columns had the same collation, but what we
> > actually need to compare is not the collations associated with the
> > column but the collation used for partitioning. Fix that.
> >
> > Robert Haas and Amit Langote
> >
> > Discussion:
> http://postgr.es/m/0f95f924-0efa-4cf5-eb5f-9a3d1bc3c33d@lab.ntt.co.jp
>
> Good find.
>
> > Now I suspect the commit 2af28e6 forgot to fix RelOptInfo's partexprs
> collation.
> > If we change the partexprs collation to Partition Key collation, as I
> said in [1], this bug will not happen.
> > But I'm not sure this fix will affect other codes that use RelOptInfo's
> partexprs.
>
> Yeah, I suspect we could maybe just put the collation from
> partcollation into varcollid of the partition key Var, but what about
> partition keys that are not simple column references? Would they
> carry the correct collation such that exprCollation() returns the
> right collation OID?
>
The value of RelOptInfo's partexprs actually are assigned from
RelationData's rd_partkey.
And the rd_partkey is filled from pg_partitioned_table. The partcollation
also gets from pg_partitioned_table,
even though the partition key is not simple column reference, the collation
is correct.
> Also, we'll need to be sure that the semantic of anything that's using
> partexprs is not broken because of this.
>
>
Yeah. We should be careful.
--
Thanks,
Tender Wang
^ permalink raw reply [nested|flat] 30+ messages in thread
* Re: BUG #18568: BUG: Result wrong when do group by on partition table!
@ 2024-10-23 01:12 Amit Langote <amitlangote09@gmail.com>
parent: Amit Langote <amitlangote09@gmail.com>
0 siblings, 0 replies; 30+ messages in thread
From: Amit Langote @ 2024-10-23 01:12 UTC (permalink / raw)
To: jian he <jian.universality@gmail.com>; +Cc: 狂奔的蜗牛 <1105066510@qq.com>; Tender Wang <tndrwang@gmail.com>; pgsql-bugs <pgsql-bugs@lists.postgresql.org>; aleksander <aleksander@timescale.com>
On Tue, Oct 22, 2024 at 9:28 PM Amit Langote <amitlangote09@gmail.com> wrote:
> On Mon, Oct 21, 2024 at 5:10 PM jian he <jian.universality@gmail.com> wrote:
> >
> > On Thu, Aug 8, 2024 at 7:21 PM 狂奔的蜗牛 <1105066510@qq.com> wrote:
> > >
> > > Oh no!
> > > Sorry, I made a mistake on patch.
> > > v7 is new patch.
> >
> > regress tests will fail for build with.
> > -Dicu=disabled
> >
> > So I guess you have to put
> > src/test/regress/sql/partition_aggregate.sql related tests to
> > src/test/regress/sql/collate.icu.utf8.sql.
>
> Yes, I can see a regression diff such as this one when I push the
> patch to Cirrus CI:
>
> diff -U3 /tmp/cirrus-ci-build/src/test/regress/expected/partition_aggregate.out
> /tmp/cirrus-ci-build/build-32/testrun/regress/regress/results/partition_aggregate.out
> --- /tmp/cirrus-ci-build/src/test/regress/expected/partition_aggregate.out
> 2024-10-22 07:42:10.098181779 +0000
> +++ /tmp/cirrus-ci-build/build-32/testrun/regress/regress/results/partition_aggregate.out
> 2024-10-22 07:51:54.836528856 +0000
> @@ -1529,115 +1529,52 @@
> locale = 'und-u-ks-level2',
> deterministic = false
> );
> +ERROR: current database's encoding is not supported with this provider
My last 2 emails to 狂奔的蜗牛 (1105066510@qq.com) have been blocked.
--
Thanks, Amit Langote
^ permalink raw reply [nested|flat] 30+ messages in thread
* Re: BUG #18568: BUG: Result wrong when do group by on partition table!
@ 2024-10-23 06:25 Tender Wang <tndrwang@gmail.com>
parent: Amit Langote <amitlangote09@gmail.com>
1 sibling, 1 reply; 30+ messages in thread
From: Tender Wang @ 2024-10-23 06:25 UTC (permalink / raw)
To: Amit Langote <amitlangote09@gmail.com>; +Cc: 1105066510@qq.com; pgsql-bugs@lists.postgresql.org
Amit Langote <amitlangote09@gmail.com> 于2024年10月22日周二 17:25写道：
> On Tue, Oct 22, 2024 at 5:30 PM Tender Wang <tndrwang@gmail.com> wrote:
> > Amit Langote <amitlangote09@gmail.com> 于2024年10月22日周二 15:33写道：
> >> Not really. As the documentation says, collation can be specified per
> >> column or per operation:
> >>
> >> https://www.postgresql.org/docs/current/collation.html
> >>
> >> In this case, the operation is partitioning. When you specify the
> >> COLLATE clause for a partition key, it means that the partitioning
> >> logic, such as partition tuple routing, will use that collation
> >> instead of the column-specified or the column type's collation.
> >
> >
> > Since you said partition key had its own collation, and but we used
> column type's collation in
> > set_baserel_partition_key_exprs() as below:
> >
> > partexpr = (Expr *) makeVar(varno, attno,
> > partkey->parttypid[cnt],
> > partkey->parttypmod[cnt],
> > partkey->parttypcoll[cnt], 0);
> >
> > I think why not we directly use the partition key collation(e.g.
> partcollation).
>
> That's a good question but I don't immediately know the answer.
>
> It seems like it has been like this since the beginning or since the
> commit that added the RelOptInfo.partexprs field (9140cf8269).
>
Hi Amit,
I have another case that can confirm what I suspect. I now think in
set_baserel_partition_key_exprs(),
we should use partkey->partcollation[cnt] instead of
partkey->parttypcoll[cnt].
I find a wrong result when enable partitionwise_join, as below:
postgres=# set enable_partitionwise_join = on;
SET
postgres=# set max_parallel_workers_per_gather = 0;
SET
postgres=# explain select count(*) from pagg_tab t1 join pagg_tab t2 on
t1.c = t2.c;
QUERY PLAN
--------------------------------------------------------------------------------------------
Aggregate (cost=17010.00..17010.01 rows=1 width=8)
-> Append (cost=16.50..14760.00 rows=900000 width=0)
-> Hash Join (cost=16.50..2052.00 rows=180000 width=0)
Hash Cond: (t1_1.c = t2_1.c)
-> Seq Scan on pagg_tab_p2 t1_1 (cost=0.00..9.00 rows=600
width=2)
-> Hash (cost=9.00..9.00 rows=600 width=2)
-> Seq Scan on pagg_tab_p2 t2_1 (cost=0.00..9.00
rows=600 width=2)
-> Hash Join (cost=33.00..4104.00 rows=360000 width=0)
Hash Cond: (t1_2.c = t2_2.c)
-> Seq Scan on pagg_tab_p3 t1_2 (cost=0.00..18.00
rows=1200 width=2)
-> Hash (cost=18.00..18.00 rows=1200 width=2)
-> Seq Scan on pagg_tab_p3 t2_2 (cost=0.00..18.00
rows=1200 width=2)
-> Hash Join (cost=33.00..4104.00 rows=360000 width=0)
Hash Cond: (t1_3.c = t2_3.c)
-> Seq Scan on pagg_tab_p1 t1_3 (cost=0.00..18.00
rows=1200 width=2)
-> Hash (cost=18.00..18.00 rows=1200 width=2)
-> Seq Scan on pagg_tab_p1 t2_3 (cost=0.00..18.00
rows=1200 width=2)
(17 rows)
postgres=# select count(*) from pagg_tab t1 join pagg_tab t2 on t1.c =
t2.c;
count
--------
900000
(1 row)
postgres=# set enable_partitionwise_join = off;
SET
postgres=# explain select count(*) from pagg_tab t1 join pagg_tab t2 on
t1.c = t2.c;
QUERY PLAN
--------------------------------------------------------------------------------------------
Aggregate (cost=1269.02..1269.03 rows=1 width=8)
-> Merge Join (cost=466.52..1156.52 rows=45000 width=0)
Merge Cond: (t1.c = t2.c)
-> Sort (cost=233.26..240.76 rows=3000 width=2)
Sort Key: t1.c COLLATE case_insensitive
-> Append (cost=0.00..60.00 rows=3000 width=2)
-> Seq Scan on pagg_tab_p2 t1_1 (cost=0.00..9.00
rows=600 width=2)
-> Seq Scan on pagg_tab_p3 t1_2 (cost=0.00..18.00
rows=1200 width=2)
-> Seq Scan on pagg_tab_p1 t1_3 (cost=0.00..18.00
rows=1200 width=2)
-> Sort (cost=233.26..240.76 rows=3000 width=2)
Sort Key: t2.c COLLATE case_insensitive
-> Append (cost=0.00..60.00 rows=3000 width=2)
-> Seq Scan on pagg_tab_p2 t2_1 (cost=0.00..9.00
rows=600 width=2)
-> Seq Scan on pagg_tab_p3 t2_2 (cost=0.00..18.00
rows=1200 width=2)
-> Seq Scan on pagg_tab_p1 t2_3 (cost=0.00..18.00
rows=1200 width=2)
(15 rows)
postgres=# select count(*) from pagg_tab t1 join pagg_tab t2 on t1.c =
t2.c;
count
---------
1800000
(1 row)
I gave a try that use partkey->partcollation[cnt] then the query result is
correct when enable_partitionwise_join is on.
And I found some codes only use equal not check the collation, for example,
in match_expr_to_partition_keys():
if (equal(lfirst(lc), expr))
lfirst(lc) is partkeys.
I thinks other codes assume that the collation of partexprs in RelOptInfo
is same with pg_partitioned_table. But it is not.
--
Thanks,
Tender Wang
^ permalink raw reply [nested|flat] 30+ messages in thread
* Re: BUG #18568: BUG: Result wrong when do group by on partition table!
@ 2024-10-23 10:42 Tender Wang <tndrwang@gmail.com>
parent: Tender Wang <tndrwang@gmail.com>
0 siblings, 1 reply; 30+ messages in thread
From: Tender Wang @ 2024-10-23 10:42 UTC (permalink / raw)
To: Amit Langote <amitlangote09@gmail.com>; +Cc: 1105066510@qq.com; pgsql-bugs@lists.postgresql.org
Tender Wang <tndrwang@gmail.com> 于2024年10月23日周三 14:25写道：
>
>
> And I found some codes only use equal not check the collation, for
> example, in match_expr_to_partition_keys():
> if (equal(lfirst(lc), expr))
> lfirst(lc) is partkeys.
> I thinks other codes assume that the collation of partexprs in RelOptInfo
> is same with pg_partitioned_table. But it is not.
>
>
It seems that only partition pruning logic not only does equal() but also
checks the collation match. I find below codes in
match_clause_to_partition_key():
/*
* Partition key match also requires collation match. There may be
* multiple partkeys with the same expression but different
* collations, so failure is NOMATCH.
*/
if (!PartCollMatchesExprColl(partcoll, opclause->inputcollid))
return PARTCLAUSE_NOMATCH;
Others only do equal(), as far as I know.
I tried the patch I provided in [1], and the regression test cases all
passed.
[1]
https://www.postgresql.org/message-id/CAHewXNnKLrZYG4iqaYw%3DuB3XWRrYRZHo7VtcMsbUEbdbajQg2Q%40mail.g...
--
Thanks,
Tender Wang
^ permalink raw reply [nested|flat] 30+ messages in thread
* Re: BUG #18568: BUG: Result wrong when do group by on partition table!
@ 2024-10-23 14:18 jian he <jian.universality@gmail.com>
parent: Tender Wang <tndrwang@gmail.com>
0 siblings, 1 reply; 30+ messages in thread
From: jian he @ 2024-10-23 14:18 UTC (permalink / raw)
To: Tender Wang <tndrwang@gmail.com>; +Cc: Amit Langote <amitlangote09@gmail.com>; 1105066510@qq.com; pgsql-bugs@lists.postgresql.org
On Wed, Oct 23, 2024 at 6:43 PM Tender Wang <tndrwang@gmail.com> wrote:
>
> I tried the patch I provided in [1], and the regression test cases all passed.
>
////////////////////////
ComputePartitionAttrs code snippet
ELSE
{
/* Expression */
Node *expr = pelem->expr;
char partattname[16];
Assert(expr != NULL);
atttype = exprType(expr);
attcollation = exprCollation(expr);
}
/*
* Apply collation override if any
*/
if (pelem->collation)
attcollation = get_collation_oid(pelem->collation, false);
partcollation[attn] = attcollation;
////////////////////////
create table coll_pruning_multi (a text) partition by range (substr(a,
1) collate "POSIX", substr(a, 1) collate "C");
PartitionElem->expr only cover "substr(a,1)".
PartitionElem->collation is for explicitly COLLATION clauses.
you can also see
https://github.com/postgres/postgres/blob/master/src/backend/parser/gram.y#L4556
^ permalink raw reply [nested|flat] 30+ messages in thread
* Re: BUG #18568: BUG: Result wrong when do group by on partition table!
@ 2024-10-24 02:04 Tender Wang <tndrwang@gmail.com>
parent: jian he <jian.universality@gmail.com>
0 siblings, 1 reply; 30+ messages in thread
From: Tender Wang @ 2024-10-24 02:04 UTC (permalink / raw)
To: jian he <jian.universality@gmail.com>; +Cc: Amit Langote <amitlangote09@gmail.com>; 1105066510@qq.com; pgsql-bugs@lists.postgresql.org
jian he <jian.universality@gmail.com> 于2024年10月23日周三 22:18写道：
> On Wed, Oct 23, 2024 at 6:43 PM Tender Wang <tndrwang@gmail.com> wrote:
> >
> > I tried the patch I provided in [1], and the regression test cases all
> passed.
> >
>
> ////////////////////////
> ComputePartitionAttrs code snippet
> ELSE
> {
> /* Expression */
> Node *expr = pelem->expr;
> char partattname[16];
> Assert(expr != NULL);
> atttype = exprType(expr);
> attcollation = exprCollation(expr);
> }
> /*
> * Apply collation override if any
> */
> if (pelem->collation)
> attcollation = get_collation_oid(pelem->collation, false);
> partcollation[attn] = attcollation;
> ////////////////////////
>
> create table coll_pruning_multi (a text) partition by range (substr(a,
> 1) collate "POSIX", substr(a, 1) collate "C");
> PartitionElem->expr only cover "substr(a,1)".
> PartitionElem->collation is for explicitly COLLATION clauses.
> you can also see
>
> https://github.com/postgres/postgres/blob/master/src/backend/parser/gram.y#L4556
>
> From the above "collation override" comments, we can say
> exprCollation(PartitionElem->expr)
> does not always equal PartitionElem->collation
> PartitionElem->collation is the true collation OID.
>
> so you change in but didn't cover the ELSE branch.
>
> else
> {
> if (lc == NULL)
> elog(ERROR, "wrong number of partition key expressions");
> /* Re-stamp the expression with given varno. */
> partexpr = (Expr *) copyObject(lfirst(lc));
> ChangeVarNodes((Node *) partexpr, 1, varno, 0);
> lc = lnext(partkey->partexprs, lc);
> }
>
> as you mentioned partkey->partcollation is correct collation for
> PartitionKey.
> but the ELSE branch, we cannot do
> else
> {
> if (lc == NULL)
> elog(ERROR, "wrong number of partition key expressions");
> /* Re-stamp the expression with given varno. */
> partexpr = (Expr *) copyObject(lfirst(lc));
> ChangeVarNodes((Node *) partexpr, 1, varno, 0);
> exprSetCollation(Node *partexpr, Oid collation)
> lc = lnext(partkey->partexprs, lc);
> }
>
> because in struct inPartitionElem, collation and expr is seperated.
> that means after set_baserel_partition_key_exprs
> we still cannot be sure that RelOptInfo->partexprs have the correct
> PartitionKey collation information.
>
Yeah, you're right. I confirm this again. In
set_baserel_partition_key_exprs(),
we copy partkey->partexprs not including partcollation, if it is not simple
column reference.
So I think how we can fix this thread issue and the [1] I reported by me
using a uniform solution.
By the way, I re-started a new thread [2] to track the issue I found in
[1]. I will reply to an email reflecting what you said here and cc you.
[1]
https://www.postgresql.org/message-id/CAHewXNnyWUEmdHrRK3yg4k2TzSbb5WnkKLWxyO%2BOVZPhPFX7ew%40mail.g...
[2]
https://www.postgresql.org/message-id/CAHewXNno_HKiQ6PqyLYfuqDtwp7KKHZiH1J7Pqyz0nr%2BPS2Dwg%40mail.g...
--
Thanks,
Tender Wang
^ permalink raw reply [nested|flat] 30+ messages in thread
* Re: BUG #18568: BUG: Result wrong when do group by on partition table!
@ 2024-11-05 07:57 Amit Langote <amitlangote09@gmail.com>
parent: Tender Wang <tndrwang@gmail.com>
0 siblings, 1 reply; 30+ messages in thread
From: Amit Langote @ 2024-11-05 07:57 UTC (permalink / raw)
To: Tender Wang <tndrwang@gmail.com>; +Cc: jian he <jian.universality@gmail.com>; 1105066510@qq.com; pgsql-bugs@lists.postgresql.org
On Thu, Oct 24, 2024 at 11:04 AM Tender Wang <tndrwang@gmail.com> wrote:
> jian he <jian.universality@gmail.com> 于2024年10月23日周三 22:18写道：
>>
>> On Wed, Oct 23, 2024 at 6:43 PM Tender Wang <tndrwang@gmail.com> wrote:
>> >
>> > I tried the patch I provided in [1], and the regression test cases all passed.
>> >
>>
>> ////////////////////////
>> ComputePartitionAttrs code snippet
>> ELSE
>> {
>> /* Expression */
>> Node *expr = pelem->expr;
>> char partattname[16];
>> Assert(expr != NULL);
>> atttype = exprType(expr);
>> attcollation = exprCollation(expr);
>> }
>> /*
>> * Apply collation override if any
>> */
>> if (pelem->collation)
>> attcollation = get_collation_oid(pelem->collation, false);
>> partcollation[attn] = attcollation;
>> ////////////////////////
>>
>> create table coll_pruning_multi (a text) partition by range (substr(a,
>> 1) collate "POSIX", substr(a, 1) collate "C");
>> PartitionElem->expr only cover "substr(a,1)".
>> PartitionElem->collation is for explicitly COLLATION clauses.
>> you can also see
>> https://github.com/postgres/postgres/blob/master/src/backend/parser/gram.y#L4556
>>
>> From the above "collation override" comments, we can say
>> exprCollation(PartitionElem->expr)
>> does not always equal PartitionElem->collation
>> PartitionElem->collation is the true collation OID.
>>
>> so you change in but didn't cover the ELSE branch.
>>
>> else
>> {
>> if (lc == NULL)
>> elog(ERROR, "wrong number of partition key expressions");
>> /* Re-stamp the expression with given varno. */
>> partexpr = (Expr *) copyObject(lfirst(lc));
>> ChangeVarNodes((Node *) partexpr, 1, varno, 0);
>> lc = lnext(partkey->partexprs, lc);
>> }
>>
>> as you mentioned partkey->partcollation is correct collation for PartitionKey.
>> but the ELSE branch, we cannot do
>> else
>> {
>> if (lc == NULL)
>> elog(ERROR, "wrong number of partition key expressions");
>> /* Re-stamp the expression with given varno. */
>> partexpr = (Expr *) copyObject(lfirst(lc));
>> ChangeVarNodes((Node *) partexpr, 1, varno, 0);
>> exprSetCollation(Node *partexpr, Oid collation)
>> lc = lnext(partkey->partexprs, lc);
>> }
>>
>> because in struct inPartitionElem, collation and expr is seperated.
>> that means after set_baserel_partition_key_exprs
>> we still cannot be sure that RelOptInfo->partexprs have the correct
>> PartitionKey collation information.
>
>
> Yeah, you're right. I confirm this again. In set_baserel_partition_key_exprs(),
> we copy partkey->partexprs not including partcollation, if it is not simple column reference.
>
> So I think how we can fix this thread issue and the [1] I reported by me using a uniform solution.
> By the way, I re-started a new thread [2] to track the issue I found in [1]. I will reply to an email reflecting what you said here and cc you.
>
> [1] https://www.postgresql.org/message-id/CAHewXNnyWUEmdHrRK3yg4k2TzSbb5WnkKLWxyO%2BOVZPhPFX7ew%40mail.g...
>
> [2] https://www.postgresql.org/message-id/CAHewXNno_HKiQ6PqyLYfuqDtwp7KKHZiH1J7Pqyz0nr%2BPS2Dwg%40mail.g...
FTR, the patch to fix the bug reported here is being discussed at [2].
--
Thanks, Amit Langote
^ permalink raw reply [nested|flat] 30+ messages in thread
* Re: BUG #18568: BUG: Result wrong when do group by on partition table!
@ 2024-11-08 09:28 Amit Langote <amitlangote09@gmail.com>
parent: Amit Langote <amitlangote09@gmail.com>
0 siblings, 1 reply; 30+ messages in thread
From: Amit Langote @ 2024-11-08 09:28 UTC (permalink / raw)
To: Tender Wang <tndrwang@gmail.com>; +Cc: jian he <jian.universality@gmail.com>; 1105066510@qq.com; pgsql-bugs@lists.postgresql.org
On Tue, Nov 5, 2024 at 4:57 PM Amit Langote <amitlangote09@gmail.com> wrote:
> On Thu, Oct 24, 2024 at 11:04 AM Tender Wang <tndrwang@gmail.com> wrote:
> > jian he <jian.universality@gmail.com> 于2024年10月23日周三 22:18写道：
> >>
> >> On Wed, Oct 23, 2024 at 6:43 PM Tender Wang <tndrwang@gmail.com> wrote:
> >> >
> >> > I tried the patch I provided in [1], and the regression test cases all passed.
> >> >
> >>
> >> ////////////////////////
> >> ComputePartitionAttrs code snippet
> >> ELSE
> >> {
> >> /* Expression */
> >> Node *expr = pelem->expr;
> >> char partattname[16];
> >> Assert(expr != NULL);
> >> atttype = exprType(expr);
> >> attcollation = exprCollation(expr);
> >> }
> >> /*
> >> * Apply collation override if any
> >> */
> >> if (pelem->collation)
> >> attcollation = get_collation_oid(pelem->collation, false);
> >> partcollation[attn] = attcollation;
> >> ////////////////////////
> >>
> >> create table coll_pruning_multi (a text) partition by range (substr(a,
> >> 1) collate "POSIX", substr(a, 1) collate "C");
> >> PartitionElem->expr only cover "substr(a,1)".
> >> PartitionElem->collation is for explicitly COLLATION clauses.
> >> you can also see
> >> https://github.com/postgres/postgres/blob/master/src/backend/parser/gram.y#L4556
> >>
> >> From the above "collation override" comments, we can say
> >> exprCollation(PartitionElem->expr)
> >> does not always equal PartitionElem->collation
> >> PartitionElem->collation is the true collation OID.
> >>
> >> so you change in but didn't cover the ELSE branch.
> >>
> >> else
> >> {
> >> if (lc == NULL)
> >> elog(ERROR, "wrong number of partition key expressions");
> >> /* Re-stamp the expression with given varno. */
> >> partexpr = (Expr *) copyObject(lfirst(lc));
> >> ChangeVarNodes((Node *) partexpr, 1, varno, 0);
> >> lc = lnext(partkey->partexprs, lc);
> >> }
> >>
> >> as you mentioned partkey->partcollation is correct collation for PartitionKey.
> >> but the ELSE branch, we cannot do
> >> else
> >> {
> >> if (lc == NULL)
> >> elog(ERROR, "wrong number of partition key expressions");
> >> /* Re-stamp the expression with given varno. */
> >> partexpr = (Expr *) copyObject(lfirst(lc));
> >> ChangeVarNodes((Node *) partexpr, 1, varno, 0);
> >> exprSetCollation(Node *partexpr, Oid collation)
> >> lc = lnext(partkey->partexprs, lc);
> >> }
> >>
> >> because in struct inPartitionElem, collation and expr is seperated.
> >> that means after set_baserel_partition_key_exprs
> >> we still cannot be sure that RelOptInfo->partexprs have the correct
> >> PartitionKey collation information.
> >
> >
> > Yeah, you're right. I confirm this again. In set_baserel_partition_key_exprs(),
> > we copy partkey->partexprs not including partcollation, if it is not simple column reference.
> >
> > So I think how we can fix this thread issue and the [1] I reported by me using a uniform solution.
> > By the way, I re-started a new thread [2] to track the issue I found in [1]. I will reply to an email reflecting what you said here and cc you.
> >
> > [1] https://www.postgresql.org/message-id/CAHewXNnyWUEmdHrRK3yg4k2TzSbb5WnkKLWxyO%2BOVZPhPFX7ew%40mail.g...
> >
> > [2] https://www.postgresql.org/message-id/CAHewXNno_HKiQ6PqyLYfuqDtwp7KKHZiH1J7Pqyz0nr%2BPS2Dwg%40mail.g...
>
> FTR, the patch to fix the bug reported here is being discussed at [2].
I've pushed the fix for this issue to all the branches down to 12.
Thanks for the report and the analyses.
--
Thanks, Amit Langote
^ permalink raw reply [nested|flat] 30+ messages in thread
* Re: BUG #18568: BUG: Result wrong when do group by on partition table!
@ 2024-11-08 09:36 Tender Wang <tndrwang@gmail.com>
parent: Amit Langote <amitlangote09@gmail.com>
0 siblings, 0 replies; 30+ messages in thread
From: Tender Wang @ 2024-11-08 09:36 UTC (permalink / raw)
To: Amit Langote <amitlangote09@gmail.com>; +Cc: jian he <jian.universality@gmail.com>; 1105066510@qq.com; pgsql-bugs@lists.postgresql.org
Amit Langote <amitlangote09@gmail.com> 于2024年11月8日周五 17:28写道：
> I've pushed the fix for this issue to all the branches down to 12.
>
> Thanks for the report and the analyses.
>
Thanks for pushing these collation-related fixed patches.
--
Thanks,
Tender Wang
^ permalink raw reply [nested|flat] 30+ messages in thread
end of thread, other threads:[~2024-11-08 09:36 UTC | newest]
Thread overview: 30+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2024-08-06 06:23 BUG #18568: BUG: Result wrong when do group by on partition table! PG Bug reporting form <noreply@postgresql.org>
2024-08-06 08:42 ` Tender Wang <tndrwang@gmail.com>
2024-08-06 09:09 ` Aleksander Alekseev <aleksander@timescale.com>
2024-08-06 09:27 ` Tender Wang <tndrwang@gmail.com>
2024-08-06 14:37 ` Tender Wang <tndrwang@gmail.com>
2024-08-07 04:01 ` Tender Wang <tndrwang@gmail.com>
2024-08-07 06:57 ` Tender Wang <tndrwang@gmail.com>
2024-08-07 09:23 ` Tender Wang <tndrwang@gmail.com>
2024-08-06 13:23 ` Tender Wang <tndrwang@gmail.com>
2024-10-22 07:33 ` Amit Langote <amitlangote09@gmail.com>
2024-10-22 07:46 ` Tender Wang <tndrwang@gmail.com>
2024-10-22 08:30 ` Tender Wang <tndrwang@gmail.com>
2024-10-22 09:25 ` Amit Langote <amitlangote09@gmail.com>
2024-10-22 11:14 ` Tender Wang <tndrwang@gmail.com>
2024-10-22 12:11 ` Amit Langote <amitlangote09@gmail.com>
2024-10-22 14:29 ` Tender Wang <tndrwang@gmail.com>
2024-10-23 06:25 ` Tender Wang <tndrwang@gmail.com>
2024-10-23 10:42 ` Tender Wang <tndrwang@gmail.com>
2024-10-23 14:18 ` jian he <jian.universality@gmail.com>
2024-10-24 02:04 ` Tender Wang <tndrwang@gmail.com>
2024-11-05 07:57 ` Amit Langote <amitlangote09@gmail.com>
2024-11-08 09:28 ` Amit Langote <amitlangote09@gmail.com>
2024-11-08 09:36 ` Tender Wang <tndrwang@gmail.com>
2024-08-07 15:04 Re: BUG #18568: BUG: Result wrong when do group by on partition table! jian he <jian.universality@gmail.com>
2024-08-08 07:51 ` jian he <jian.universality@gmail.com>
2024-08-08 08:20 ` Tender Wang <tndrwang@gmail.com>
2024-08-08 09:46 ` Tender Wang <tndrwang@gmail.com>
2024-10-21 08:10 ` jian he <jian.universality@gmail.com>
2024-10-22 12:28 ` Amit Langote <amitlangote09@gmail.com>
2024-10-23 01:12 ` Amit Langote <amitlangote09@gmail.com>
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox