public inbox for [email protected]
help / color / mirror / Atom feed[PATCH v22 6/8] Row pattern recognition patch (docs).
15+ messages / 5 participants
[nested] [flat]
* [PATCH v22 6/8] Row pattern recognition patch (docs).
@ 2024-09-19 04:48 Tatsuo Ishii <[email protected]>
0 siblings, 0 replies; 15+ messages in thread
From: Tatsuo Ishii @ 2024-09-19 04:48 UTC (permalink / raw)
---
doc/src/sgml/advanced.sgml | 82 ++++++++++++++++++++++++++++++++++++
doc/src/sgml/func.sgml | 54 ++++++++++++++++++++++++
doc/src/sgml/ref/select.sgml | 38 ++++++++++++++++-
3 files changed, 172 insertions(+), 2 deletions(-)
diff --git a/doc/src/sgml/advanced.sgml b/doc/src/sgml/advanced.sgml
index 755c9f1485..b0b1d1c51e 100644
--- a/doc/src/sgml/advanced.sgml
+++ b/doc/src/sgml/advanced.sgml
@@ -537,6 +537,88 @@ WHERE pos < 3;
<literal>rank</literal> less than 3.
</para>
+ <para>
+ Row pattern common syntax can be used to perform row pattern recognition
+ in a query. The row pattern common syntax includes two sub
+ clauses: <literal>DEFINE</literal>
+ and <literal>PATTERN</literal>. <literal>DEFINE</literal> defines
+ definition variables along with an expression. The expression must be a
+ logical expression, which means it must
+ return <literal>TRUE</literal>, <literal>FALSE</literal>
+ or <literal>NULL</literal>. The expression may comprise column references
+ and functions. Window functions, aggregate functions and subqueries are
+ not allowed. An example of <literal>DEFINE</literal> is as follows.
+
+<programlisting>
+DEFINE
+ LOWPRICE AS price <= 100,
+ UP AS price > PREV(price),
+ DOWN AS price < PREV(price)
+</programlisting>
+
+ Note that <function>PREV</function> returns the price column in the
+ previous row if it's called in a context of row pattern recognition. Thus in
+ the second line the definition variable "UP" is <literal>TRUE</literal>
+ when the price column in the current row is greater than the price column
+ in the previous row. Likewise, "DOWN" is <literal>TRUE</literal> when when
+ the price column in the current row is lower than the price column in the
+ previous row.
+ </para>
+ <para>
+ Once <literal>DEFINE</literal> exists, <literal>PATTERN</literal> can be
+ used. <literal>PATTERN</literal> defines a sequence of rows that satisfies
+ certain conditions. For example following <literal>PATTERN</literal>
+ defines that a row starts with the condition "LOWPRICE", then one or more
+ rows satisfy "UP" and finally one or more rows satisfy "DOWN". Note that
+ "+" means one or more matches. Also you can use "*", which means zero or
+ more matches. If a sequence of rows which satisfies the PATTERN is found,
+ in the starting row of the sequence of rows all window functions and
+ aggregates are shown in the target list. Note that aggregations only look
+ into the matched rows, rather than whole frame. On the second or
+ subsequent rows all window functions are NULL. Aggregates are NULL or 0
+ (count case) depending on its aggregation definition. For rows that do not
+ match on the PATTERN, all window functions and aggregates are shown AS
+ NULL too, except count showing 0. This is because the rows do not match,
+ thus they are in an empty frame. Example of a <literal>SELECT</literal>
+ using the <literal>DEFINE</literal> and <literal>PATTERN</literal> clause
+ is as follows.
+
+<programlisting>
+SELECT company, tdate, price,
+ first_value(price) OVER w,
+ max(price) OVER w,
+ count(price) OVER w
+FROM stock
+ WINDOW w AS (
+ PARTITION BY company
+ ORDER BY tdate
+ ROWS BETWEEN CURRENT ROW AND UNBOUNDED FOLLOWING
+ AFTER MATCH SKIP PAST LAST ROW
+ INITIAL
+ PATTERN (LOWPRICE UP+ DOWN+)
+ DEFINE
+ LOWPRICE AS price <= 100,
+ UP AS price > PREV(price),
+ DOWN AS price < PREV(price)
+);
+</programlisting>
+<screen>
+ company | tdate | price | first_value | max | count
+----------+------------+-------+-------------+-----+-------
+ company1 | 2023-07-01 | 100 | 100 | 200 | 4
+ company1 | 2023-07-02 | 200 | | | 0
+ company1 | 2023-07-03 | 150 | | | 0
+ company1 | 2023-07-04 | 140 | | | 0
+ company1 | 2023-07-05 | 150 | | | 0
+ company1 | 2023-07-06 | 90 | 90 | 130 | 4
+ company1 | 2023-07-07 | 110 | | | 0
+ company1 | 2023-07-08 | 130 | | | 0
+ company1 | 2023-07-09 | 120 | | | 0
+ company1 | 2023-07-10 | 130 | | | 0
+(10 rows)
+</screen>
+ </para>
+
<para>
When a query involves multiple window functions, it is possible to write
out each one with a separate <literal>OVER</literal> clause, but this is
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 6f75bd0c7d..4482c80a70 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -23261,6 +23261,7 @@ SELECT count(*) FROM sometable;
returns <literal>NULL</literal> if there is no such row.
</para></entry>
</row>
+
</tbody>
</tgroup>
</table>
@@ -23300,6 +23301,59 @@ SELECT count(*) FROM sometable;
Other frame specifications can be used to obtain other effects.
</para>
+ <para>
+ Row pattern recognition navigation functions are listed in
+ <xref linkend="functions-rpr-navigation-table"/>. These functions
+ can be used to describe DEFINE clause of Row pattern recognition.
+ </para>
+
+ <table id="functions-rpr-navigation-table">
+ <title>Row Pattern Navigation Functions</title>
+ <tgroup cols="1">
+ <thead>
+ <row>
+ <entry role="func_table_entry"><para role="func_signature">
+ Function
+ </para>
+ <para>
+ Description
+ </para></entry>
+ </row>
+ </thead>
+
+ <tbody>
+ <row>
+ <entry role="func_table_entry"><para role="func_signature">
+ <indexterm>
+ <primary>prev</primary>
+ </indexterm>
+ <function>prev</function> ( <parameter>value</parameter> <type>anyelement</type> )
+ <returnvalue>anyelement</returnvalue>
+ </para>
+ <para>
+ Returns the column value at the previous row;
+ returns NULL if there is no previous row in the window frame.
+ </para></entry>
+ </row>
+
+ <row>
+ <entry role="func_table_entry"><para role="func_signature">
+ <indexterm>
+ <primary>next</primary>
+ </indexterm>
+ <function>next</function> ( <parameter>value</parameter> <type>anyelement</type> )
+ <returnvalue>anyelement</returnvalue>
+ </para>
+ <para>
+ Returns the column value at the next row;
+ returns NULL if there is no next row in the window frame.
+ </para></entry>
+ </row>
+
+ </tbody>
+ </tgroup>
+ </table>
+
<note>
<para>
The SQL standard defines a <literal>RESPECT NULLS</literal> or
diff --git a/doc/src/sgml/ref/select.sgml b/doc/src/sgml/ref/select.sgml
index d7089eac0b..7e1c9989ba 100644
--- a/doc/src/sgml/ref/select.sgml
+++ b/doc/src/sgml/ref/select.sgml
@@ -969,8 +969,8 @@ WINDOW <replaceable class="parameter">window_name</replaceable> AS ( <replaceabl
The <replaceable class="parameter">frame_clause</replaceable> can be one of
<synopsis>
-{ RANGE | ROWS | GROUPS } <replaceable>frame_start</replaceable> [ <replaceable>frame_exclusion</replaceable> ]
-{ RANGE | ROWS | GROUPS } BETWEEN <replaceable>frame_start</replaceable> AND <replaceable>frame_end</replaceable> [ <replaceable>frame_exclusion</replaceable> ]
+{ RANGE | ROWS | GROUPS } <replaceable>frame_start</replaceable> [ <replaceable>frame_exclusion</replaceable> ] [row_pattern_common_syntax]
+{ RANGE | ROWS | GROUPS } BETWEEN <replaceable>frame_start</replaceable> AND <replaceable>frame_end</replaceable> [ <replaceable>frame_exclusion</replaceable> ] [row_pattern_common_syntax]
</synopsis>
where <replaceable>frame_start</replaceable>
@@ -1077,6 +1077,40 @@ EXCLUDE NO OTHERS
a given peer group will be in the frame or excluded from it.
</para>
+ <para>
+ The
+ optional <replaceable class="parameter">row_pattern_common_syntax</replaceable>
+ defines the <firstterm>row pattern recognition condition</firstterm> for
+ this
+ window. <replaceable class="parameter">row_pattern_common_syntax</replaceable>
+ includes following subclauses. <literal>AFTER MATCH SKIP PAST LAST
+ ROW</literal> or <literal>AFTER MATCH SKIP TO NEXT ROW</literal> controls
+ how to proceed to next row position after a match
+ found. With <literal>AFTER MATCH SKIP PAST LAST ROW</literal> (the
+ default) next row position is next to the last row of previous match. On
+ the other hand, with <literal>AFTER MATCH SKIP TO NEXT ROW</literal> next
+ row position is always next to the last row of previous
+ match. <literal>DEFINE</literal> defines definition variables along with a
+ boolean expression. <literal>PATTERN</literal> defines a sequence of rows
+ that satisfies certain conditions using variables defined
+ in <literal>DEFINE</literal> clause. If the variable is not defined in
+ the <literal>DEFINE</literal> clause, it is implicitly assumed
+ following is defined in the <literal>DEFINE</literal> clause.
+
+<synopsis>
+<literal>variable_name</literal> AS TRUE
+</synopsis>
+
+ Note that the maximu number of variables defined
+ in <literal>DEFINE</literal> clause is 26.
+
+<synopsis>
+[ AFTER MATCH SKIP PAST LAST ROW | AFTER MATCH SKIP TO NEXT ROW ]
+PATTERN <replaceable class="parameter">pattern_variable_name</replaceable>[+] [, ...]
+DEFINE <replaceable class="parameter">definition_varible_name</replaceable> AS <replaceable class="parameter">expression</replaceable> [, ...]
+</synopsis>
+ </para>
+
<para>
The purpose of a <literal>WINDOW</literal> clause is to specify the
behavior of <firstterm>window functions</firstterm> appearing in the query's
--
2.25.1
----Next_Part(Thu_Sep_19_13_59_47_2024_608)--
Content-Type: Text/X-Patch; charset=us-ascii
Content-Transfer-Encoding: 7bit
Content-Disposition: inline;
filename="v22-0007-Row-pattern-recognition-patch-tests.patch"
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: Wrong result when enable_partitionwise_join is on if collation of PartitionKey and Column is different.
@ 2024-10-30 03:57 jian he <[email protected]>
2024-10-30 12:36 ` Re: Wrong result when enable_partitionwise_join is on if collation of PartitionKey and Column is different. Junwang Zhao <[email protected]>
0 siblings, 1 reply; 15+ messages in thread
From: jian he @ 2024-10-30 03:57 UTC (permalink / raw)
To: Tender Wang <[email protected]>; +Cc: Amit Langote <[email protected]>; PostgreSQL Hackers <[email protected]>
I missed a case when column collation and partition key collation are
the same and indeterministic.
that should be fine for partition-wise join.
so v2 attached.
have_partkey_equi_join, match_expr_to_partition_keys didn't do any
collation related check.
propose v2 change disallow partitionwise join for case when
column collation is indeterministic *and* is differ from partition
key's collation.
the attached partition_wise_join_collation.sql is the test script.
you may use it to compare with the master behavior.
Attachments:
[application/sql] partition_wise_join_collation.sql (8.1K, ../../CACJufxF1bmHSc+L7Hg7QxnH9UDbyP+qMsK8Z2ZBU8kux6C=t3A@mail.gmail.com/2-partition_wise_join_collation.sql)
download
[text/x-patch] v2-0001-partition_wise_join_collation_check.diff (3.8K, ../../CACJufxF1bmHSc+L7Hg7QxnH9UDbyP+qMsK8Z2ZBU8kux6C=t3A@mail.gmail.com/3-v2-0001-partition_wise_join_collation_check.diff)
download | inline diff:
diff --git a/src/backend/optimizer/util/relnode.c b/src/backend/optimizer/util/relnode.c
index d7266e4cdb..428751b05f 100644
--- a/src/backend/optimizer/util/relnode.c
+++ b/src/backend/optimizer/util/relnode.c
@@ -74,7 +74,7 @@ static bool have_partkey_equi_join(PlannerInfo *root, RelOptInfo *joinrel,
RelOptInfo *rel1, RelOptInfo *rel2,
JoinType jointype, List *restrictlist);
static int match_expr_to_partition_keys(Expr *expr, RelOptInfo *rel,
- bool strict_op);
+ bool strict_op, bool *coll_incompatiable);
static void set_joinrel_partition_key_exprs(RelOptInfo *joinrel,
RelOptInfo *outer_rel, RelOptInfo *inner_rel,
JoinType jointype);
@@ -2104,6 +2104,7 @@ have_partkey_equi_join(PlannerInfo *root, RelOptInfo *joinrel,
Expr *expr1;
Expr *expr2;
bool strict_op;
+ bool coll_incompatiable = false;
int ipk1;
int ipk2;
@@ -2167,10 +2168,11 @@ have_partkey_equi_join(PlannerInfo *root, RelOptInfo *joinrel,
* Only clauses referencing the partition keys are useful for
* partitionwise join.
*/
- ipk1 = match_expr_to_partition_keys(expr1, rel1, strict_op);
+ ipk1 = match_expr_to_partition_keys(expr1, rel1, strict_op, &coll_incompatiable);
if (ipk1 < 0)
continue;
- ipk2 = match_expr_to_partition_keys(expr2, rel2, strict_op);
+
+ ipk2 = match_expr_to_partition_keys(expr2, rel2, strict_op, &coll_incompatiable);
if (ipk2 < 0)
continue;
@@ -2181,6 +2183,15 @@ have_partkey_equi_join(PlannerInfo *root, RelOptInfo *joinrel,
if (ipk1 != ipk2)
continue;
+ /*
+ * we generally assume parttion key and expr's collation are fine for
+ * partition-wise join. forgidden case is column collation is
+ * indeterministic and partition key's collation not same as column's.
+ * see match_expr_to_partition_keys also.
+ */
+ if (coll_incompatiable)
+ return false;
+
/* Ignore clause if we already proved these keys equal. */
if (pk_known_equal[ipk1])
continue;
@@ -2296,9 +2307,15 @@ have_partkey_equi_join(PlannerInfo *root, RelOptInfo *joinrel,
* strict_op must be true if the expression will be compared with the
* partition key using a strict operator. This allows us to consider
* nullable as well as nonnullable partition keys.
+ * if exprCollation(expr) is inderministic also not equal to partcollation,
+ * that means same value with different apperances can live in different
+ * partition, coll_incompatiable return set to true. In that case, we cannot do
+ * partition-wise join. we are OK with expression's collation same as partition
+ * key's even though they are indeterministic.
+ *
*/
static int
-match_expr_to_partition_keys(Expr *expr, RelOptInfo *rel, bool strict_op)
+match_expr_to_partition_keys(Expr *expr, RelOptInfo *rel, bool strict_op, bool *coll_incompatiable)
{
int cnt;
@@ -2315,11 +2332,22 @@ match_expr_to_partition_keys(Expr *expr, RelOptInfo *rel, bool strict_op)
{
ListCell *lc;
+ Oid partcoll = rel->part_scheme->partcollation[cnt];
+
/* We can always match to the non-nullable partition keys. */
foreach(lc, rel->partexprs[cnt])
{
if (equal(lfirst(lc), expr))
+ {
+ Oid colloid = exprCollation((Node *) expr);
+
+ if ((partcoll != colloid) &&
+ OidIsValid(colloid) &&
+ !get_collation_isdeterministic(colloid))
+ *coll_incompatiable = true;
+
return cnt;
+ }
}
if (!strict_op)
@@ -2335,7 +2363,15 @@ match_expr_to_partition_keys(Expr *expr, RelOptInfo *rel, bool strict_op)
foreach(lc, rel->nullable_partexprs[cnt])
{
if (equal(lfirst(lc), expr))
+ {
+ Oid colloid = exprCollation((Node *) expr);
+
+ if ((partcoll != colloid) &&
+ OidIsValid(colloid) &&
+ !get_collation_isdeterministic(colloid))
+ *coll_incompatiable = true;
return cnt;
+ }
}
}
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: Wrong result when enable_partitionwise_join is on if collation of PartitionKey and Column is different.
2024-10-30 03:57 Re: Wrong result when enable_partitionwise_join is on if collation of PartitionKey and Column is different. jian he <[email protected]>
@ 2024-10-30 12:36 ` Junwang Zhao <[email protected]>
2024-10-31 13:08 ` Re: Wrong result when enable_partitionwise_join is on if collation of PartitionKey and Column is different. Amit Langote <[email protected]>
0 siblings, 1 reply; 15+ messages in thread
From: Junwang Zhao @ 2024-10-30 12:36 UTC (permalink / raw)
To: jian he <[email protected]>; +Cc: Tender Wang <[email protected]>; Amit Langote <[email protected]>; PostgreSQL Hackers <[email protected]>
On Wed, Oct 30, 2024 at 11:58 AM jian he <[email protected]> wrote:
>
> I missed a case when column collation and partition key collation are
> the same and indeterministic.
> that should be fine for partition-wise join.
> so v2 attached.
>
> have_partkey_equi_join, match_expr_to_partition_keys didn't do any
> collation related check.
> propose v2 change disallow partitionwise join for case when
> column collation is indeterministic *and* is differ from partition
> key's collation.
>
> the attached partition_wise_join_collation.sql is the test script.
> you may use it to compare with the master behavior.
What if the partkey collation and column collation are both deterministic,
but with different sort order?
I'm not familiar with this part of the code base, but it seems to me the
partition wise join should use partkey collation instead of column collation,
because it's the partkey collation that decides which partition a row to
be dispatched.
What Jian proposed is also reasonable but seems another aspect of $subject?
Just some random thought, might be wrong ;(
--
Regards
Junwang Zhao
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: Wrong result when enable_partitionwise_join is on if collation of PartitionKey and Column is different.
2024-10-30 03:57 Re: Wrong result when enable_partitionwise_join is on if collation of PartitionKey and Column is different. jian he <[email protected]>
2024-10-30 12:36 ` Re: Wrong result when enable_partitionwise_join is on if collation of PartitionKey and Column is different. Junwang Zhao <[email protected]>
@ 2024-10-31 13:08 ` Amit Langote <[email protected]>
2024-11-01 02:39 ` Re: Wrong result when enable_partitionwise_join is on if collation of PartitionKey and Column is different. Tender Wang <[email protected]>
2024-11-01 02:59 ` Re: Wrong result when enable_partitionwise_join is on if collation of PartitionKey and Column is different. Tender Wang <[email protected]>
2024-11-01 05:39 ` Re: Wrong result when enable_partitionwise_join is on if collation of PartitionKey and Column is different. jian he <[email protected]>
0 siblings, 3 replies; 15+ messages in thread
From: Amit Langote @ 2024-10-31 13:08 UTC (permalink / raw)
To: Junwang Zhao <[email protected]>; +Cc: jian he <[email protected]>; Tender Wang <[email protected]>; PostgreSQL Hackers <[email protected]>
On Wed, Oct 30, 2024 at 9:36 PM Junwang Zhao <[email protected]> wrote:
> On Wed, Oct 30, 2024 at 11:58 AM jian he <[email protected]> wrote:
> >
> > I missed a case when column collation and partition key collation are
> > the same and indeterministic.
> > that should be fine for partition-wise join.
> > so v2 attached.
> >
> > have_partkey_equi_join, match_expr_to_partition_keys didn't do any
> > collation related check.
> > propose v2 change disallow partitionwise join for case when
> > column collation is indeterministic *and* is differ from partition
> > key's collation.
> >
> > the attached partition_wise_join_collation.sql is the test script.
> > you may use it to compare with the master behavior.
>
> What if the partkey collation and column collation are both deterministic,
> but with different sort order?
>
> I'm not familiar with this part of the code base, but it seems to me the
> partition wise join should use partkey collation instead of column collation,
> because it's the partkey collation that decides which partition a row to
> be dispatched.
>
> What Jian proposed is also reasonable but seems another aspect of $subject?
I think we should insist that the join key collation and the partition
collation are exactly the same and refuse to match them if they are
not.
+ {
+ Oid colloid = exprCollation((Node *) expr);
+
+ if ((partcoll != colloid) &&
+ OidIsValid(colloid) &&
+ !get_collation_isdeterministic(colloid))
+ *coll_incompatiable = true;
I am not quite sure what is the point of checking whether or not the
expression collation is deterministic after confirming that it's not
the same as partcoll.
Attached 0002 is what I came up with. One thing that's different from
what Jian proposed is that match_expr_to_partition_keys() returns -1
(expr not matched to any key) when the collation is also not matched
instead of using a separate output parameter for that.
0001 is the patch for the partitionwise grouping issue (bug #18568).
I concluded that even partial aggregate should be disallowed when the
grouping collation doesn't match partitioning collation. The result
with partial partitionwise grouping is not the same as when
enable_partitionwise_aggregate is off.
--
Thanks, Amit Langote
Attachments:
[application/octet-stream] v3-0001-Disallow-partitionwise-grouping-when-collation-do.patch (11.7K, ../../CA+HiwqEHx++qTjCGmWyYZDiPR=_fLGPjR7RcFEVnSNZVc-Qj2g@mail.gmail.com/2-v3-0001-Disallow-partitionwise-grouping-when-collation-do.patch)
download | inline diff:
From 03cb00c55fef0f194c3c585f43f004d0a74542f5 Mon Sep 17 00:00:00 2001
From: Amit Langote <[email protected]>
Date: Thu, 31 Oct 2024 21:59:28 +0900
Subject: [PATCH v3 1/2] Disallow partitionwise grouping when collation doesn't
match
Insist that the collation used for grouping matches exactly with the
collation used for partitioning to allow using either full or
partial partitionwise grouping.
Bug: #18568
Reported-by: Webbo Han <[email protected]>
Author: Webbo Han <[email protected]>
Reviewed-by: Tender Wang <[email protected]>
Reviewed-by: Aleksander Alekseev <[email protected]>
Reviewed-by: Jian He <[email protected]>
Discussion: https://postgr.es/m/[email protected]
Discussion: https://postgr.es/m/[email protected]
Discussion: https://postgr.es/m/CAHewXNno_HKiQ6PqyLYfuqDtwp7KKHZiH1J7Pqyz0nr+PS2Dwg@mail.gmail.com
---
src/backend/optimizer/plan/planner.c | 53 +++++++++---
.../regress/expected/collate.icu.utf8.out | 85 +++++++++++++++++++
.../regress/expected/partition_aggregate.out | 11 ---
src/test/regress/sql/collate.icu.utf8.sql | 35 ++++++++
src/test/regress/sql/partition_aggregate.sql | 1 -
5 files changed, 163 insertions(+), 22 deletions(-)
diff --git a/src/backend/optimizer/plan/planner.c b/src/backend/optimizer/plan/planner.c
index 0f423e9684..60bb86a99c 100644
--- a/src/backend/optimizer/plan/planner.c
+++ b/src/backend/optimizer/plan/planner.c
@@ -253,7 +253,8 @@ static void create_partitionwise_grouping_paths(PlannerInfo *root,
GroupPathExtraData *extra);
static bool group_by_has_partkey(RelOptInfo *input_rel,
List *targetList,
- List *groupClause);
+ List *groupClause,
+ bool *collation_incompatible);
static int common_prefix_cmp(const void *a, const void *b);
static List *generate_setop_child_grouplist(SetOperationStmt *op,
List *targetlist);
@@ -4090,6 +4091,12 @@ create_ordinary_grouping_paths(PlannerInfo *root, RelOptInfo *input_rel,
if (extra->patype != PARTITIONWISE_AGGREGATE_NONE &&
IS_PARTITIONED_REL(input_rel))
{
+ bool collation_incompatible = false;
+ bool group_by_contains_partkey =
+ group_by_has_partkey(input_rel, extra->targetList,
+ root->parse->groupClause,
+ &collation_incompatible);
+
/*
* If this is the topmost relation or if the parent relation is doing
* full partitionwise aggregation, then we can do full partitionwise
@@ -4103,10 +4110,10 @@ create_ordinary_grouping_paths(PlannerInfo *root, RelOptInfo *input_rel,
* okay if some of the partitioning columns were proved redundant.
*/
if (extra->patype == PARTITIONWISE_AGGREGATE_FULL &&
- group_by_has_partkey(input_rel, extra->targetList,
- root->parse->groupClause))
+ group_by_contains_partkey)
patype = PARTITIONWISE_AGGREGATE_FULL;
- else if ((extra->flags & GROUPING_CAN_PARTIAL_AGG) != 0)
+ else if ((extra->flags & GROUPING_CAN_PARTIAL_AGG) != 0 &&
+ !collation_incompatible)
patype = PARTITIONWISE_AGGREGATE_PARTIAL;
else
patype = PARTITIONWISE_AGGREGATE_NONE;
@@ -8105,13 +8112,14 @@ create_partitionwise_grouping_paths(PlannerInfo *root,
/*
* group_by_has_partkey
*
- * Returns true, if all the partition keys of the given relation are part of
- * the GROUP BY clauses, false otherwise.
+ * Returns true if all the partition keys of the given relation are part of
+ * the GROUP BY clauses, including having matching collation, false otherwise.
*/
static bool
group_by_has_partkey(RelOptInfo *input_rel,
List *targetList,
- List *groupClause)
+ List *groupClause,
+ bool *collation_incompatible)
{
List *groupexprs = get_sortgrouplist_exprs(groupClause, targetList);
int cnt = 0;
@@ -8134,13 +8142,38 @@ 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 groupcoll = exprCollation((Node *) groupexpr);
+
+ if (IsA(groupexpr, RelabelType))
+ groupexpr = ((RelabelType *) groupexpr)->arg;
+
+ if (OidIsValid(partcoll) && OidIsValid(groupcoll) &&
+ partcoll != groupcoll)
+ {
+ *collation_incompatible = true;
+ return false;
+ }
+
+ /*
+ * Ensure that the grouping collation is compatible with the
+ * partitioning collation.
+ */
+ if (equal(groupexpr, partexpr))
+ {
+ found = true;
+ break;
+ }
}
+
+ if (found)
+ break;
}
/*
diff --git a/src/test/regress/expected/collate.icu.utf8.out b/src/test/regress/expected/collate.icu.utf8.out
index faa376e060..3d6d8f9a20 100644
--- a/src/test/regress/expected/collate.icu.utf8.out
+++ b/src/test/regress/expected/collate.icu.utf8.out
@@ -2054,6 +2054,91 @@ SELECT (SELECT count(*) FROM test33_0) <> (SELECT count(*) FROM test33_1);
t
(1 row)
+--
+-- Bug #18568
+--
+-- Partitionwise aggregate (full or partial) should not be used when a
+-- partition key's collation doesn't match that of the GROUP BY column it is
+-- matched with.
+SET max_parallel_workers_per_gather TO 0;
+SET enable_incremental_sort TO off;
+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');
+CREATE TABLE pagg_tab3_p2 PARTITION OF pagg_tab3 FOR VALUES IN ('B', 'A');
+INSERT INTO pagg_tab3 SELECT substr('abAB', (i % 4) +1 , 1) FROM generate_series(0, 7) i;
+ANALYZE pagg_tab3;
+SET enable_partitionwise_aggregate TO false;
+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
+ -> HashAggregate
+ Group Key: pagg_tab3.c
+ -> Append
+ -> Seq Scan on pagg_tab3_p2 pagg_tab3_1
+ -> Seq Scan on pagg_tab3_p1 pagg_tab3_2
+(7 rows)
+
+SELECT upper(c collate case_insensitive), count(c) FROM pagg_tab3 GROUP BY c collate case_insensitive ORDER BY 1;
+ upper | count
+-------+-------
+ A | 4
+ B | 4
+(2 rows)
+
+-- No partitionwise aggregation allowed!
+SET enable_partitionwise_aggregate TO true;
+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
+ -> HashAggregate
+ Group Key: pagg_tab3.c
+ -> Append
+ -> Seq Scan on pagg_tab3_p2 pagg_tab3_1
+ -> Seq Scan on pagg_tab3_p1 pagg_tab3_2
+(7 rows)
+
+SELECT upper(c collate case_insensitive), count(c) FROM pagg_tab3 GROUP BY c collate case_insensitive ORDER BY 1;
+ upper | count
+-------+-------
+ A | 4
+ B | 4
+(2 rows)
+
+-- OK to use full partitionwise aggregate after changing the GROUP BY column's
+-- collation to be the same as that of the partition key.
+EXPLAIN (COSTS OFF)
+SELECT c collate "C", count(c) FROM pagg_tab3 GROUP BY c collate "C" ORDER BY 1;
+ QUERY PLAN
+--------------------------------------------------------
+ Sort
+ Sort Key: ((pagg_tab3.c)::text) COLLATE "C"
+ -> Append
+ -> HashAggregate
+ Group Key: (pagg_tab3.c)::text
+ -> Seq Scan on pagg_tab3_p2 pagg_tab3
+ -> HashAggregate
+ Group Key: (pagg_tab3_1.c)::text
+ -> Seq Scan on pagg_tab3_p1 pagg_tab3_1
+(9 rows)
+
+SELECT c collate "C", count(c) FROM pagg_tab3 GROUP BY c collate "C" ORDER BY 1;
+ c | count
+---+-------
+ A | 2
+ B | 2
+ a | 2
+ b | 2
+(4 rows)
+
+RESET enable_partitionwise_aggregate;
+RESET max_parallel_workers_per_gather;
+RESET enable_incremental_sort;
-- cleanup
RESET search_path;
SET client_min_messages TO warning;
diff --git a/src/test/regress/expected/partition_aggregate.out b/src/test/regress/expected/partition_aggregate.out
index 5f2c0cf578..670eb98906 100644
--- a/src/test/regress/expected/partition_aggregate.out
+++ b/src/test/regress/expected/partition_aggregate.out
@@ -1507,14 +1507,3 @@ SELECT x, sum(y), avg(y), count(*) FROM pagg_tab_para GROUP BY x HAVING avg(y) <
-> Seq Scan on pagg_tab_para_p3 pagg_tab_para_2
(15 rows)
-SELECT x, sum(y), avg(y), count(*) FROM pagg_tab_para GROUP BY x HAVING avg(y) < 7 ORDER BY 1, 2, 3;
- x | sum | avg | count
-----+------+--------------------+-------
- 0 | 5000 | 5.0000000000000000 | 1000
- 1 | 6000 | 6.0000000000000000 | 1000
- 10 | 5000 | 5.0000000000000000 | 1000
- 11 | 6000 | 6.0000000000000000 | 1000
- 20 | 5000 | 5.0000000000000000 | 1000
- 21 | 6000 | 6.0000000000000000 | 1000
-(6 rows)
-
diff --git a/src/test/regress/sql/collate.icu.utf8.sql b/src/test/regress/sql/collate.icu.utf8.sql
index 80f28a97d7..eaf9a99be7 100644
--- a/src/test/regress/sql/collate.icu.utf8.sql
+++ b/src/test/regress/sql/collate.icu.utf8.sql
@@ -796,6 +796,41 @@ INSERT INTO test33 VALUES (2, 'DEF');
-- they end up in the same partition (but it's platform-dependent which one)
SELECT (SELECT count(*) FROM test33_0) <> (SELECT count(*) FROM test33_1);
+--
+-- Bug #18568
+--
+-- Partitionwise aggregate (full or partial) should not be used when a
+-- partition key's collation doesn't match that of the GROUP BY column it is
+-- matched with.
+SET max_parallel_workers_per_gather TO 0;
+SET enable_incremental_sort TO off;
+
+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');
+CREATE TABLE pagg_tab3_p2 PARTITION OF pagg_tab3 FOR VALUES IN ('B', 'A');
+INSERT INTO pagg_tab3 SELECT substr('abAB', (i % 4) +1 , 1) FROM generate_series(0, 7) i;
+ANALYZE pagg_tab3;
+
+SET enable_partitionwise_aggregate TO false;
+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;
+
+-- No partitionwise aggregation allowed!
+SET enable_partitionwise_aggregate TO true;
+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;
+
+-- OK to use full partitionwise aggregate after changing the GROUP BY column's
+-- collation to be the same as that of the partition key.
+EXPLAIN (COSTS OFF)
+SELECT c collate "C", count(c) FROM pagg_tab3 GROUP BY c collate "C" ORDER BY 1;
+SELECT c collate "C", count(c) FROM pagg_tab3 GROUP BY c collate "C" ORDER BY 1;
+
+RESET enable_partitionwise_aggregate;
+RESET max_parallel_workers_per_gather;
+RESET enable_incremental_sort;
-- cleanup
RESET search_path;
diff --git a/src/test/regress/sql/partition_aggregate.sql b/src/test/regress/sql/partition_aggregate.sql
index ab070fee24..1e263c1caf 100644
--- a/src/test/regress/sql/partition_aggregate.sql
+++ b/src/test/regress/sql/partition_aggregate.sql
@@ -333,4 +333,3 @@ 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;
--
2.43.0
[application/octet-stream] v3-0002-Disallow-partitionwise-join-when-collation-doesn-.patch (8.4K, ../../CA+HiwqEHx++qTjCGmWyYZDiPR=_fLGPjR7RcFEVnSNZVc-Qj2g@mail.gmail.com/3-v3-0002-Disallow-partitionwise-join-when-collation-doesn-.patch)
download | inline diff:
From ccf9f6cfe2a8078961ce2250ec56884ac12b6e01 Mon Sep 17 00:00:00 2001
From: Amit Langote <[email protected]>
Date: Thu, 31 Oct 2024 21:58:30 +0900
Subject: [PATCH v3 2/2] Disallow partitionwise join when collation doesn't
match
Insist that the collation used for joining matches exactly with the
collation used for partitioning to allow using partitionwise join.
Reported-by: Tender Wang <[email protected]>
Author: Jian He <[email protected]>
Author: Amit Langote <[email protected]>
Reviewed-by: Tender Wang <[email protected]>
Reviewed-by: Junwang Zhao <[email protected]>
Discussion: https://postgr.es/m/[email protected]
Discussion: https://postgr.es/m/[email protected]
Discussion: https://postgr.es/m/CAHewXNno_HKiQ6PqyLYfuqDtwp7KKHZiH1J7Pqyz0nr+PS2Dwg@mail.gmail.com
---
src/backend/optimizer/util/relnode.c | 26 +++++-
.../regress/expected/collate.icu.utf8.out | 89 +++++++++++++++++++
src/test/regress/sql/collate.icu.utf8.sql | 17 ++++
3 files changed, 130 insertions(+), 2 deletions(-)
diff --git a/src/backend/optimizer/util/relnode.c b/src/backend/optimizer/util/relnode.c
index d7266e4cdb..0dfcd3a556 100644
--- a/src/backend/optimizer/util/relnode.c
+++ b/src/backend/optimizer/util/relnode.c
@@ -2258,6 +2258,8 @@ have_partkey_equi_join(PlannerInfo *root, RelOptInfo *joinrel,
{
Node *expr1 = (Node *) lfirst(lc);
ListCell *lc2;
+ Oid partcoll1 = rel1->part_scheme->partcollation[ipk];
+ Oid exprcoll1 = exprCollation(expr1);
foreach(lc2, rel2->partexprs[ipk])
{
@@ -2265,8 +2267,18 @@ have_partkey_equi_join(PlannerInfo *root, RelOptInfo *joinrel,
if (exprs_known_equal(root, expr1, expr2, btree_opfamily))
{
- pk_known_equal[ipk] = true;
- break;
+ Oid partcoll2 = rel1->part_scheme->partcollation[ipk];
+ Oid exprcoll2 = exprCollation(expr2);
+
+ /*
+ * Ensure that the collations match those of the partition
+ * keys.
+ */
+ if (partcoll1 == exprcoll1 && partcoll2 == exprcoll2)
+ {
+ pk_known_equal[ipk] = true;
+ break;
+ }
}
}
if (pk_known_equal[ipk])
@@ -2301,6 +2313,7 @@ static int
match_expr_to_partition_keys(Expr *expr, RelOptInfo *rel, bool strict_op)
{
int cnt;
+ Oid exprcoll = exprCollation((Node *) expr);
/* This function should be called only for partitioned relations. */
Assert(rel->part_scheme);
@@ -2314,10 +2327,15 @@ match_expr_to_partition_keys(Expr *expr, RelOptInfo *rel, bool strict_op)
for (cnt = 0; cnt < rel->part_scheme->partnatts; cnt++)
{
ListCell *lc;
+ Oid partcoll = rel->part_scheme->partcollation[cnt];
/* We can always match to the non-nullable partition keys. */
foreach(lc, rel->partexprs[cnt])
{
+ if (OidIsValid(partcoll) && OidIsValid(exprcoll) &&
+ partcoll != exprcoll)
+ return -1;
+
if (equal(lfirst(lc), expr))
return cnt;
}
@@ -2334,6 +2352,10 @@ match_expr_to_partition_keys(Expr *expr, RelOptInfo *rel, bool strict_op)
*/
foreach(lc, rel->nullable_partexprs[cnt])
{
+ if (OidIsValid(partcoll) && OidIsValid(exprcoll) &&
+ partcoll != exprcoll)
+ return -1;
+
if (equal(lfirst(lc), expr))
return cnt;
}
diff --git a/src/test/regress/expected/collate.icu.utf8.out b/src/test/regress/expected/collate.icu.utf8.out
index 3d6d8f9a20..56239737cd 100644
--- a/src/test/regress/expected/collate.icu.utf8.out
+++ b/src/test/regress/expected/collate.icu.utf8.out
@@ -2136,6 +2136,95 @@ SELECT c collate "C", count(c) FROM pagg_tab3 GROUP BY c collate "C" ORDER BY 1;
b | 2
(4 rows)
+-- Partitionwise join should not be allowed too when the collation used by the
+-- join keys doesn't match the partition key
+SET enable_partitionwise_join TO false;
+EXPLAIN (COSTS OFF)
+SELECT t1.c, count(t2.c) FROM pagg_tab3 t1 JOIN pagg_tab3 t2 ON t1.c = t2.c GROUP BY 1 ORDER BY 1;
+ QUERY PLAN
+-------------------------------------------------------------
+ Sort
+ Sort Key: t1.c COLLATE case_insensitive
+ -> HashAggregate
+ Group Key: t1.c
+ -> Hash Join
+ Hash Cond: (t1.c = t2.c)
+ -> Append
+ -> Seq Scan on pagg_tab3_p2 t1_1
+ -> Seq Scan on pagg_tab3_p1 t1_2
+ -> Hash
+ -> Append
+ -> Seq Scan on pagg_tab3_p2 t2_1
+ -> Seq Scan on pagg_tab3_p1 t2_2
+(13 rows)
+
+SELECT t1.c, count(t2.c) FROM pagg_tab3 t1 JOIN pagg_tab3 t2 ON t1.c = t2.c GROUP BY 1 ORDER BY 1;
+ c | count
+---+-------
+ A | 16
+ B | 16
+(2 rows)
+
+SET enable_partitionwise_join TO true;
+EXPLAIN (COSTS OFF)
+SELECT t1.c, count(t2.c) FROM pagg_tab3 t1 JOIN pagg_tab3 t2 ON t1.c = t2.c GROUP BY 1 ORDER BY 1;
+ QUERY PLAN
+-------------------------------------------------------------
+ Sort
+ Sort Key: t1.c COLLATE case_insensitive
+ -> HashAggregate
+ Group Key: t1.c
+ -> Hash Join
+ Hash Cond: (t1.c = t2.c)
+ -> Append
+ -> Seq Scan on pagg_tab3_p2 t1_1
+ -> Seq Scan on pagg_tab3_p1 t1_2
+ -> Hash
+ -> Append
+ -> Seq Scan on pagg_tab3_p2 t2_1
+ -> Seq Scan on pagg_tab3_p1 t2_2
+(13 rows)
+
+SELECT t1.c, count(t2.c) FROM pagg_tab3 t1 JOIN pagg_tab3 t2 ON t1.c = t2.c GROUP BY 1 ORDER BY 1;
+ c | count
+---+-------
+ A | 16
+ B | 16
+(2 rows)
+
+-- OK when the join key uses the same collation.
+EXPLAIN (COSTS OFF)
+SELECT t1.c COLLATE "C", count(t2.c) FROM pagg_tab3 t1 JOIN pagg_tab3 t2 ON t1.c = t2.c COLLATE "C" GROUP BY t1.c COLLATE "C" ORDER BY 1;
+ QUERY PLAN
+------------------------------------------------------------------
+ Sort
+ Sort Key: ((t1.c)::text) COLLATE "C"
+ -> Append
+ -> HashAggregate
+ Group Key: (t1.c)::text
+ -> Hash Join
+ Hash Cond: ((t1.c)::text = (t2.c)::text)
+ -> Seq Scan on pagg_tab3_p2 t1
+ -> Hash
+ -> Seq Scan on pagg_tab3_p2 t2
+ -> HashAggregate
+ Group Key: (t1_1.c)::text
+ -> Hash Join
+ Hash Cond: ((t1_1.c)::text = (t2_1.c)::text)
+ -> Seq Scan on pagg_tab3_p1 t1_1
+ -> Hash
+ -> Seq Scan on pagg_tab3_p1 t2_1
+(17 rows)
+
+SELECT t1.c COLLATE "C", count(t2.c) FROM pagg_tab3 t1 JOIN pagg_tab3 t2 ON t1.c = t2.c COLLATE "C" GROUP BY t1.c COLLATE "C" ORDER BY 1;
+ c | count
+---+-------
+ A | 4
+ B | 4
+ a | 4
+ b | 4
+(4 rows)
+
RESET enable_partitionwise_aggregate;
RESET max_parallel_workers_per_gather;
RESET enable_incremental_sort;
diff --git a/src/test/regress/sql/collate.icu.utf8.sql b/src/test/regress/sql/collate.icu.utf8.sql
index eaf9a99be7..b5f872742b 100644
--- a/src/test/regress/sql/collate.icu.utf8.sql
+++ b/src/test/regress/sql/collate.icu.utf8.sql
@@ -828,6 +828,23 @@ EXPLAIN (COSTS OFF)
SELECT c collate "C", count(c) FROM pagg_tab3 GROUP BY c collate "C" ORDER BY 1;
SELECT c collate "C", count(c) FROM pagg_tab3 GROUP BY c collate "C" ORDER BY 1;
+-- Partitionwise join should not be allowed too when the collation used by the
+-- join keys doesn't match the partition key
+SET enable_partitionwise_join TO false;
+EXPLAIN (COSTS OFF)
+SELECT t1.c, count(t2.c) FROM pagg_tab3 t1 JOIN pagg_tab3 t2 ON t1.c = t2.c GROUP BY 1 ORDER BY 1;
+SELECT t1.c, count(t2.c) FROM pagg_tab3 t1 JOIN pagg_tab3 t2 ON t1.c = t2.c GROUP BY 1 ORDER BY 1;
+
+SET enable_partitionwise_join TO true;
+EXPLAIN (COSTS OFF)
+SELECT t1.c, count(t2.c) FROM pagg_tab3 t1 JOIN pagg_tab3 t2 ON t1.c = t2.c GROUP BY 1 ORDER BY 1;
+SELECT t1.c, count(t2.c) FROM pagg_tab3 t1 JOIN pagg_tab3 t2 ON t1.c = t2.c GROUP BY 1 ORDER BY 1;
+
+-- OK when the join key uses the same collation.
+EXPLAIN (COSTS OFF)
+SELECT t1.c COLLATE "C", count(t2.c) FROM pagg_tab3 t1 JOIN pagg_tab3 t2 ON t1.c = t2.c COLLATE "C" GROUP BY t1.c COLLATE "C" ORDER BY 1;
+SELECT t1.c COLLATE "C", count(t2.c) FROM pagg_tab3 t1 JOIN pagg_tab3 t2 ON t1.c = t2.c COLLATE "C" GROUP BY t1.c COLLATE "C" ORDER BY 1;
+
RESET enable_partitionwise_aggregate;
RESET max_parallel_workers_per_gather;
RESET enable_incremental_sort;
--
2.43.0
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: Wrong result when enable_partitionwise_join is on if collation of PartitionKey and Column is different.
2024-10-30 03:57 Re: Wrong result when enable_partitionwise_join is on if collation of PartitionKey and Column is different. jian he <[email protected]>
2024-10-30 12:36 ` Re: Wrong result when enable_partitionwise_join is on if collation of PartitionKey and Column is different. Junwang Zhao <[email protected]>
2024-10-31 13:08 ` Re: Wrong result when enable_partitionwise_join is on if collation of PartitionKey and Column is different. Amit Langote <[email protected]>
@ 2024-11-01 02:39 ` Tender Wang <[email protected]>
2024-11-01 06:55 ` Re: Wrong result when enable_partitionwise_join is on if collation of PartitionKey and Column is different. Amit Langote <[email protected]>
2 siblings, 1 reply; 15+ messages in thread
From: Tender Wang @ 2024-11-01 02:39 UTC (permalink / raw)
To: Amit Langote <[email protected]>; +Cc: Junwang Zhao <[email protected]>; jian he <[email protected]>; PostgreSQL Hackers <[email protected]>
Amit Langote <[email protected]> 于2024年10月31日周四 21:09写道：
> On Wed, Oct 30, 2024 at 9:36 PM Junwang Zhao <[email protected]> wrote:
> > On Wed, Oct 30, 2024 at 11:58 AM jian he <[email protected]>
> wrote:
> > >
> > > I missed a case when column collation and partition key collation are
> > > the same and indeterministic.
> > > that should be fine for partition-wise join.
> > > so v2 attached.
> > >
> > > have_partkey_equi_join, match_expr_to_partition_keys didn't do any
> > > collation related check.
> > > propose v2 change disallow partitionwise join for case when
> > > column collation is indeterministic *and* is differ from partition
> > > key's collation.
> > >
> > > the attached partition_wise_join_collation.sql is the test script.
> > > you may use it to compare with the master behavior.
> >
> > What if the partkey collation and column collation are both
> deterministic,
> > but with different sort order?
> >
> > I'm not familiar with this part of the code base, but it seems to me the
> > partition wise join should use partkey collation instead of column
> collation,
> > because it's the partkey collation that decides which partition a row to
> > be dispatched.
> >
> > What Jian proposed is also reasonable but seems another aspect of
> $subject?
>
> I think we should insist that the join key collation and the partition
> collation are exactly the same and refuse to match them if they are
> not.
>
> + {
> + Oid colloid = exprCollation((Node *) expr);
> +
> + if ((partcoll != colloid) &&
> + OidIsValid(colloid) &&
> + !get_collation_isdeterministic(colloid))
> + *coll_incompatiable = true;
>
> I am not quite sure what is the point of checking whether or not the
> expression collation is deterministic after confirming that it's not
> the same as partcoll.
>
Me, too.
>
> Attached 0002 is what I came up with. One thing that's different from
> what Jian proposed is that match_expr_to_partition_keys() returns -1
>
Agree.
(expr not matched to any key) when the collation is also not matched
> instead of using a separate output parameter for that.
>
In have_partkey_equi_join()
...
if (exprs_known_equal(root, expr1, expr2, btree_opfamily))
{
Oid partcoll2 = rel1->part_scheme->partcollation[ipk];
....
I think we should use rel2 here, not rel1.
--
Thanks,
Tender Wang
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: Wrong result when enable_partitionwise_join is on if collation of PartitionKey and Column is different.
2024-10-30 03:57 Re: Wrong result when enable_partitionwise_join is on if collation of PartitionKey and Column is different. jian he <[email protected]>
2024-10-30 12:36 ` Re: Wrong result when enable_partitionwise_join is on if collation of PartitionKey and Column is different. Junwang Zhao <[email protected]>
2024-10-31 13:08 ` Re: Wrong result when enable_partitionwise_join is on if collation of PartitionKey and Column is different. Amit Langote <[email protected]>
2024-11-01 02:39 ` Re: Wrong result when enable_partitionwise_join is on if collation of PartitionKey and Column is different. Tender Wang <[email protected]>
@ 2024-11-01 06:55 ` Amit Langote <[email protected]>
0 siblings, 0 replies; 15+ messages in thread
From: Amit Langote @ 2024-11-01 06:55 UTC (permalink / raw)
To: Tender Wang <[email protected]>; +Cc: Junwang Zhao <[email protected]>; jian he <[email protected]>; PostgreSQL Hackers <[email protected]>
Hi,
On Fri, Nov 1, 2024 at 11:39 AM Tender Wang <[email protected]> wrote:
> Amit Langote <[email protected]> 于2024年10月31日周四 21:09写道：
>> On Wed, Oct 30, 2024 at 9:36 PM Junwang Zhao <[email protected]> wrote:
>> > On Wed, Oct 30, 2024 at 11:58 AM jian he <[email protected]> wrote:
> In have_partkey_equi_join()
> ...
> if (exprs_known_equal(root, expr1, expr2, btree_opfamily))
> {
> Oid partcoll2 = rel1->part_scheme->partcollation[ipk];
> ....
> I think we should use rel2 here, not rel1.
Hmm, yeah, this should be fixed. Though, this is not a problem
because both rel1 and rel2 would be pointing to the same
PartitionScheme, because otherwise we wouldn't be in
have_partkey_equi_join(). See this in build_joinrel_partition_info():
/*
* We can only consider this join as an input to further partitionwise
* joins if (a) the input relations are partitioned and have
* consider_partitionwise_join=true, (b) the partition schemes match, and
* (c) we can identify an equi-join between the partition keys. Note that
* if it were possible for have_partkey_equi_join to return different
* answers for the same joinrel depending on which join ordering we try
* first, this logic would break. That shouldn't happen, though, because
* of the way the query planner deduces implied equalities and reorders
* the joins. Please see optimizer/README for details.
*/
if (outer_rel->part_scheme == NULL || inner_rel->part_scheme == NULL ||
!outer_rel->consider_partitionwise_join ||
!inner_rel->consider_partitionwise_join ||
outer_rel->part_scheme != inner_rel->part_scheme ||
!have_partkey_equi_join(root, joinrel, outer_rel, inner_rel,
sjinfo->jointype, restrictlist))
{
Assert(!IS_PARTITIONED_REL(joinrel));
return;
}
I've changed the condition to match only partcoll1 and exprcoll1, and
if they match, Assert that partcoll2 and exprcoll2 match too. That's
because partcoll1 and partcoll2 would be the same as they are from the
same PartitionScheme and expr1 and expr2 are known equal() so their
collations should match too.
--
Thanks, Amit Langote
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: Wrong result when enable_partitionwise_join is on if collation of PartitionKey and Column is different.
2024-10-30 03:57 Re: Wrong result when enable_partitionwise_join is on if collation of PartitionKey and Column is different. jian he <[email protected]>
2024-10-30 12:36 ` Re: Wrong result when enable_partitionwise_join is on if collation of PartitionKey and Column is different. Junwang Zhao <[email protected]>
2024-10-31 13:08 ` Re: Wrong result when enable_partitionwise_join is on if collation of PartitionKey and Column is different. Amit Langote <[email protected]>
@ 2024-11-01 02:59 ` Tender Wang <[email protected]>
2 siblings, 0 replies; 15+ messages in thread
From: Tender Wang @ 2024-11-01 02:59 UTC (permalink / raw)
To: Amit Langote <[email protected]>; +Cc: Junwang Zhao <[email protected]>; jian he <[email protected]>; PostgreSQL Hackers <[email protected]>
Amit Langote <[email protected]> 于2024年10月31日周四 21:09写道：
>
> 0001 is the patch for the partitionwise grouping issue (bug #18568).
> I concluded that even partial aggregate should be disallowed when the
> grouping collation doesn't match partitioning collation. The result
> with partial partitionwise grouping is not the same as when
> enable_partitionwise_aggregate is off.
>
LGTM
--
Thanks,
Tender Wang
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: Wrong result when enable_partitionwise_join is on if collation of PartitionKey and Column is different.
2024-10-30 03:57 Re: Wrong result when enable_partitionwise_join is on if collation of PartitionKey and Column is different. jian he <[email protected]>
2024-10-30 12:36 ` Re: Wrong result when enable_partitionwise_join is on if collation of PartitionKey and Column is different. Junwang Zhao <[email protected]>
2024-10-31 13:08 ` Re: Wrong result when enable_partitionwise_join is on if collation of PartitionKey and Column is different. Amit Langote <[email protected]>
@ 2024-11-01 05:39 ` jian he <[email protected]>
2024-11-01 07:25 ` Re: Wrong result when enable_partitionwise_join is on if collation of PartitionKey and Column is different. Amit Langote <[email protected]>
2 siblings, 1 reply; 15+ messages in thread
From: jian he @ 2024-11-01 05:39 UTC (permalink / raw)
To: Amit Langote <[email protected]>; +Cc: Junwang Zhao <[email protected]>; Tender Wang <[email protected]>; PostgreSQL Hackers <[email protected]>
On Thu, Oct 31, 2024 at 9:09 PM Amit Langote <[email protected]> wrote:
>
>
> I think we should insist that the join key collation and the partition
> collation are exactly the same and refuse to match them if they are
> not.
>
> + {
> + Oid colloid = exprCollation((Node *) expr);
> +
> + if ((partcoll != colloid) &&
> + OidIsValid(colloid) &&
> + !get_collation_isdeterministic(colloid))
> + *coll_incompatiable = true;
>
> I am not quite sure what is the point of checking whether or not the
> expression collation is deterministic after confirming that it's not
> the same as partcoll.
>
> Attached 0002 is what I came up with. One thing that's different from
> what Jian proposed is that match_expr_to_partition_keys() returns -1
> (expr not matched to any key) when the collation is also not matched
> instead of using a separate output parameter for that.
>
i was thinking that
CREATE TABLE part_tab (c text collate "POSIX") PARTITION BY LIST(c collate "C");
maybe can do partitionwise join.
join key collation and the partition key collation same sure would
make things easy.
about 0002.
Similar to PartCollMatchesExprColl in match_clause_to_partition_key
I think we can simply do the following:
no need to hack match_expr_to_partition_keys.
@@ -2181,6 +2181,9 @@ have_partkey_equi_join(PlannerInfo *root,
RelOptInfo *joinrel,
if (ipk1 != ipk2)
continue;
+ if (rel1->part_scheme->partcollation[ipk1] !=
opexpr->inputcollid)
+ return false;
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: Wrong result when enable_partitionwise_join is on if collation of PartitionKey and Column is different.
2024-10-30 03:57 Re: Wrong result when enable_partitionwise_join is on if collation of PartitionKey and Column is different. jian he <[email protected]>
2024-10-30 12:36 ` Re: Wrong result when enable_partitionwise_join is on if collation of PartitionKey and Column is different. Junwang Zhao <[email protected]>
2024-10-31 13:08 ` Re: Wrong result when enable_partitionwise_join is on if collation of PartitionKey and Column is different. Amit Langote <[email protected]>
2024-11-01 05:39 ` Re: Wrong result when enable_partitionwise_join is on if collation of PartitionKey and Column is different. jian he <[email protected]>
@ 2024-11-01 07:25 ` Amit Langote <[email protected]>
2024-11-01 08:07 ` Re: Wrong result when enable_partitionwise_join is on if collation of PartitionKey and Column is different. jian he <[email protected]>
0 siblings, 1 reply; 15+ messages in thread
From: Amit Langote @ 2024-11-01 07:25 UTC (permalink / raw)
To: jian he <[email protected]>; +Cc: Junwang Zhao <[email protected]>; Tender Wang <[email protected]>; PostgreSQL Hackers <[email protected]>; Ashutosh Bapat <[email protected]>
Hi,
On Fri, Nov 1, 2024 at 2:39 PM jian he <[email protected]> wrote:
> On Thu, Oct 31, 2024 at 9:09 PM Amit Langote <[email protected]> wrote:
> >
> >
> > I think we should insist that the join key collation and the partition
> > collation are exactly the same and refuse to match them if they are
> > not.
> >
> > + {
> > + Oid colloid = exprCollation((Node *) expr);
> > +
> > + if ((partcoll != colloid) &&
> > + OidIsValid(colloid) &&
> > + !get_collation_isdeterministic(colloid))
> > + *coll_incompatiable = true;
> >
> > I am not quite sure what is the point of checking whether or not the
> > expression collation is deterministic after confirming that it's not
> > the same as partcoll.
> >
> > Attached 0002 is what I came up with. One thing that's different from
> > what Jian proposed is that match_expr_to_partition_keys() returns -1
> > (expr not matched to any key) when the collation is also not matched
> > instead of using a separate output parameter for that.
> >
> i was thinking that
> CREATE TABLE part_tab (c text collate "POSIX") PARTITION BY LIST(c collate "C");
> maybe can do partitionwise join.
> join key collation and the partition key collation same sure would
> make things easy.
I think that's maybe ok to do as a new feature (use partitionwise join
even if collations differ but are both deterministic?), but we should
take a more restrictive approach in a bug fix that is to be
back-patched.
> about 0002.
> Similar to PartCollMatchesExprColl in match_clause_to_partition_key
> I think we can simply do the following:
> no need to hack match_expr_to_partition_keys.
>
> @@ -2181,6 +2181,9 @@ have_partkey_equi_join(PlannerInfo *root,
> RelOptInfo *joinrel,
> if (ipk1 != ipk2)
> continue;
>
> + if (rel1->part_scheme->partcollation[ipk1] !=
> opexpr->inputcollid)
> + return false;
Yes, looks like that should be enough, thanks.
I've updated the patch. I've added another test case to test the new
collation matching code in the code block of have_partkey_equi_join()
that pairs partition keys via equivalence class.
Adding Ashutosh to cc, as the original author of this code, to get his
thoughts on these fixes.
--
Thanks, Amit Langote
Attachments:
[application/octet-stream] v4-0002-Disallow-partitionwise-join-when-collation-doesn-.patch (12.2K, ../../CA+HiwqEQHVbAdFWNcrnDEZXk=m-Mk5ZtCoFq_tKkyoHPRATuag@mail.gmail.com/2-v4-0002-Disallow-partitionwise-join-when-collation-doesn-.patch)
download | inline diff:
From b9cd75e360e47aeed008d8060473c617b0f6deb1 Mon Sep 17 00:00:00 2001
From: Amit Langote <[email protected]>
Date: Fri, 1 Nov 2024 15:32:33 +0900
Subject: [PATCH v4 2/2] Disallow partitionwise join when collation doesn't
match
Insist that the collation used for joining matches exactly with the
collation used for partitioning to allow using partitionwise join.
Reported-by: Tender Wang <[email protected]>
Author: Jian He <[email protected]>
Author: Amit Langote <[email protected]>
Reviewed-by: Tender Wang <[email protected]>
Reviewed-by: Junwang Zhao <[email protected]>
Discussion: https://postgr.es/m/[email protected]
Discussion: https://postgr.es/m/[email protected]
Discussion: https://postgr.es/m/CAHewXNno_HKiQ6PqyLYfuqDtwp7KKHZiH1J7Pqyz0nr+PS2Dwg@mail.gmail.com
---
src/backend/optimizer/util/relnode.c | 28 +++-
.../regress/expected/collate.icu.utf8.out | 154 ++++++++++++++++++
src/test/regress/sql/collate.icu.utf8.sql | 35 ++++
3 files changed, 215 insertions(+), 2 deletions(-)
diff --git a/src/backend/optimizer/util/relnode.c b/src/backend/optimizer/util/relnode.c
index d7266e4cdb..489fe6c26f 100644
--- a/src/backend/optimizer/util/relnode.c
+++ b/src/backend/optimizer/util/relnode.c
@@ -2185,6 +2185,10 @@ have_partkey_equi_join(PlannerInfo *root, RelOptInfo *joinrel,
if (pk_known_equal[ipk1])
continue;
+ /* Reject if the partition key collation differs from the clause's. */
+ if (rel1->part_scheme->partcollation[ipk1] != opexpr->inputcollid)
+ return false;
+
/*
* The clause allows partitionwise join only if it uses the same
* operator family as that specified by the partition key.
@@ -2258,6 +2262,8 @@ have_partkey_equi_join(PlannerInfo *root, RelOptInfo *joinrel,
{
Node *expr1 = (Node *) lfirst(lc);
ListCell *lc2;
+ Oid partcoll1 = rel1->part_scheme->partcollation[ipk];
+ Oid exprcoll1 = exprCollation(expr1);
foreach(lc2, rel2->partexprs[ipk])
{
@@ -2265,8 +2271,26 @@ have_partkey_equi_join(PlannerInfo *root, RelOptInfo *joinrel,
if (exprs_known_equal(root, expr1, expr2, btree_opfamily))
{
- pk_known_equal[ipk] = true;
- break;
+ /*
+ * Ensure that the collation of the expression matches
+ * that of the partition key. Checking just one collation
+ * (partcoll1 and exprcoll1) suffices because partcoll1
+ * and partcoll2, as well as exprcoll1 and exprcoll2,
+ * should be identical. This holds because both rel1 and
+ * rel2 use the same PartitionScheme and expr1 and expr2
+ * are equal.
+ */
+ if (partcoll1 == exprcoll1)
+ {
+ Oid partcoll2 PG_USED_FOR_ASSERTS_ONLY =
+ rel1->part_scheme->partcollation[ipk];
+ Oid exprcoll2 PG_USED_FOR_ASSERTS_ONLY =
+ exprCollation(expr2);
+
+ Assert(partcoll2 == exprcoll2);
+ pk_known_equal[ipk] = true;
+ break;
+ }
}
}
if (pk_known_equal[ipk])
diff --git a/src/test/regress/expected/collate.icu.utf8.out b/src/test/regress/expected/collate.icu.utf8.out
index 737cf363a2..abc3ce23a4 100644
--- a/src/test/regress/expected/collate.icu.utf8.out
+++ b/src/test/regress/expected/collate.icu.utf8.out
@@ -2136,7 +2136,161 @@ SELECT c collate "C", count(c) FROM pagg_tab3 GROUP BY c collate "C" ORDER BY 1;
b | 2
(4 rows)
+-- Partitionwise join should not be allowed too when the collation used by the
+-- join keys doesn't match the partition key collation.
+SET enable_partitionwise_join TO false;
+EXPLAIN (COSTS OFF)
+SELECT t1.c, count(t2.c) FROM pagg_tab3 t1 JOIN pagg_tab3 t2 ON t1.c = t2.c GROUP BY 1 ORDER BY 1;
+ QUERY PLAN
+-------------------------------------------------------------
+ Sort
+ Sort Key: t1.c COLLATE case_insensitive
+ -> HashAggregate
+ Group Key: t1.c
+ -> Hash Join
+ Hash Cond: (t1.c = t2.c)
+ -> Append
+ -> Seq Scan on pagg_tab3_p2 t1_1
+ -> Seq Scan on pagg_tab3_p1 t1_2
+ -> Hash
+ -> Append
+ -> Seq Scan on pagg_tab3_p2 t2_1
+ -> Seq Scan on pagg_tab3_p1 t2_2
+(13 rows)
+
+SELECT t1.c, count(t2.c) FROM pagg_tab3 t1 JOIN pagg_tab3 t2 ON t1.c = t2.c GROUP BY 1 ORDER BY 1;
+ c | count
+---+-------
+ A | 16
+ B | 16
+(2 rows)
+
+SET enable_partitionwise_join TO true;
+EXPLAIN (COSTS OFF)
+SELECT t1.c, count(t2.c) FROM pagg_tab3 t1 JOIN pagg_tab3 t2 ON t1.c = t2.c GROUP BY 1 ORDER BY 1;
+ QUERY PLAN
+-------------------------------------------------------------
+ Sort
+ Sort Key: t1.c COLLATE case_insensitive
+ -> HashAggregate
+ Group Key: t1.c
+ -> Hash Join
+ Hash Cond: (t1.c = t2.c)
+ -> Append
+ -> Seq Scan on pagg_tab3_p2 t1_1
+ -> Seq Scan on pagg_tab3_p1 t1_2
+ -> Hash
+ -> Append
+ -> Seq Scan on pagg_tab3_p2 t2_1
+ -> Seq Scan on pagg_tab3_p1 t2_2
+(13 rows)
+
+SELECT t1.c, count(t2.c) FROM pagg_tab3 t1 JOIN pagg_tab3 t2 ON t1.c = t2.c GROUP BY 1 ORDER BY 1;
+ c | count
+---+-------
+ A | 16
+ B | 16
+(2 rows)
+
+-- OK when the join clause uses the same collation as the partition key.
+EXPLAIN (COSTS OFF)
+SELECT t1.c COLLATE "C", count(t2.c) FROM pagg_tab3 t1 JOIN pagg_tab3 t2 ON t1.c = t2.c COLLATE "C" GROUP BY t1.c COLLATE "C" ORDER BY 1;
+ QUERY PLAN
+------------------------------------------------------------------
+ Sort
+ Sort Key: ((t1.c)::text) COLLATE "C"
+ -> Append
+ -> HashAggregate
+ Group Key: (t1.c)::text
+ -> Hash Join
+ Hash Cond: ((t1.c)::text = (t2.c)::text)
+ -> Seq Scan on pagg_tab3_p2 t1
+ -> Hash
+ -> Seq Scan on pagg_tab3_p2 t2
+ -> HashAggregate
+ Group Key: (t1_1.c)::text
+ -> Hash Join
+ Hash Cond: ((t1_1.c)::text = (t2_1.c)::text)
+ -> Seq Scan on pagg_tab3_p1 t1_1
+ -> Hash
+ -> Seq Scan on pagg_tab3_p1 t2_1
+(17 rows)
+
+SELECT t1.c COLLATE "C", count(t2.c) FROM pagg_tab3 t1 JOIN pagg_tab3 t2 ON t1.c = t2.c COLLATE "C" GROUP BY t1.c COLLATE "C" ORDER BY 1;
+ c | count
+---+-------
+ A | 4
+ B | 4
+ a | 4
+ b | 4
+(4 rows)
+
+-- Another case where the partition keys are matched via equivalence class,
+-- not a join restriction clause.
+CREATE TABLE pagg_tab4 (c text collate case_insensitive, b text collate case_insensitive) PARTITION BY LIST(b collate "C");
+CREATE TABLE pagg_tab4_p1 PARTITION OF pagg_tab4 FOR VALUES IN ('a', 'b');
+CREATE TABLE pagg_tab4_p2 PARTITION OF pagg_tab4 FOR VALUES IN ('B', 'A');
+INSERT INTO pagg_tab4 (b, c) SELECT substr('abAB', (i % 4) + 1 , 1), substr('abAB', (i % 2) + 1 , 1) FROM generate_series(0, 7) i;
+ANALYZE pagg_tab4;
+EXPLAIN (COSTS OFF)
+SELECT t1.c, count(t2.c) FROM pagg_tab3 t1 JOIN pagg_tab4 t2 ON t1.c = t2.c AND t1.c = t2.b GROUP BY 1 ORDER BY 1;
+ QUERY PLAN
+-------------------------------------------------------------
+ Sort
+ Sort Key: t1.c COLLATE case_insensitive
+ -> HashAggregate
+ Group Key: t1.c
+ -> Hash Join
+ Hash Cond: (t1.c = t2.c)
+ -> Append
+ -> Seq Scan on pagg_tab3_p2 t1_1
+ -> Seq Scan on pagg_tab3_p1 t1_2
+ -> Hash
+ -> Append
+ -> Seq Scan on pagg_tab4_p2 t2_1
+ Filter: (c = b)
+ -> Seq Scan on pagg_tab4_p1 t2_2
+ Filter: (c = b)
+(15 rows)
+
+SELECT t1.c, count(t2.c) FROM pagg_tab3 t1 JOIN pagg_tab4 t2 ON t1.c = t2.c AND t1.c = t2.b GROUP BY 1 ORDER BY 1;
+ c | count
+---+-------
+ A | 16
+ B | 16
+(2 rows)
+
+-- OK when the join clause uses the same collation as the partition key
+EXPLAIN (COSTS OFF)
+SELECT t1.c, count(t2.c) FROM pagg_tab3 t1 JOIN pagg_tab4 t2 ON t1.c = t2.c AND t1.c = t2.b COLLATE "C" GROUP BY 1 ORDER BY 1;
+ QUERY PLAN
+------------------------------------------------------------------------------------------
+ Sort
+ Sort Key: t1.c COLLATE case_insensitive
+ -> HashAggregate
+ Group Key: t1.c
+ -> Append
+ -> Hash Join
+ Hash Cond: ((t1_1.c = t2_1.c) AND ((t1_1.c)::text = (t2_1.b)::text))
+ -> Seq Scan on pagg_tab3_p2 t1_1
+ -> Hash
+ -> Seq Scan on pagg_tab4_p2 t2_1
+ -> Hash Join
+ Hash Cond: ((t1_2.c = t2_2.c) AND ((t1_2.c)::text = (t2_2.b)::text))
+ -> Seq Scan on pagg_tab3_p1 t1_2
+ -> Hash
+ -> Seq Scan on pagg_tab4_p1 t2_2
+(15 rows)
+
+SELECT t1.c, count(t2.c) FROM pagg_tab3 t1 JOIN pagg_tab4 t2 ON t1.c = t2.c AND t1.c = t2.b COLLATE "C" GROUP BY 1 ORDER BY 1;
+ c | count
+---+-------
+ A | 8
+ B | 8
+(2 rows)
+
DROP TABLE pagg_tab3;
+DROP TABLE pagg_tab4;
RESET enable_partitionwise_aggregate;
RESET max_parallel_workers_per_gather;
RESET enable_incremental_sort;
diff --git a/src/test/regress/sql/collate.icu.utf8.sql b/src/test/regress/sql/collate.icu.utf8.sql
index ca1bb7c1f2..2f37a1a97b 100644
--- a/src/test/regress/sql/collate.icu.utf8.sql
+++ b/src/test/regress/sql/collate.icu.utf8.sql
@@ -828,7 +828,42 @@ EXPLAIN (COSTS OFF)
SELECT c collate "C", count(c) FROM pagg_tab3 GROUP BY c collate "C" ORDER BY 1;
SELECT c collate "C", count(c) FROM pagg_tab3 GROUP BY c collate "C" ORDER BY 1;
+-- Partitionwise join should not be allowed too when the collation used by the
+-- join keys doesn't match the partition key collation.
+SET enable_partitionwise_join TO false;
+EXPLAIN (COSTS OFF)
+SELECT t1.c, count(t2.c) FROM pagg_tab3 t1 JOIN pagg_tab3 t2 ON t1.c = t2.c GROUP BY 1 ORDER BY 1;
+SELECT t1.c, count(t2.c) FROM pagg_tab3 t1 JOIN pagg_tab3 t2 ON t1.c = t2.c GROUP BY 1 ORDER BY 1;
+
+SET enable_partitionwise_join TO true;
+EXPLAIN (COSTS OFF)
+SELECT t1.c, count(t2.c) FROM pagg_tab3 t1 JOIN pagg_tab3 t2 ON t1.c = t2.c GROUP BY 1 ORDER BY 1;
+SELECT t1.c, count(t2.c) FROM pagg_tab3 t1 JOIN pagg_tab3 t2 ON t1.c = t2.c GROUP BY 1 ORDER BY 1;
+
+-- OK when the join clause uses the same collation as the partition key.
+EXPLAIN (COSTS OFF)
+SELECT t1.c COLLATE "C", count(t2.c) FROM pagg_tab3 t1 JOIN pagg_tab3 t2 ON t1.c = t2.c COLLATE "C" GROUP BY t1.c COLLATE "C" ORDER BY 1;
+SELECT t1.c COLLATE "C", count(t2.c) FROM pagg_tab3 t1 JOIN pagg_tab3 t2 ON t1.c = t2.c COLLATE "C" GROUP BY t1.c COLLATE "C" ORDER BY 1;
+
+-- Another case where the partition keys are matched via equivalence class,
+-- not a join restriction clause.
+CREATE TABLE pagg_tab4 (c text collate case_insensitive, b text collate case_insensitive) PARTITION BY LIST(b collate "C");
+CREATE TABLE pagg_tab4_p1 PARTITION OF pagg_tab4 FOR VALUES IN ('a', 'b');
+CREATE TABLE pagg_tab4_p2 PARTITION OF pagg_tab4 FOR VALUES IN ('B', 'A');
+INSERT INTO pagg_tab4 (b, c) SELECT substr('abAB', (i % 4) + 1 , 1), substr('abAB', (i % 2) + 1 , 1) FROM generate_series(0, 7) i;
+ANALYZE pagg_tab4;
+
+EXPLAIN (COSTS OFF)
+SELECT t1.c, count(t2.c) FROM pagg_tab3 t1 JOIN pagg_tab4 t2 ON t1.c = t2.c AND t1.c = t2.b GROUP BY 1 ORDER BY 1;
+SELECT t1.c, count(t2.c) FROM pagg_tab3 t1 JOIN pagg_tab4 t2 ON t1.c = t2.c AND t1.c = t2.b GROUP BY 1 ORDER BY 1;
+
+-- OK when the join clause uses the same collation as the partition key
+EXPLAIN (COSTS OFF)
+SELECT t1.c, count(t2.c) FROM pagg_tab3 t1 JOIN pagg_tab4 t2 ON t1.c = t2.c AND t1.c = t2.b COLLATE "C" GROUP BY 1 ORDER BY 1;
+SELECT t1.c, count(t2.c) FROM pagg_tab3 t1 JOIN pagg_tab4 t2 ON t1.c = t2.c AND t1.c = t2.b COLLATE "C" GROUP BY 1 ORDER BY 1;
+
DROP TABLE pagg_tab3;
+DROP TABLE pagg_tab4;
RESET enable_partitionwise_aggregate;
RESET max_parallel_workers_per_gather;
--
2.43.0
[application/octet-stream] v4-0001-Disallow-partitionwise-grouping-when-collation-do.patch (12.6K, ../../CA+HiwqEQHVbAdFWNcrnDEZXk=m-Mk5ZtCoFq_tKkyoHPRATuag@mail.gmail.com/3-v4-0001-Disallow-partitionwise-grouping-when-collation-do.patch)
download | inline diff:
From d8f6753eeb70349ccb45d21dc741dbe6fbaaf2a5 Mon Sep 17 00:00:00 2001
From: Amit Langote <[email protected]>
Date: Fri, 1 Nov 2024 16:15:50 +0900
Subject: [PATCH v4 1/2] Disallow partitionwise grouping when collation doesn't
match
Insist that the collation used for grouping matches exactly with the
collation used for partitioning to allow using either full or
partial partitionwise grouping.
Bug: #18568
Reported-by: Webbo Han <[email protected]>
Author: Webbo Han <[email protected]>
Reviewed-by: Tender Wang <[email protected]>
Reviewed-by: Aleksander Alekseev <[email protected]>
Reviewed-by: Jian He <[email protected]>
Discussion: https://postgr.es/m/[email protected]
Discussion: https://postgr.es/m/[email protected]
Discussion: https://postgr.es/m/CAHewXNno_HKiQ6PqyLYfuqDtwp7KKHZiH1J7Pqyz0nr+PS2Dwg@mail.gmail.com
---
src/backend/optimizer/plan/planner.c | 66 +++++++++++---
.../regress/expected/collate.icu.utf8.out | 86 +++++++++++++++++++
.../regress/expected/partition_aggregate.out | 11 ---
src/test/regress/sql/collate.icu.utf8.sql | 37 ++++++++
src/test/regress/sql/partition_aggregate.sql | 1 -
5 files changed, 175 insertions(+), 26 deletions(-)
diff --git a/src/backend/optimizer/plan/planner.c b/src/backend/optimizer/plan/planner.c
index 0f423e9684..4044b13071 100644
--- a/src/backend/optimizer/plan/planner.c
+++ b/src/backend/optimizer/plan/planner.c
@@ -253,7 +253,8 @@ static void create_partitionwise_grouping_paths(PlannerInfo *root,
GroupPathExtraData *extra);
static bool group_by_has_partkey(RelOptInfo *input_rel,
List *targetList,
- List *groupClause);
+ List *groupClause,
+ bool *collation_incompatible);
static int common_prefix_cmp(const void *a, const void *b);
static List *generate_setop_child_grouplist(SetOperationStmt *op,
List *targetlist);
@@ -4090,23 +4091,30 @@ create_ordinary_grouping_paths(PlannerInfo *root, RelOptInfo *input_rel,
if (extra->patype != PARTITIONWISE_AGGREGATE_NONE &&
IS_PARTITIONED_REL(input_rel))
{
+ bool collation_incompatible = false;
+ bool group_by_contains_partkey =
+ group_by_has_partkey(input_rel, extra->targetList,
+ root->parse->groupClause,
+ &collation_incompatible);
+
/*
* If this is the topmost relation or if the parent relation is doing
* full partitionwise aggregation, then we can do full partitionwise
* aggregation provided that the GROUP BY clause contains all of the
- * partitioning columns at this level. Otherwise, we can do at most
- * partial partitionwise aggregation. But if partial aggregation is
- * not supported in general then we can't use it for partitionwise
- * aggregation either.
+ * partitioning columns at this level and the collation used by GROUP
+ * BY matches the partitioning collation. Otherwise, we can do at most
+ * partial partitionwise aggregation, but again only if the collation
+ * is compatible. If partial aggregation is not supported in general
+ * then we can't use it for partitionwise aggregation either.
*
* Check parse->groupClause not processed_groupClause, because it's
* okay if some of the partitioning columns were proved redundant.
*/
if (extra->patype == PARTITIONWISE_AGGREGATE_FULL &&
- group_by_has_partkey(input_rel, extra->targetList,
- root->parse->groupClause))
+ group_by_contains_partkey)
patype = PARTITIONWISE_AGGREGATE_FULL;
- else if ((extra->flags & GROUPING_CAN_PARTIAL_AGG) != 0)
+ else if ((extra->flags & GROUPING_CAN_PARTIAL_AGG) != 0 &&
+ !collation_incompatible)
patype = PARTITIONWISE_AGGREGATE_PARTIAL;
else
patype = PARTITIONWISE_AGGREGATE_NONE;
@@ -8105,13 +8113,18 @@ create_partitionwise_grouping_paths(PlannerInfo *root,
/*
* group_by_has_partkey
*
- * Returns true, if all the partition keys of the given relation are part of
- * the GROUP BY clauses, false otherwise.
+ * Returns true if all the partition keys of the given relation are part of
+ * the GROUP BY clauses, including having matching collation, false otherwise.
+ *
+ * Returns false also if a collation mismatch is detected between a partition
+ * key and its corresponding expression in groupClause, in which case.
+ * *collation_incompatible is set to true.
*/
static bool
group_by_has_partkey(RelOptInfo *input_rel,
List *targetList,
- List *groupClause)
+ List *groupClause,
+ bool *collation_incompatible)
{
List *groupexprs = get_sortgrouplist_exprs(groupClause, targetList);
int cnt = 0;
@@ -8134,13 +8147,38 @@ 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 groupcoll = exprCollation((Node *) groupexpr);
+
+ if (IsA(groupexpr, RelabelType))
+ groupexpr = ((RelabelType *) groupexpr)->arg;
+
+ /*
+ * Ensure that the grouping collation is compatible with the
+ * partitioning collation.
+ */
+ if (OidIsValid(partcoll) && OidIsValid(groupcoll) &&
+ partcoll != groupcoll)
+ {
+ *collation_incompatible = true;
+ return false;
+ }
+
+ if (equal(groupexpr, partexpr))
+ {
+ found = true;
+ break;
+ }
}
+
+ if (found)
+ break;
}
/*
diff --git a/src/test/regress/expected/collate.icu.utf8.out b/src/test/regress/expected/collate.icu.utf8.out
index faa376e060..737cf363a2 100644
--- a/src/test/regress/expected/collate.icu.utf8.out
+++ b/src/test/regress/expected/collate.icu.utf8.out
@@ -2054,6 +2054,92 @@ SELECT (SELECT count(*) FROM test33_0) <> (SELECT count(*) FROM test33_1);
t
(1 row)
+--
+-- Bug #18568
+--
+-- Partitionwise aggregate (full or partial) should not be used when a
+-- partition key's collation doesn't match that of the GROUP BY column it is
+-- matched with.
+SET max_parallel_workers_per_gather TO 0;
+SET enable_incremental_sort TO off;
+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');
+CREATE TABLE pagg_tab3_p2 PARTITION OF pagg_tab3 FOR VALUES IN ('B', 'A');
+INSERT INTO pagg_tab3 SELECT substr('abAB', (i % 4) +1 , 1) FROM generate_series(0, 7) i;
+ANALYZE pagg_tab3;
+SET enable_partitionwise_aggregate TO false;
+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
+ -> HashAggregate
+ Group Key: pagg_tab3.c
+ -> Append
+ -> Seq Scan on pagg_tab3_p2 pagg_tab3_1
+ -> Seq Scan on pagg_tab3_p1 pagg_tab3_2
+(7 rows)
+
+SELECT upper(c collate case_insensitive), count(c) FROM pagg_tab3 GROUP BY c collate case_insensitive ORDER BY 1;
+ upper | count
+-------+-------
+ A | 4
+ B | 4
+(2 rows)
+
+-- No partitionwise aggregation allowed!
+SET enable_partitionwise_aggregate TO true;
+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
+ -> HashAggregate
+ Group Key: pagg_tab3.c
+ -> Append
+ -> Seq Scan on pagg_tab3_p2 pagg_tab3_1
+ -> Seq Scan on pagg_tab3_p1 pagg_tab3_2
+(7 rows)
+
+SELECT upper(c collate case_insensitive), count(c) FROM pagg_tab3 GROUP BY c collate case_insensitive ORDER BY 1;
+ upper | count
+-------+-------
+ A | 4
+ B | 4
+(2 rows)
+
+-- OK to use full partitionwise aggregate after changing the GROUP BY column's
+-- collation to be the same as that of the partition key.
+EXPLAIN (COSTS OFF)
+SELECT c collate "C", count(c) FROM pagg_tab3 GROUP BY c collate "C" ORDER BY 1;
+ QUERY PLAN
+--------------------------------------------------------
+ Sort
+ Sort Key: ((pagg_tab3.c)::text) COLLATE "C"
+ -> Append
+ -> HashAggregate
+ Group Key: (pagg_tab3.c)::text
+ -> Seq Scan on pagg_tab3_p2 pagg_tab3
+ -> HashAggregate
+ Group Key: (pagg_tab3_1.c)::text
+ -> Seq Scan on pagg_tab3_p1 pagg_tab3_1
+(9 rows)
+
+SELECT c collate "C", count(c) FROM pagg_tab3 GROUP BY c collate "C" ORDER BY 1;
+ c | count
+---+-------
+ A | 2
+ B | 2
+ a | 2
+ b | 2
+(4 rows)
+
+DROP TABLE pagg_tab3;
+RESET enable_partitionwise_aggregate;
+RESET max_parallel_workers_per_gather;
+RESET enable_incremental_sort;
-- cleanup
RESET search_path;
SET client_min_messages TO warning;
diff --git a/src/test/regress/expected/partition_aggregate.out b/src/test/regress/expected/partition_aggregate.out
index 5f2c0cf578..670eb98906 100644
--- a/src/test/regress/expected/partition_aggregate.out
+++ b/src/test/regress/expected/partition_aggregate.out
@@ -1507,14 +1507,3 @@ SELECT x, sum(y), avg(y), count(*) FROM pagg_tab_para GROUP BY x HAVING avg(y) <
-> Seq Scan on pagg_tab_para_p3 pagg_tab_para_2
(15 rows)
-SELECT x, sum(y), avg(y), count(*) FROM pagg_tab_para GROUP BY x HAVING avg(y) < 7 ORDER BY 1, 2, 3;
- x | sum | avg | count
-----+------+--------------------+-------
- 0 | 5000 | 5.0000000000000000 | 1000
- 1 | 6000 | 6.0000000000000000 | 1000
- 10 | 5000 | 5.0000000000000000 | 1000
- 11 | 6000 | 6.0000000000000000 | 1000
- 20 | 5000 | 5.0000000000000000 | 1000
- 21 | 6000 | 6.0000000000000000 | 1000
-(6 rows)
-
diff --git a/src/test/regress/sql/collate.icu.utf8.sql b/src/test/regress/sql/collate.icu.utf8.sql
index 80f28a97d7..ca1bb7c1f2 100644
--- a/src/test/regress/sql/collate.icu.utf8.sql
+++ b/src/test/regress/sql/collate.icu.utf8.sql
@@ -796,6 +796,43 @@ INSERT INTO test33 VALUES (2, 'DEF');
-- they end up in the same partition (but it's platform-dependent which one)
SELECT (SELECT count(*) FROM test33_0) <> (SELECT count(*) FROM test33_1);
+--
+-- Bug #18568
+--
+-- Partitionwise aggregate (full or partial) should not be used when a
+-- partition key's collation doesn't match that of the GROUP BY column it is
+-- matched with.
+SET max_parallel_workers_per_gather TO 0;
+SET enable_incremental_sort TO off;
+
+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');
+CREATE TABLE pagg_tab3_p2 PARTITION OF pagg_tab3 FOR VALUES IN ('B', 'A');
+INSERT INTO pagg_tab3 SELECT substr('abAB', (i % 4) +1 , 1) FROM generate_series(0, 7) i;
+ANALYZE pagg_tab3;
+
+SET enable_partitionwise_aggregate TO false;
+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;
+
+-- No partitionwise aggregation allowed!
+SET enable_partitionwise_aggregate TO true;
+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;
+
+-- OK to use full partitionwise aggregate after changing the GROUP BY column's
+-- collation to be the same as that of the partition key.
+EXPLAIN (COSTS OFF)
+SELECT c collate "C", count(c) FROM pagg_tab3 GROUP BY c collate "C" ORDER BY 1;
+SELECT c collate "C", count(c) FROM pagg_tab3 GROUP BY c collate "C" ORDER BY 1;
+
+DROP TABLE pagg_tab3;
+
+RESET enable_partitionwise_aggregate;
+RESET max_parallel_workers_per_gather;
+RESET enable_incremental_sort;
-- cleanup
RESET search_path;
diff --git a/src/test/regress/sql/partition_aggregate.sql b/src/test/regress/sql/partition_aggregate.sql
index ab070fee24..1e263c1caf 100644
--- a/src/test/regress/sql/partition_aggregate.sql
+++ b/src/test/regress/sql/partition_aggregate.sql
@@ -333,4 +333,3 @@ 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;
--
2.43.0
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: Wrong result when enable_partitionwise_join is on if collation of PartitionKey and Column is different.
2024-10-30 03:57 Re: Wrong result when enable_partitionwise_join is on if collation of PartitionKey and Column is different. jian he <[email protected]>
2024-10-30 12:36 ` Re: Wrong result when enable_partitionwise_join is on if collation of PartitionKey and Column is different. Junwang Zhao <[email protected]>
2024-10-31 13:08 ` Re: Wrong result when enable_partitionwise_join is on if collation of PartitionKey and Column is different. Amit Langote <[email protected]>
2024-11-01 05:39 ` Re: Wrong result when enable_partitionwise_join is on if collation of PartitionKey and Column is different. jian he <[email protected]>
2024-11-01 07:25 ` Re: Wrong result when enable_partitionwise_join is on if collation of PartitionKey and Column is different. Amit Langote <[email protected]>
@ 2024-11-01 08:07 ` jian he <[email protected]>
2024-11-01 10:08 ` Re: Wrong result when enable_partitionwise_join is on if collation of PartitionKey and Column is different. Amit Langote <[email protected]>
0 siblings, 1 reply; 15+ messages in thread
From: jian he @ 2024-11-01 08:07 UTC (permalink / raw)
To: Amit Langote <[email protected]>; +Cc: Junwang Zhao <[email protected]>; Tender Wang <[email protected]>; PostgreSQL Hackers <[email protected]>; Ashutosh Bapat <[email protected]>
just a quick reply while testing v4-0001.
tests copy from src/test/regress/sql/partition_aggregate.sql first 40 lines.
drop table if exists pagg_tab;
CREATE TABLE pagg_tab (a int, b int, c text, d int) PARTITION BY
LIST(c collate "C");
CREATE TABLE pagg_tab_p1 PARTITION OF pagg_tab FOR VALUES IN ('0000',
'0001', '0002', '0003', '0004');
CREATE TABLE pagg_tab_p2 PARTITION OF pagg_tab FOR VALUES IN ('0005',
'0006', '0007', '0008');
CREATE TABLE pagg_tab_p3 PARTITION OF pagg_tab FOR VALUES IN ('0009',
'0010', '0011');
INSERT INTO pagg_tab SELECT (i % 20), (i % 30), to_char(i % 12,
'FM0000'), i % 30 FROM generate_series(0, 2999) i;
ANALYZE pagg_tab;
EXPLAIN (COSTS OFF, settings)
SELECT a, sum(b), avg(b), count(*), max(b) FROM pagg_tab GROUP BY a;
QUERY PLAN
-----------------------------------------------------------------------------------------------------------------------------------------------------------
Finalize HashAggregate
Group Key: pagg_tab.a
-> Append
-> Partial HashAggregate
Group Key: pagg_tab.a
-> Seq Scan on pagg_tab_p1 pagg_tab
-> Partial HashAggregate
Group Key: pagg_tab_1.a
-> Seq Scan on pagg_tab_p2 pagg_tab_1
-> Partial HashAggregate
Group Key: pagg_tab_2.a
-> Seq Scan on pagg_tab_p3 pagg_tab_2
Settings: enable_partitionwise_aggregate = 'on',
enable_partitionwise_join = 'on', max_parallel_workers_per_gather =
'0', enable_increm
ental_sort = 'off'
drop table if exists pagg_tab;
CREATE TABLE pagg_tab (a text, b int, c text, d int) PARTITION BY
LIST(c collate "C");
CREATE TABLE pagg_tab_p1 PARTITION OF pagg_tab FOR VALUES IN ('0000',
'0001', '0002', '0003', '0004');
CREATE TABLE pagg_tab_p2 PARTITION OF pagg_tab FOR VALUES IN ('0005',
'0006', '0007', '0008');
CREATE TABLE pagg_tab_p3 PARTITION OF pagg_tab FOR VALUES IN ('0009',
'0010', '0011');
INSERT INTO pagg_tab SELECT (i % 20)::text, (i % 30), to_char(i % 12,
'FM0000'), i % 30 FROM generate_series(0, 2999) i;
ANALYZE pagg_tab;
EXPLAIN (COSTS OFF, settings)
SELECT a, sum(b), avg(b), count(*), max(b) FROM pagg_tab GROUP BY a;
QUERY PLAN
-----------------------------------------------------------------------------------------------------------------------------------------------------------
HashAggregate
Group Key: pagg_tab.a
-> Append
-> Seq Scan on pagg_tab_p1 pagg_tab_1
-> Seq Scan on pagg_tab_p2 pagg_tab_2
-> Seq Scan on pagg_tab_p3 pagg_tab_3
Settings: enable_partitionwise_aggregate = 'on',
enable_partitionwise_join = 'on', max_parallel_workers_per_gather =
'0', enable_increm
ental_sort = 'off'
it seems "PARTITION BY LIST(c collate "C");" collation compare with
"GROUP BY a;".
set collation_incompatible returned true.
make it cannot do PARTITIONWISE_AGGREGATE_PARTIAL.
but here "group by a", "a" is text data type, we can still do
PARTITIONWISE_AGGREGATE_PARTIAL
?
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: Wrong result when enable_partitionwise_join is on if collation of PartitionKey and Column is different.
2024-10-30 03:57 Re: Wrong result when enable_partitionwise_join is on if collation of PartitionKey and Column is different. jian he <[email protected]>
2024-10-30 12:36 ` Re: Wrong result when enable_partitionwise_join is on if collation of PartitionKey and Column is different. Junwang Zhao <[email protected]>
2024-10-31 13:08 ` Re: Wrong result when enable_partitionwise_join is on if collation of PartitionKey and Column is different. Amit Langote <[email protected]>
2024-11-01 05:39 ` Re: Wrong result when enable_partitionwise_join is on if collation of PartitionKey and Column is different. jian he <[email protected]>
2024-11-01 07:25 ` Re: Wrong result when enable_partitionwise_join is on if collation of PartitionKey and Column is different. Amit Langote <[email protected]>
2024-11-01 08:07 ` Re: Wrong result when enable_partitionwise_join is on if collation of PartitionKey and Column is different. jian he <[email protected]>
@ 2024-11-01 10:08 ` Amit Langote <[email protected]>
2024-11-04 03:27 ` Re: Wrong result when enable_partitionwise_join is on if collation of PartitionKey and Column is different. jian he <[email protected]>
0 siblings, 1 reply; 15+ messages in thread
From: Amit Langote @ 2024-11-01 10:08 UTC (permalink / raw)
To: jian he <[email protected]>; +Cc: Junwang Zhao <[email protected]>; Tender Wang <[email protected]>; PostgreSQL Hackers <[email protected]>; Ashutosh Bapat <[email protected]>
On Fri, Nov 1, 2024 at 5:08 PM jian he <[email protected]> wrote:
> just a quick reply while testing v4-0001.
> tests copy from src/test/regress/sql/partition_aggregate.sql first 40 lines.
>
> drop table if exists pagg_tab;
> CREATE TABLE pagg_tab (a int, b int, c text, d int) PARTITION BY
> LIST(c collate "C");
> CREATE TABLE pagg_tab_p1 PARTITION OF pagg_tab FOR VALUES IN ('0000',
> '0001', '0002', '0003', '0004');
> CREATE TABLE pagg_tab_p2 PARTITION OF pagg_tab FOR VALUES IN ('0005',
> '0006', '0007', '0008');
> CREATE TABLE pagg_tab_p3 PARTITION OF pagg_tab FOR VALUES IN ('0009',
> '0010', '0011');
> INSERT INTO pagg_tab SELECT (i % 20), (i % 30), to_char(i % 12,
> 'FM0000'), i % 30 FROM generate_series(0, 2999) i;
> ANALYZE pagg_tab;
>
> EXPLAIN (COSTS OFF, settings)
> SELECT a, sum(b), avg(b), count(*), max(b) FROM pagg_tab GROUP BY a;
>
>
> QUERY PLAN
>
> -----------------------------------------------------------------------------------------------------------------------------------------------------------
> Finalize HashAggregate
> Group Key: pagg_tab.a
> -> Append
> -> Partial HashAggregate
> Group Key: pagg_tab.a
> -> Seq Scan on pagg_tab_p1 pagg_tab
> -> Partial HashAggregate
> Group Key: pagg_tab_1.a
> -> Seq Scan on pagg_tab_p2 pagg_tab_1
> -> Partial HashAggregate
> Group Key: pagg_tab_2.a
> -> Seq Scan on pagg_tab_p3 pagg_tab_2
> Settings: enable_partitionwise_aggregate = 'on',
> enable_partitionwise_join = 'on', max_parallel_workers_per_gather =
> '0', enable_increm
> ental_sort = 'off'
>
>
>
> drop table if exists pagg_tab;
> CREATE TABLE pagg_tab (a text, b int, c text, d int) PARTITION BY
> LIST(c collate "C");
> CREATE TABLE pagg_tab_p1 PARTITION OF pagg_tab FOR VALUES IN ('0000',
> '0001', '0002', '0003', '0004');
> CREATE TABLE pagg_tab_p2 PARTITION OF pagg_tab FOR VALUES IN ('0005',
> '0006', '0007', '0008');
> CREATE TABLE pagg_tab_p3 PARTITION OF pagg_tab FOR VALUES IN ('0009',
> '0010', '0011');
> INSERT INTO pagg_tab SELECT (i % 20)::text, (i % 30), to_char(i % 12,
> 'FM0000'), i % 30 FROM generate_series(0, 2999) i;
> ANALYZE pagg_tab;
> EXPLAIN (COSTS OFF, settings)
> SELECT a, sum(b), avg(b), count(*), max(b) FROM pagg_tab GROUP BY a;
>
>
> QUERY PLAN
>
> -----------------------------------------------------------------------------------------------------------------------------------------------------------
> HashAggregate
> Group Key: pagg_tab.a
> -> Append
> -> Seq Scan on pagg_tab_p1 pagg_tab_1
> -> Seq Scan on pagg_tab_p2 pagg_tab_2
> -> Seq Scan on pagg_tab_p3 pagg_tab_3
> Settings: enable_partitionwise_aggregate = 'on',
> enable_partitionwise_join = 'on', max_parallel_workers_per_gather =
> '0', enable_increm
> ental_sort = 'off'
>
>
> it seems "PARTITION BY LIST(c collate "C");" collation compare with
> "GROUP BY a;".
> set collation_incompatible returned true.
> make it cannot do PARTITIONWISE_AGGREGATE_PARTIAL.
>
> but here "group by a", "a" is text data type, we can still do
> PARTITIONWISE_AGGREGATE_PARTIAL
> ?
Good catch. Looks like I added a bug in group_by_has_partkey() --
collation_incompatible should be set only when a grouping expression
matches a partition key.
--
Thanks, Amit Langote
Attachments:
[application/octet-stream] v5-0002-Disallow-partitionwise-join-when-collation-doesn-.patch (12.2K, ../../CA+HiwqH_XkE2RyYtU_TKfASeV7+q3GLDzM3ssH7pc0eu5Fh-fw@mail.gmail.com/2-v5-0002-Disallow-partitionwise-join-when-collation-doesn-.patch)
download | inline diff:
From f3bd854e7ee04653a162c8980171fb4591813901 Mon Sep 17 00:00:00 2001
From: Amit Langote <[email protected]>
Date: Fri, 1 Nov 2024 15:32:33 +0900
Subject: [PATCH v5 2/2] Disallow partitionwise join when collation doesn't
match
Insist that the collation used for joining matches exactly with the
collation used for partitioning to allow using partitionwise join.
Reported-by: Tender Wang <[email protected]>
Author: Jian He <[email protected]>
Author: Amit Langote <[email protected]>
Reviewed-by: Tender Wang <[email protected]>
Reviewed-by: Junwang Zhao <[email protected]>
Discussion: https://postgr.es/m/[email protected]
Discussion: https://postgr.es/m/[email protected]
Discussion: https://postgr.es/m/CAHewXNno_HKiQ6PqyLYfuqDtwp7KKHZiH1J7Pqyz0nr+PS2Dwg@mail.gmail.com
---
src/backend/optimizer/util/relnode.c | 28 +++-
.../regress/expected/collate.icu.utf8.out | 154 ++++++++++++++++++
src/test/regress/sql/collate.icu.utf8.sql | 35 ++++
3 files changed, 215 insertions(+), 2 deletions(-)
diff --git a/src/backend/optimizer/util/relnode.c b/src/backend/optimizer/util/relnode.c
index d7266e4cdb..489fe6c26f 100644
--- a/src/backend/optimizer/util/relnode.c
+++ b/src/backend/optimizer/util/relnode.c
@@ -2185,6 +2185,10 @@ have_partkey_equi_join(PlannerInfo *root, RelOptInfo *joinrel,
if (pk_known_equal[ipk1])
continue;
+ /* Reject if the partition key collation differs from the clause's. */
+ if (rel1->part_scheme->partcollation[ipk1] != opexpr->inputcollid)
+ return false;
+
/*
* The clause allows partitionwise join only if it uses the same
* operator family as that specified by the partition key.
@@ -2258,6 +2262,8 @@ have_partkey_equi_join(PlannerInfo *root, RelOptInfo *joinrel,
{
Node *expr1 = (Node *) lfirst(lc);
ListCell *lc2;
+ Oid partcoll1 = rel1->part_scheme->partcollation[ipk];
+ Oid exprcoll1 = exprCollation(expr1);
foreach(lc2, rel2->partexprs[ipk])
{
@@ -2265,8 +2271,26 @@ have_partkey_equi_join(PlannerInfo *root, RelOptInfo *joinrel,
if (exprs_known_equal(root, expr1, expr2, btree_opfamily))
{
- pk_known_equal[ipk] = true;
- break;
+ /*
+ * Ensure that the collation of the expression matches
+ * that of the partition key. Checking just one collation
+ * (partcoll1 and exprcoll1) suffices because partcoll1
+ * and partcoll2, as well as exprcoll1 and exprcoll2,
+ * should be identical. This holds because both rel1 and
+ * rel2 use the same PartitionScheme and expr1 and expr2
+ * are equal.
+ */
+ if (partcoll1 == exprcoll1)
+ {
+ Oid partcoll2 PG_USED_FOR_ASSERTS_ONLY =
+ rel1->part_scheme->partcollation[ipk];
+ Oid exprcoll2 PG_USED_FOR_ASSERTS_ONLY =
+ exprCollation(expr2);
+
+ Assert(partcoll2 == exprcoll2);
+ pk_known_equal[ipk] = true;
+ break;
+ }
}
}
if (pk_known_equal[ipk])
diff --git a/src/test/regress/expected/collate.icu.utf8.out b/src/test/regress/expected/collate.icu.utf8.out
index 1ec9a2548b..e6af9b405f 100644
--- a/src/test/regress/expected/collate.icu.utf8.out
+++ b/src/test/regress/expected/collate.icu.utf8.out
@@ -2164,7 +2164,161 @@ SELECT a, count(c collate "C") FROM pagg_tab3 GROUP BY a ORDER BY 1;
4 | 3
(4 rows)
+-- Partitionwise join should not be allowed too when the collation used by the
+-- join keys doesn't match the partition key collation.
+SET enable_partitionwise_join TO false;
+EXPLAIN (COSTS OFF)
+SELECT t1.c, count(t2.c) FROM pagg_tab3 t1 JOIN pagg_tab3 t2 ON t1.c = t2.c GROUP BY 1 ORDER BY 1;
+ QUERY PLAN
+-------------------------------------------------------------
+ Sort
+ Sort Key: t1.c COLLATE case_insensitive
+ -> HashAggregate
+ Group Key: t1.c
+ -> Hash Join
+ Hash Cond: (t1.c = t2.c)
+ -> Append
+ -> Seq Scan on pagg_tab3_p2 t1_1
+ -> Seq Scan on pagg_tab3_p1 t1_2
+ -> Hash
+ -> Append
+ -> Seq Scan on pagg_tab3_p2 t2_1
+ -> Seq Scan on pagg_tab3_p1 t2_2
+(13 rows)
+
+SELECT t1.c, count(t2.c) FROM pagg_tab3 t1 JOIN pagg_tab3 t2 ON t1.c = t2.c GROUP BY 1 ORDER BY 1;
+ c | count
+---+-------
+ A | 36
+ B | 36
+(2 rows)
+
+SET enable_partitionwise_join TO true;
+EXPLAIN (COSTS OFF)
+SELECT t1.c, count(t2.c) FROM pagg_tab3 t1 JOIN pagg_tab3 t2 ON t1.c = t2.c GROUP BY 1 ORDER BY 1;
+ QUERY PLAN
+-------------------------------------------------------------
+ Sort
+ Sort Key: t1.c COLLATE case_insensitive
+ -> HashAggregate
+ Group Key: t1.c
+ -> Hash Join
+ Hash Cond: (t1.c = t2.c)
+ -> Append
+ -> Seq Scan on pagg_tab3_p2 t1_1
+ -> Seq Scan on pagg_tab3_p1 t1_2
+ -> Hash
+ -> Append
+ -> Seq Scan on pagg_tab3_p2 t2_1
+ -> Seq Scan on pagg_tab3_p1 t2_2
+(13 rows)
+
+SELECT t1.c, count(t2.c) FROM pagg_tab3 t1 JOIN pagg_tab3 t2 ON t1.c = t2.c GROUP BY 1 ORDER BY 1;
+ c | count
+---+-------
+ A | 36
+ B | 36
+(2 rows)
+
+-- OK when the join clause uses the same collation as the partition key.
+EXPLAIN (COSTS OFF)
+SELECT t1.c COLLATE "C", count(t2.c) FROM pagg_tab3 t1 JOIN pagg_tab3 t2 ON t1.c = t2.c COLLATE "C" GROUP BY t1.c COLLATE "C" ORDER BY 1;
+ QUERY PLAN
+------------------------------------------------------------------
+ Sort
+ Sort Key: ((t1.c)::text) COLLATE "C"
+ -> Append
+ -> HashAggregate
+ Group Key: (t1.c)::text
+ -> Hash Join
+ Hash Cond: ((t1.c)::text = (t2.c)::text)
+ -> Seq Scan on pagg_tab3_p2 t1
+ -> Hash
+ -> Seq Scan on pagg_tab3_p2 t2
+ -> HashAggregate
+ Group Key: (t1_1.c)::text
+ -> Hash Join
+ Hash Cond: ((t1_1.c)::text = (t2_1.c)::text)
+ -> Seq Scan on pagg_tab3_p1 t1_1
+ -> Hash
+ -> Seq Scan on pagg_tab3_p1 t2_1
+(17 rows)
+
+SELECT t1.c COLLATE "C", count(t2.c) FROM pagg_tab3 t1 JOIN pagg_tab3 t2 ON t1.c = t2.c COLLATE "C" GROUP BY t1.c COLLATE "C" ORDER BY 1;
+ c | count
+---+-------
+ A | 9
+ B | 9
+ a | 9
+ b | 9
+(4 rows)
+
+-- Another case where the partition keys are matched via equivalence class,
+-- not a join restriction clause.
+CREATE TABLE pagg_tab4 (c text collate case_insensitive, b text collate case_insensitive) PARTITION BY LIST(b collate "C");
+CREATE TABLE pagg_tab4_p1 PARTITION OF pagg_tab4 FOR VALUES IN ('a', 'b');
+CREATE TABLE pagg_tab4_p2 PARTITION OF pagg_tab4 FOR VALUES IN ('B', 'A');
+INSERT INTO pagg_tab4 (b, c) SELECT substr('abAB', (i % 4) + 1 , 1), substr('abAB', (i % 2) + 1 , 1) FROM generate_series(0, 11) i;
+ANALYZE pagg_tab4;
+EXPLAIN (COSTS OFF)
+SELECT t1.c, count(t2.c) FROM pagg_tab3 t1 JOIN pagg_tab4 t2 ON t1.c = t2.c AND t1.c = t2.b GROUP BY 1 ORDER BY 1;
+ QUERY PLAN
+-------------------------------------------------------------
+ Sort
+ Sort Key: t1.c COLLATE case_insensitive
+ -> HashAggregate
+ Group Key: t1.c
+ -> Hash Join
+ Hash Cond: (t1.c = t2.c)
+ -> Append
+ -> Seq Scan on pagg_tab3_p2 t1_1
+ -> Seq Scan on pagg_tab3_p1 t1_2
+ -> Hash
+ -> Append
+ -> Seq Scan on pagg_tab4_p2 t2_1
+ Filter: (c = b)
+ -> Seq Scan on pagg_tab4_p1 t2_2
+ Filter: (c = b)
+(15 rows)
+
+SELECT t1.c, count(t2.c) FROM pagg_tab3 t1 JOIN pagg_tab4 t2 ON t1.c = t2.c AND t1.c = t2.b GROUP BY 1 ORDER BY 1;
+ c | count
+---+-------
+ A | 36
+ B | 36
+(2 rows)
+
+-- OK when the join clause uses the same collation as the partition key
+EXPLAIN (COSTS OFF)
+SELECT t1.c, count(t2.c) FROM pagg_tab3 t1 JOIN pagg_tab4 t2 ON t1.c = t2.c AND t1.c = t2.b COLLATE "C" GROUP BY 1 ORDER BY 1;
+ QUERY PLAN
+------------------------------------------------------------------------------------------
+ Sort
+ Sort Key: t1.c COLLATE case_insensitive
+ -> HashAggregate
+ Group Key: t1.c
+ -> Append
+ -> Hash Join
+ Hash Cond: ((t1_1.c = t2_1.c) AND ((t1_1.c)::text = (t2_1.b)::text))
+ -> Seq Scan on pagg_tab3_p2 t1_1
+ -> Hash
+ -> Seq Scan on pagg_tab4_p2 t2_1
+ -> Hash Join
+ Hash Cond: ((t1_2.c = t2_2.c) AND ((t1_2.c)::text = (t2_2.b)::text))
+ -> Seq Scan on pagg_tab3_p1 t1_2
+ -> Hash
+ -> Seq Scan on pagg_tab4_p1 t2_2
+(15 rows)
+
+SELECT t1.c, count(t2.c) FROM pagg_tab3 t1 JOIN pagg_tab4 t2 ON t1.c = t2.c AND t1.c = t2.b COLLATE "C" GROUP BY 1 ORDER BY 1;
+ c | count
+---+-------
+ A | 18
+ B | 18
+(2 rows)
+
DROP TABLE pagg_tab3;
+DROP TABLE pagg_tab4;
RESET enable_partitionwise_aggregate;
RESET max_parallel_workers_per_gather;
RESET enable_incremental_sort;
diff --git a/src/test/regress/sql/collate.icu.utf8.sql b/src/test/regress/sql/collate.icu.utf8.sql
index 792bc94361..407e5e5b21 100644
--- a/src/test/regress/sql/collate.icu.utf8.sql
+++ b/src/test/regress/sql/collate.icu.utf8.sql
@@ -834,7 +834,42 @@ EXPLAIN (COSTS OFF)
SELECT a, count(c collate "C") FROM pagg_tab3 GROUP BY a ORDER BY 1;
SELECT a, count(c collate "C") FROM pagg_tab3 GROUP BY a ORDER BY 1;
+-- Partitionwise join should not be allowed too when the collation used by the
+-- join keys doesn't match the partition key collation.
+SET enable_partitionwise_join TO false;
+EXPLAIN (COSTS OFF)
+SELECT t1.c, count(t2.c) FROM pagg_tab3 t1 JOIN pagg_tab3 t2 ON t1.c = t2.c GROUP BY 1 ORDER BY 1;
+SELECT t1.c, count(t2.c) FROM pagg_tab3 t1 JOIN pagg_tab3 t2 ON t1.c = t2.c GROUP BY 1 ORDER BY 1;
+
+SET enable_partitionwise_join TO true;
+EXPLAIN (COSTS OFF)
+SELECT t1.c, count(t2.c) FROM pagg_tab3 t1 JOIN pagg_tab3 t2 ON t1.c = t2.c GROUP BY 1 ORDER BY 1;
+SELECT t1.c, count(t2.c) FROM pagg_tab3 t1 JOIN pagg_tab3 t2 ON t1.c = t2.c GROUP BY 1 ORDER BY 1;
+
+-- OK when the join clause uses the same collation as the partition key.
+EXPLAIN (COSTS OFF)
+SELECT t1.c COLLATE "C", count(t2.c) FROM pagg_tab3 t1 JOIN pagg_tab3 t2 ON t1.c = t2.c COLLATE "C" GROUP BY t1.c COLLATE "C" ORDER BY 1;
+SELECT t1.c COLLATE "C", count(t2.c) FROM pagg_tab3 t1 JOIN pagg_tab3 t2 ON t1.c = t2.c COLLATE "C" GROUP BY t1.c COLLATE "C" ORDER BY 1;
+
+-- Another case where the partition keys are matched via equivalence class,
+-- not a join restriction clause.
+CREATE TABLE pagg_tab4 (c text collate case_insensitive, b text collate case_insensitive) PARTITION BY LIST(b collate "C");
+CREATE TABLE pagg_tab4_p1 PARTITION OF pagg_tab4 FOR VALUES IN ('a', 'b');
+CREATE TABLE pagg_tab4_p2 PARTITION OF pagg_tab4 FOR VALUES IN ('B', 'A');
+INSERT INTO pagg_tab4 (b, c) SELECT substr('abAB', (i % 4) + 1 , 1), substr('abAB', (i % 2) + 1 , 1) FROM generate_series(0, 11) i;
+ANALYZE pagg_tab4;
+
+EXPLAIN (COSTS OFF)
+SELECT t1.c, count(t2.c) FROM pagg_tab3 t1 JOIN pagg_tab4 t2 ON t1.c = t2.c AND t1.c = t2.b GROUP BY 1 ORDER BY 1;
+SELECT t1.c, count(t2.c) FROM pagg_tab3 t1 JOIN pagg_tab4 t2 ON t1.c = t2.c AND t1.c = t2.b GROUP BY 1 ORDER BY 1;
+
+-- OK when the join clause uses the same collation as the partition key
+EXPLAIN (COSTS OFF)
+SELECT t1.c, count(t2.c) FROM pagg_tab3 t1 JOIN pagg_tab4 t2 ON t1.c = t2.c AND t1.c = t2.b COLLATE "C" GROUP BY 1 ORDER BY 1;
+SELECT t1.c, count(t2.c) FROM pagg_tab3 t1 JOIN pagg_tab4 t2 ON t1.c = t2.c AND t1.c = t2.b COLLATE "C" GROUP BY 1 ORDER BY 1;
+
DROP TABLE pagg_tab3;
+DROP TABLE pagg_tab4;
RESET enable_partitionwise_aggregate;
RESET max_parallel_workers_per_gather;
--
2.43.0
[application/octet-stream] v5-0001-Disallow-partitionwise-grouping-when-collation-do.patch (12.3K, ../../CA+HiwqH_XkE2RyYtU_TKfASeV7+q3GLDzM3ssH7pc0eu5Fh-fw@mail.gmail.com/3-v5-0001-Disallow-partitionwise-grouping-when-collation-do.patch)
download | inline diff:
From 9ac55edd5118bed524c1671421cb81fa705e9920 Mon Sep 17 00:00:00 2001
From: Amit Langote <[email protected]>
Date: Fri, 1 Nov 2024 16:15:50 +0900
Subject: [PATCH v5 1/2] Disallow partitionwise grouping when collation doesn't
match
Insist that the collation used for grouping matches exactly with the
collation used for partitioning to allow using either full or
partial partitionwise grouping.
Bug: #18568
Reported-by: Webbo Han <[email protected]>
Author: Webbo Han <[email protected]>
Reviewed-by: Tender Wang <[email protected]>
Reviewed-by: Aleksander Alekseev <[email protected]>
Reviewed-by: Jian He <[email protected]>
Discussion: https://postgr.es/m/[email protected]
Discussion: https://postgr.es/m/[email protected]
Discussion: https://postgr.es/m/CAHewXNno_HKiQ6PqyLYfuqDtwp7KKHZiH1J7Pqyz0nr+PS2Dwg@mail.gmail.com
---
src/backend/optimizer/plan/planner.c | 67 +++++++---
.../regress/expected/collate.icu.utf8.out | 114 ++++++++++++++++++
src/test/regress/sql/collate.icu.utf8.sql | 43 +++++++
3 files changed, 210 insertions(+), 14 deletions(-)
diff --git a/src/backend/optimizer/plan/planner.c b/src/backend/optimizer/plan/planner.c
index 0f423e9684..24a3ff087e 100644
--- a/src/backend/optimizer/plan/planner.c
+++ b/src/backend/optimizer/plan/planner.c
@@ -253,7 +253,8 @@ static void create_partitionwise_grouping_paths(PlannerInfo *root,
GroupPathExtraData *extra);
static bool group_by_has_partkey(RelOptInfo *input_rel,
List *targetList,
- List *groupClause);
+ List *groupClause,
+ bool *collation_incompatible);
static int common_prefix_cmp(const void *a, const void *b);
static List *generate_setop_child_grouplist(SetOperationStmt *op,
List *targetlist);
@@ -4090,23 +4091,30 @@ create_ordinary_grouping_paths(PlannerInfo *root, RelOptInfo *input_rel,
if (extra->patype != PARTITIONWISE_AGGREGATE_NONE &&
IS_PARTITIONED_REL(input_rel))
{
+ bool collation_incompatible = false;
+ bool group_by_contains_partkey =
+ group_by_has_partkey(input_rel, extra->targetList,
+ root->parse->groupClause,
+ &collation_incompatible);
+
/*
* If this is the topmost relation or if the parent relation is doing
* full partitionwise aggregation, then we can do full partitionwise
* aggregation provided that the GROUP BY clause contains all of the
- * partitioning columns at this level. Otherwise, we can do at most
- * partial partitionwise aggregation. But if partial aggregation is
- * not supported in general then we can't use it for partitionwise
- * aggregation either.
+ * partitioning columns at this level and the collation used by GROUP
+ * BY matches the partitioning collation. Otherwise, we can do at most
+ * partial partitionwise aggregation, but again only if the collation
+ * is compatible. If partial aggregation is not supported in general
+ * then we can't use it for partitionwise aggregation either.
*
* Check parse->groupClause not processed_groupClause, because it's
* okay if some of the partitioning columns were proved redundant.
*/
if (extra->patype == PARTITIONWISE_AGGREGATE_FULL &&
- group_by_has_partkey(input_rel, extra->targetList,
- root->parse->groupClause))
+ group_by_contains_partkey)
patype = PARTITIONWISE_AGGREGATE_FULL;
- else if ((extra->flags & GROUPING_CAN_PARTIAL_AGG) != 0)
+ else if ((extra->flags & GROUPING_CAN_PARTIAL_AGG) != 0 &&
+ !collation_incompatible)
patype = PARTITIONWISE_AGGREGATE_PARTIAL;
else
patype = PARTITIONWISE_AGGREGATE_NONE;
@@ -8105,13 +8113,18 @@ create_partitionwise_grouping_paths(PlannerInfo *root,
/*
* group_by_has_partkey
*
- * Returns true, if all the partition keys of the given relation are part of
- * the GROUP BY clauses, false otherwise.
+ * Returns true if all the partition keys of the given relation are part of
+ * the GROUP BY clauses, including having matching collation, false otherwise.
+ *
+ * Returns false also if a collation mismatch is detected between a partition
+ * key and its corresponding expression in groupClause, in which case.
+ * *collation_incompatible is set to true.
*/
static bool
group_by_has_partkey(RelOptInfo *input_rel,
List *targetList,
- List *groupClause)
+ List *groupClause,
+ bool *collation_incompatible)
{
List *groupexprs = get_sortgrouplist_exprs(groupClause, targetList);
int cnt = 0;
@@ -8134,13 +8147,39 @@ 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 groupcoll = exprCollation((Node *) groupexpr);
+
+ if (IsA(groupexpr, RelabelType))
+ groupexpr = ((RelabelType *) groupexpr)->arg;
+
+ if (equal(groupexpr, partexpr))
+ {
+
+ /*
+ * Reject a match if the grouping collation does not match
+ * the partitioning collation.
+ */
+ if (OidIsValid(partcoll) && OidIsValid(groupcoll) &&
+ partcoll != groupcoll)
+ {
+ *collation_incompatible = true;
+ return false;
+ }
+
+ found = true;
+ break;
+ }
}
+
+ if (found)
+ break;
}
/*
diff --git a/src/test/regress/expected/collate.icu.utf8.out b/src/test/regress/expected/collate.icu.utf8.out
index faa376e060..1ec9a2548b 100644
--- a/src/test/regress/expected/collate.icu.utf8.out
+++ b/src/test/regress/expected/collate.icu.utf8.out
@@ -2054,6 +2054,120 @@ SELECT (SELECT count(*) FROM test33_0) <> (SELECT count(*) FROM test33_1);
t
(1 row)
+--
+-- Bug #18568
+--
+-- Partitionwise aggregate (full or partial) should not be used when a
+-- partition key's collation doesn't match that of the GROUP BY column it is
+-- matched with.
+SET max_parallel_workers_per_gather TO 0;
+SET enable_incremental_sort TO off;
+CREATE TABLE pagg_tab3 (a text, 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');
+CREATE TABLE pagg_tab3_p2 PARTITION OF pagg_tab3 FOR VALUES IN ('B', 'A');
+INSERT INTO pagg_tab3 SELECT i % 4 + 1, substr('abAB', (i % 4) + 1 , 1) FROM generate_series(0, 11) i;
+ANALYZE pagg_tab3;
+SET enable_partitionwise_aggregate TO false;
+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
+ -> HashAggregate
+ Group Key: pagg_tab3.c
+ -> Append
+ -> Seq Scan on pagg_tab3_p2 pagg_tab3_1
+ -> Seq Scan on pagg_tab3_p1 pagg_tab3_2
+(7 rows)
+
+SELECT upper(c collate case_insensitive), count(c) FROM pagg_tab3 GROUP BY c collate case_insensitive ORDER BY 1;
+ upper | count
+-------+-------
+ A | 6
+ B | 6
+(2 rows)
+
+-- No partitionwise aggregation allowed!
+SET enable_partitionwise_aggregate TO true;
+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
+ -> HashAggregate
+ Group Key: pagg_tab3.c
+ -> Append
+ -> Seq Scan on pagg_tab3_p2 pagg_tab3_1
+ -> Seq Scan on pagg_tab3_p1 pagg_tab3_2
+(7 rows)
+
+SELECT upper(c collate case_insensitive), count(c) FROM pagg_tab3 GROUP BY c collate case_insensitive ORDER BY 1;
+ upper | count
+-------+-------
+ A | 6
+ B | 6
+(2 rows)
+
+-- OK to use full partitionwise aggregate after changing the GROUP BY column's
+-- collation to be the same as that of the partition key.
+EXPLAIN (COSTS OFF)
+SELECT c collate "C", count(c) FROM pagg_tab3 GROUP BY c collate "C" ORDER BY 1;
+ QUERY PLAN
+--------------------------------------------------------
+ Sort
+ Sort Key: ((pagg_tab3.c)::text) COLLATE "C"
+ -> Append
+ -> HashAggregate
+ Group Key: (pagg_tab3.c)::text
+ -> Seq Scan on pagg_tab3_p2 pagg_tab3
+ -> HashAggregate
+ Group Key: (pagg_tab3_1.c)::text
+ -> Seq Scan on pagg_tab3_p1 pagg_tab3_1
+(9 rows)
+
+SELECT c collate "C", count(c) FROM pagg_tab3 GROUP BY c collate "C" ORDER BY 1;
+ c | count
+---+-------
+ A | 3
+ B | 3
+ a | 3
+ b | 3
+(4 rows)
+
+-- Also OK to use partial partitionwise aggregate when grouping columns do not
+-- include partition key columns
+EXPLAIN (COSTS OFF)
+SELECT a, count(c collate "C") FROM pagg_tab3 GROUP BY a ORDER BY 1;
+ QUERY PLAN
+--------------------------------------------------------------
+ Finalize GroupAggregate
+ Group Key: pagg_tab3.a
+ -> Sort
+ Sort Key: pagg_tab3.a
+ -> Append
+ -> Partial HashAggregate
+ Group Key: pagg_tab3.a
+ -> Seq Scan on pagg_tab3_p2 pagg_tab3
+ -> Partial HashAggregate
+ Group Key: pagg_tab3_1.a
+ -> Seq Scan on pagg_tab3_p1 pagg_tab3_1
+(11 rows)
+
+SELECT a, count(c collate "C") FROM pagg_tab3 GROUP BY a ORDER BY 1;
+ a | count
+---+-------
+ 1 | 3
+ 2 | 3
+ 3 | 3
+ 4 | 3
+(4 rows)
+
+DROP TABLE pagg_tab3;
+RESET enable_partitionwise_aggregate;
+RESET max_parallel_workers_per_gather;
+RESET enable_incremental_sort;
-- cleanup
RESET search_path;
SET client_min_messages TO warning;
diff --git a/src/test/regress/sql/collate.icu.utf8.sql b/src/test/regress/sql/collate.icu.utf8.sql
index 80f28a97d7..792bc94361 100644
--- a/src/test/regress/sql/collate.icu.utf8.sql
+++ b/src/test/regress/sql/collate.icu.utf8.sql
@@ -796,6 +796,49 @@ INSERT INTO test33 VALUES (2, 'DEF');
-- they end up in the same partition (but it's platform-dependent which one)
SELECT (SELECT count(*) FROM test33_0) <> (SELECT count(*) FROM test33_1);
+--
+-- Bug #18568
+--
+-- Partitionwise aggregate (full or partial) should not be used when a
+-- partition key's collation doesn't match that of the GROUP BY column it is
+-- matched with.
+SET max_parallel_workers_per_gather TO 0;
+SET enable_incremental_sort TO off;
+
+CREATE TABLE pagg_tab3 (a text, 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');
+CREATE TABLE pagg_tab3_p2 PARTITION OF pagg_tab3 FOR VALUES IN ('B', 'A');
+INSERT INTO pagg_tab3 SELECT i % 4 + 1, substr('abAB', (i % 4) + 1 , 1) FROM generate_series(0, 11) i;
+ANALYZE pagg_tab3;
+
+SET enable_partitionwise_aggregate TO false;
+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;
+
+-- No partitionwise aggregation allowed!
+SET enable_partitionwise_aggregate TO true;
+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;
+
+-- OK to use full partitionwise aggregate after changing the GROUP BY column's
+-- collation to be the same as that of the partition key.
+EXPLAIN (COSTS OFF)
+SELECT c collate "C", count(c) FROM pagg_tab3 GROUP BY c collate "C" ORDER BY 1;
+SELECT c collate "C", count(c) FROM pagg_tab3 GROUP BY c collate "C" ORDER BY 1;
+
+-- Also OK to use partial partitionwise aggregate when grouping columns do not
+-- include partition key columns
+EXPLAIN (COSTS OFF)
+SELECT a, count(c collate "C") FROM pagg_tab3 GROUP BY a ORDER BY 1;
+SELECT a, count(c collate "C") FROM pagg_tab3 GROUP BY a ORDER BY 1;
+
+DROP TABLE pagg_tab3;
+
+RESET enable_partitionwise_aggregate;
+RESET max_parallel_workers_per_gather;
+RESET enable_incremental_sort;
-- cleanup
RESET search_path;
--
2.43.0
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: Wrong result when enable_partitionwise_join is on if collation of PartitionKey and Column is different.
2024-10-30 03:57 Re: Wrong result when enable_partitionwise_join is on if collation of PartitionKey and Column is different. jian he <[email protected]>
2024-10-30 12:36 ` Re: Wrong result when enable_partitionwise_join is on if collation of PartitionKey and Column is different. Junwang Zhao <[email protected]>
2024-10-31 13:08 ` Re: Wrong result when enable_partitionwise_join is on if collation of PartitionKey and Column is different. Amit Langote <[email protected]>
2024-11-01 05:39 ` Re: Wrong result when enable_partitionwise_join is on if collation of PartitionKey and Column is different. jian he <[email protected]>
2024-11-01 07:25 ` Re: Wrong result when enable_partitionwise_join is on if collation of PartitionKey and Column is different. Amit Langote <[email protected]>
2024-11-01 08:07 ` Re: Wrong result when enable_partitionwise_join is on if collation of PartitionKey and Column is different. jian he <[email protected]>
2024-11-01 10:08 ` Re: Wrong result when enable_partitionwise_join is on if collation of PartitionKey and Column is different. Amit Langote <[email protected]>
@ 2024-11-04 03:27 ` jian he <[email protected]>
2024-11-05 07:55 ` Re: Wrong result when enable_partitionwise_join is on if collation of PartitionKey and Column is different. Amit Langote <[email protected]>
0 siblings, 1 reply; 15+ messages in thread
From: jian he @ 2024-11-04 03:27 UTC (permalink / raw)
To: Amit Langote <[email protected]>; +Cc: Junwang Zhao <[email protected]>; Tender Wang <[email protected]>; PostgreSQL Hackers <[email protected]>; Ashutosh Bapat <[email protected]>
hi.
about v5.
if (exprs_known_equal(root, expr1, expr2, btree_opfamily))
{
/*
* Ensure that the collation of the expression matches
* that of the partition key. Checking just one collation
* (partcoll1 and exprcoll1) suffices because partcoll1
* and partcoll2, as well as exprcoll1 and exprcoll2,
* should be identical. This holds because both rel1 and
* rel2 use the same PartitionScheme and expr1 and expr2
* are equal.
*/
if (partcoll1 == exprcoll1)
{
Oid partcoll2 PG_USED_FOR_ASSERTS_ONLY =
rel1->part_scheme->partcollation[ipk];
Oid exprcoll2 PG_USED_FOR_ASSERTS_ONLY =
exprCollation(expr2);
Assert(partcoll2 == exprcoll2);
pk_known_equal[ipk] = true;
if (OidIsValid(exprcoll1))
elog(INFO, "this path called %s:%d",
__FILE_NAME__, __LINE__);
break;
}
}
tests still passed, which means that we didn't have text data type as
partition key related tests for partition-wise join.
Do we need to add one?
+-- Another case where the partition keys are matched via equivalence class,
+-- not a join restriction clause.
+
+-- OK when the join clause uses the same collation as the partition key
+EXPLAIN (COSTS OFF)
+SELECT t1.c, count(t2.c) FROM pagg_tab3 t1 JOIN pagg_tab4 t2 ON t1.c
= t2.c AND t1.c = t2.b COLLATE "C" GROUP BY 1 ORDER BY 1;
i suppose, you comments is saying that in have_partkey_equi_join
the above query will return true via
`if (exprs_known_equal(root, expr1, expr2, btree_opfamily))`
But " t1.c = t2.b COLLATE "C" already in "restrictlist".
In have_partkey_equi_join loop through "restrictlist" would return
true for above query, won't reach exprs_known_equal.
Other than the comments that confused me, the test and the results
look fine to me.
some column collation is case_insensitive, ORDER BY that column would
render the output not deterministic.
like 'A' before 'a' and 'a' before 'A' are both correct.
it may cause regress tests to fail.
So I did some minor refactoring to make the "ORDER BY" deterministic.
Attachments:
[application/octet-stream] v5-0001-make-partition-wise-partitoin-aggreagte-relate.no-cfbot (6.5K, ../../CACJufxHAnGh75sp6Df=jrOiZifHUJSJq_d4XhEhw=vULC66U+A@mail.gmail.com/2-v5-0001-make-partition-wise-partitoin-aggreagte-relate.no-cfbot)
download
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: Wrong result when enable_partitionwise_join is on if collation of PartitionKey and Column is different.
2024-10-30 03:57 Re: Wrong result when enable_partitionwise_join is on if collation of PartitionKey and Column is different. jian he <[email protected]>
2024-10-30 12:36 ` Re: Wrong result when enable_partitionwise_join is on if collation of PartitionKey and Column is different. Junwang Zhao <[email protected]>
2024-10-31 13:08 ` Re: Wrong result when enable_partitionwise_join is on if collation of PartitionKey and Column is different. Amit Langote <[email protected]>
2024-11-01 05:39 ` Re: Wrong result when enable_partitionwise_join is on if collation of PartitionKey and Column is different. jian he <[email protected]>
2024-11-01 07:25 ` Re: Wrong result when enable_partitionwise_join is on if collation of PartitionKey and Column is different. Amit Langote <[email protected]>
2024-11-01 08:07 ` Re: Wrong result when enable_partitionwise_join is on if collation of PartitionKey and Column is different. jian he <[email protected]>
2024-11-01 10:08 ` Re: Wrong result when enable_partitionwise_join is on if collation of PartitionKey and Column is different. Amit Langote <[email protected]>
2024-11-04 03:27 ` Re: Wrong result when enable_partitionwise_join is on if collation of PartitionKey and Column is different. jian he <[email protected]>
@ 2024-11-05 07:55 ` Amit Langote <[email protected]>
2024-11-06 03:19 ` Re: Wrong result when enable_partitionwise_join is on if collation of PartitionKey and Column is different. jian he <[email protected]>
0 siblings, 1 reply; 15+ messages in thread
From: Amit Langote @ 2024-11-05 07:55 UTC (permalink / raw)
To: jian he <[email protected]>; +Cc: Junwang Zhao <[email protected]>; Tender Wang <[email protected]>; PostgreSQL Hackers <[email protected]>; Ashutosh Bapat <[email protected]>
Hi,
On Mon, Nov 4, 2024 at 12:28 PM jian he <[email protected]> wrote:
>
> hi.
> about v5.
> if (exprs_known_equal(root, expr1, expr2, btree_opfamily))
> {
> /*
> * Ensure that the collation of the expression matches
> * that of the partition key. Checking just one collation
> * (partcoll1 and exprcoll1) suffices because partcoll1
> * and partcoll2, as well as exprcoll1 and exprcoll2,
> * should be identical. This holds because both rel1 and
> * rel2 use the same PartitionScheme and expr1 and expr2
> * are equal.
> */
> if (partcoll1 == exprcoll1)
> {
> Oid partcoll2 PG_USED_FOR_ASSERTS_ONLY =
> rel1->part_scheme->partcollation[ipk];
> Oid exprcoll2 PG_USED_FOR_ASSERTS_ONLY =
> exprCollation(expr2);
> Assert(partcoll2 == exprcoll2);
> pk_known_equal[ipk] = true;
> if (OidIsValid(exprcoll1))
> elog(INFO, "this path called %s:%d",
> __FILE_NAME__, __LINE__);
> break;
> }
> }
>
> tests still passed, which means that we didn't have text data type as
> partition key related tests for partition-wise join.
> Do we need to add one?
>
> +-- Another case where the partition keys are matched via equivalence class,
> +-- not a join restriction clause.
> +
> +-- OK when the join clause uses the same collation as the partition key
> +EXPLAIN (COSTS OFF)
> +SELECT t1.c, count(t2.c) FROM pagg_tab3 t1 JOIN pagg_tab4 t2 ON t1.c
> = t2.c AND t1.c = t2.b COLLATE "C" GROUP BY 1 ORDER BY 1;
>
> i suppose, you comments is saying that in have_partkey_equi_join
> the above query will return true via
> `if (exprs_known_equal(root, expr1, expr2, btree_opfamily))`
> But " t1.c = t2.b COLLATE "C" already in "restrictlist".
> In have_partkey_equi_join loop through "restrictlist" would return
> true for above query, won't reach exprs_known_equal.
>
> Other than the comments that confused me, the test and the results
> look fine to me.
Thanks, yes, a test case that exercises the partcoll1 == exprcoll1
code was missing.
> some column collation is case_insensitive, ORDER BY that column would
> render the output not deterministic.
> like 'A' before 'a' and 'a' before 'A' are both correct.
> it may cause regress tests to fail.
> So I did some minor refactoring to make the "ORDER BY" deterministic.
Thanks, merged.
--
Thanks, Amit Langote
Attachments:
[application/octet-stream] v6-0002-Disallow-partitionwise-join-when-collation-doesn-.patch (16.7K, ../../CA+HiwqHv+7gp+SPfE-=+KYmvrcVWryz+F5HNG2Rb6=UZL_A+OQ@mail.gmail.com/2-v6-0002-Disallow-partitionwise-join-when-collation-doesn-.patch)
download | inline diff:
From 36274d54a2367ef2c249ac4c4d2762387d36175e Mon Sep 17 00:00:00 2001
From: Amit Langote <[email protected]>
Date: Fri, 1 Nov 2024 15:32:33 +0900
Subject: [PATCH v6 2/2] Disallow partitionwise join when collation doesn't
match
Insist that the collation used for joining matches exactly with the
collation used for partitioning to allow using partitionwise join.
Reported-by: Tender Wang <[email protected]>
Author: Jian He <[email protected]>
Author: Amit Langote <[email protected]>
Reviewed-by: Tender Wang <[email protected]>
Reviewed-by: Junwang Zhao <[email protected]>
Discussion: https://postgr.es/m/[email protected]
Discussion: https://postgr.es/m/[email protected]
Discussion: https://postgr.es/m/CAHewXNno_HKiQ6PqyLYfuqDtwp7KKHZiH1J7Pqyz0nr+PS2Dwg@mail.gmail.com
---
src/backend/optimizer/util/relnode.c | 28 ++-
.../regress/expected/collate.icu.utf8.out | 209 ++++++++++++++++++
src/test/regress/sql/collate.icu.utf8.sql | 58 +++++
3 files changed, 293 insertions(+), 2 deletions(-)
diff --git a/src/backend/optimizer/util/relnode.c b/src/backend/optimizer/util/relnode.c
index d7266e4cdb..489fe6c26f 100644
--- a/src/backend/optimizer/util/relnode.c
+++ b/src/backend/optimizer/util/relnode.c
@@ -2185,6 +2185,10 @@ have_partkey_equi_join(PlannerInfo *root, RelOptInfo *joinrel,
if (pk_known_equal[ipk1])
continue;
+ /* Reject if the partition key collation differs from the clause's. */
+ if (rel1->part_scheme->partcollation[ipk1] != opexpr->inputcollid)
+ return false;
+
/*
* The clause allows partitionwise join only if it uses the same
* operator family as that specified by the partition key.
@@ -2258,6 +2262,8 @@ have_partkey_equi_join(PlannerInfo *root, RelOptInfo *joinrel,
{
Node *expr1 = (Node *) lfirst(lc);
ListCell *lc2;
+ Oid partcoll1 = rel1->part_scheme->partcollation[ipk];
+ Oid exprcoll1 = exprCollation(expr1);
foreach(lc2, rel2->partexprs[ipk])
{
@@ -2265,8 +2271,26 @@ have_partkey_equi_join(PlannerInfo *root, RelOptInfo *joinrel,
if (exprs_known_equal(root, expr1, expr2, btree_opfamily))
{
- pk_known_equal[ipk] = true;
- break;
+ /*
+ * Ensure that the collation of the expression matches
+ * that of the partition key. Checking just one collation
+ * (partcoll1 and exprcoll1) suffices because partcoll1
+ * and partcoll2, as well as exprcoll1 and exprcoll2,
+ * should be identical. This holds because both rel1 and
+ * rel2 use the same PartitionScheme and expr1 and expr2
+ * are equal.
+ */
+ if (partcoll1 == exprcoll1)
+ {
+ Oid partcoll2 PG_USED_FOR_ASSERTS_ONLY =
+ rel1->part_scheme->partcollation[ipk];
+ Oid exprcoll2 PG_USED_FOR_ASSERTS_ONLY =
+ exprCollation(expr2);
+
+ Assert(partcoll2 == exprcoll2);
+ pk_known_equal[ipk] = true;
+ break;
+ }
}
}
if (pk_known_equal[ipk])
diff --git a/src/test/regress/expected/collate.icu.utf8.out b/src/test/regress/expected/collate.icu.utf8.out
index a4cd9ea085..e025c2518c 100644
--- a/src/test/regress/expected/collate.icu.utf8.out
+++ b/src/test/regress/expected/collate.icu.utf8.out
@@ -2164,7 +2164,216 @@ SELECT a collate "C", count(c collate "C") FROM pagg_tab3 GROUP BY a collate "C"
4 | 3
(4 rows)
+-- Partitionwise join should not be allowed too when the collation used by the
+-- join keys doesn't match the partition key collation.
+SET enable_partitionwise_join TO false;
+EXPLAIN (COSTS OFF)
+SELECT t1.c, count(t2.c) FROM pagg_tab3 t1 JOIN pagg_tab3 t2 ON t1.c = t2.c GROUP BY 1 ORDER BY t1.c COLLATE "C";
+ QUERY PLAN
+-------------------------------------------------------------
+ Sort
+ Sort Key: t1.c COLLATE "C"
+ -> HashAggregate
+ Group Key: t1.c
+ -> Hash Join
+ Hash Cond: (t1.c = t2.c)
+ -> Append
+ -> Seq Scan on pagg_tab3_p2 t1_1
+ -> Seq Scan on pagg_tab3_p1 t1_2
+ -> Hash
+ -> Append
+ -> Seq Scan on pagg_tab3_p2 t2_1
+ -> Seq Scan on pagg_tab3_p1 t2_2
+(13 rows)
+
+SELECT t1.c, count(t2.c) FROM pagg_tab3 t1 JOIN pagg_tab3 t2 ON t1.c = t2.c GROUP BY 1 ORDER BY t1.c COLLATE "C";
+ c | count
+---+-------
+ A | 36
+ B | 36
+(2 rows)
+
+SET enable_partitionwise_join TO true;
+EXPLAIN (COSTS OFF)
+SELECT t1.c, count(t2.c) FROM pagg_tab3 t1 JOIN pagg_tab3 t2 ON t1.c = t2.c GROUP BY 1 ORDER BY t1.c COLLATE "C";
+ QUERY PLAN
+-------------------------------------------------------------
+ Sort
+ Sort Key: t1.c COLLATE "C"
+ -> HashAggregate
+ Group Key: t1.c
+ -> Hash Join
+ Hash Cond: (t1.c = t2.c)
+ -> Append
+ -> Seq Scan on pagg_tab3_p2 t1_1
+ -> Seq Scan on pagg_tab3_p1 t1_2
+ -> Hash
+ -> Append
+ -> Seq Scan on pagg_tab3_p2 t2_1
+ -> Seq Scan on pagg_tab3_p1 t2_2
+(13 rows)
+
+SELECT t1.c, count(t2.c) FROM pagg_tab3 t1 JOIN pagg_tab3 t2 ON t1.c = t2.c GROUP BY 1 ORDER BY t1.c COLLATE "C";
+ c | count
+---+-------
+ A | 36
+ B | 36
+(2 rows)
+
+-- OK when the join clause uses the same collation as the partition key.
+EXPLAIN (COSTS OFF)
+SELECT t1.c COLLATE "C", count(t2.c) FROM pagg_tab3 t1 JOIN pagg_tab3 t2 ON t1.c = t2.c COLLATE "C" GROUP BY t1.c COLLATE "C" ORDER BY t1.c COLLATE "C";
+ QUERY PLAN
+------------------------------------------------------------------
+ Sort
+ Sort Key: ((t1.c)::text) COLLATE "C"
+ -> Append
+ -> HashAggregate
+ Group Key: (t1.c)::text
+ -> Hash Join
+ Hash Cond: ((t1.c)::text = (t2.c)::text)
+ -> Seq Scan on pagg_tab3_p2 t1
+ -> Hash
+ -> Seq Scan on pagg_tab3_p2 t2
+ -> HashAggregate
+ Group Key: (t1_1.c)::text
+ -> Hash Join
+ Hash Cond: ((t1_1.c)::text = (t2_1.c)::text)
+ -> Seq Scan on pagg_tab3_p1 t1_1
+ -> Hash
+ -> Seq Scan on pagg_tab3_p1 t2_1
+(17 rows)
+
+SELECT t1.c COLLATE "C", count(t2.c) FROM pagg_tab3 t1 JOIN pagg_tab3 t2 ON t1.c = t2.c COLLATE "C" GROUP BY t1.c COLLATE "C" ORDER BY t1.c COLLATE "C";
+ c | count
+---+-------
+ A | 9
+ B | 9
+ a | 9
+ b | 9
+(4 rows)
+
+-- Few other cases where the joined partition keys are matched via equivalence
+-- class, not a join restriction clause.
+-- Collations of joined columns match, but the partition keys collation is different
+CREATE TABLE pagg_tab4 (c text collate case_insensitive, b text collate case_insensitive) PARTITION BY LIST (b collate "C");
+CREATE TABLE pagg_tab4_p1 PARTITION OF pagg_tab4 FOR VALUES IN ('a', 'b');
+CREATE TABLE pagg_tab4_p2 PARTITION OF pagg_tab4 FOR VALUES IN ('B', 'A');
+INSERT INTO pagg_tab4 (b, c) SELECT substr('abAB', (i % 4) + 1 , 1), substr('abAB', (i % 2) + 1 , 1) FROM generate_series(0, 11) i;
+ANALYZE pagg_tab4;
+EXPLAIN (COSTS OFF)
+SELECT t1.c, count(t2.c) FROM pagg_tab3 t1 JOIN pagg_tab4 t2 ON t1.c = t2.c AND t1.c = t2.b GROUP BY 1 ORDER BY t1.c COLLATE "C";
+ QUERY PLAN
+-------------------------------------------------------------
+ Sort
+ Sort Key: t1.c COLLATE "C"
+ -> HashAggregate
+ Group Key: t1.c
+ -> Hash Join
+ Hash Cond: (t1.c = t2.c)
+ -> Append
+ -> Seq Scan on pagg_tab3_p2 t1_1
+ -> Seq Scan on pagg_tab3_p1 t1_2
+ -> Hash
+ -> Append
+ -> Seq Scan on pagg_tab4_p2 t2_1
+ Filter: (c = b)
+ -> Seq Scan on pagg_tab4_p1 t2_2
+ Filter: (c = b)
+(15 rows)
+
+SELECT t1.c, count(t2.c) FROM pagg_tab3 t1 JOIN pagg_tab4 t2 ON t1.c = t2.c AND t1.c = t2.b GROUP BY 1 ORDER BY t1.c COLLATE "C";
+ c | count
+---+-------
+ A | 36
+ B | 36
+(2 rows)
+
+-- OK when the partition key collation is same as that of the join columns
+CREATE TABLE pagg_tab5 (c text collate case_insensitive, b text collate case_insensitive) PARTITION BY LIST (c collate case_insensitive);
+CREATE TABLE pagg_tab5_p1 PARTITION OF pagg_tab5 FOR VALUES IN ('a', 'b');
+CREATE TABLE pagg_tab5_p2 PARTITION OF pagg_tab5 FOR VALUES IN ('c', 'd');
+INSERT INTO pagg_tab5 (b, c) SELECT substr('abAB', (i % 4) + 1 , 1), substr('abAB', (i % 2) + 1 , 1) FROM generate_series(0, 5) i;
+INSERT INTO pagg_tab5 (b, c) SELECT substr('cdCD', (i % 4) + 1 , 1), substr('cdCD', (i % 2) + 1 , 1) FROM generate_series(0, 5) i;
+ANALYZE pagg_tab5;
+CREATE TABLE pagg_tab6 (c text collate case_insensitive, b text collate case_insensitive) PARTITION BY LIST (b collate case_insensitive);
+CREATE TABLE pagg_tab6_p1 PARTITION OF pagg_tab6 FOR VALUES IN ('a', 'b');
+CREATE TABLE pagg_tab6_p2 PARTITION OF pagg_tab6 FOR VALUES IN ('c', 'd');
+INSERT INTO pagg_tab6 (b, c) SELECT substr('abAB', (i % 4) + 1 , 1), substr('abAB', (i % 2) + 1 , 1) FROM generate_series(0, 5) i;
+INSERT INTO pagg_tab6 (b, c) SELECT substr('cdCD', (i % 4) + 1 , 1), substr('cdCD', (i % 2) + 1 , 1) FROM generate_series(0, 5) i;
+ANALYZE pagg_tab5;
+EXPLAIN (COSTS OFF)
+SELECT t1.c, count(t2.c) FROM pagg_tab5 t1 JOIN pagg_tab6 t2 ON t1.c = t2.c AND t1.c = t2.b GROUP BY 1 ORDER BY t1.c COLLATE "C";
+ QUERY PLAN
+-------------------------------------------------------------------
+ Sort
+ Sort Key: t1.c COLLATE "C"
+ -> GroupAggregate
+ Group Key: t1.c
+ -> Sort
+ Sort Key: t1.c COLLATE case_insensitive
+ -> Append
+ -> Hash Join
+ Hash Cond: (t2_1.c = t1_1.c)
+ -> Seq Scan on pagg_tab6_p1 t2_1
+ Filter: (c = b)
+ -> Hash
+ -> Seq Scan on pagg_tab5_p1 t1_1
+ -> Hash Join
+ Hash Cond: (t2_2.c = t1_2.c)
+ -> Seq Scan on pagg_tab6_p2 t2_2
+ Filter: (c = b)
+ -> Hash
+ -> Seq Scan on pagg_tab5_p2 t1_2
+(19 rows)
+
+SELECT t1.c, count(t2.c) FROM pagg_tab5 t1 JOIN pagg_tab6 t2 ON t1.c = t2.c AND t1.c = t2.b GROUP BY 1 ORDER BY t1.c COLLATE "C";
+ c | count
+---+-------
+ a | 9
+ b | 9
+ c | 9
+ d | 9
+(4 rows)
+
+SET enable_partitionwise_join TO false;
+EXPLAIN (COSTS OFF)
+SELECT t1.c, count(t2.c) FROM pagg_tab5 t1 JOIN pagg_tab6 t2 ON t1.c = t2.c AND t1.c = t2.b GROUP BY 1 ORDER BY t1.c COLLATE "C";
+ QUERY PLAN
+-------------------------------------------------------------
+ Sort
+ Sort Key: t1.c COLLATE "C"
+ -> GroupAggregate
+ Group Key: t1.c
+ -> Merge Join
+ Merge Cond: (t2.c = t1.c)
+ -> Sort
+ Sort Key: t2.c COLLATE case_insensitive
+ -> Append
+ -> Seq Scan on pagg_tab6_p1 t2_1
+ Filter: (c = b)
+ -> Seq Scan on pagg_tab6_p2 t2_2
+ Filter: (c = b)
+ -> Sort
+ Sort Key: t1.c COLLATE case_insensitive
+ -> Append
+ -> Seq Scan on pagg_tab5_p1 t1_1
+ -> Seq Scan on pagg_tab5_p2 t1_2
+(18 rows)
+
+SELECT t1.c, count(t2.c) FROM pagg_tab5 t1 JOIN pagg_tab6 t2 ON t1.c = t2.c AND t1.c = t2.b GROUP BY 1 ORDER BY t1.c COLLATE "C";
+ c | count
+---+-------
+ a | 9
+ b | 9
+ c | 9
+ d | 9
+(4 rows)
+
DROP TABLE pagg_tab3;
+DROP TABLE pagg_tab4;
+DROP TABLE pagg_tab5;
+DROP TABLE pagg_tab6;
RESET enable_partitionwise_aggregate;
RESET max_parallel_workers_per_gather;
RESET enable_incremental_sort;
diff --git a/src/test/regress/sql/collate.icu.utf8.sql b/src/test/regress/sql/collate.icu.utf8.sql
index f523dd73be..bad92b2ab7 100644
--- a/src/test/regress/sql/collate.icu.utf8.sql
+++ b/src/test/regress/sql/collate.icu.utf8.sql
@@ -834,7 +834,65 @@ EXPLAIN (COSTS OFF)
SELECT a collate "C", count(c collate "C") FROM pagg_tab3 GROUP BY a collate "C" ORDER BY 1;
SELECT a collate "C", count(c collate "C") FROM pagg_tab3 GROUP BY a collate "C" ORDER BY 1;
+-- Partitionwise join should not be allowed too when the collation used by the
+-- join keys doesn't match the partition key collation.
+SET enable_partitionwise_join TO false;
+EXPLAIN (COSTS OFF)
+SELECT t1.c, count(t2.c) FROM pagg_tab3 t1 JOIN pagg_tab3 t2 ON t1.c = t2.c GROUP BY 1 ORDER BY t1.c COLLATE "C";
+SELECT t1.c, count(t2.c) FROM pagg_tab3 t1 JOIN pagg_tab3 t2 ON t1.c = t2.c GROUP BY 1 ORDER BY t1.c COLLATE "C";
+
+SET enable_partitionwise_join TO true;
+EXPLAIN (COSTS OFF)
+SELECT t1.c, count(t2.c) FROM pagg_tab3 t1 JOIN pagg_tab3 t2 ON t1.c = t2.c GROUP BY 1 ORDER BY t1.c COLLATE "C";
+SELECT t1.c, count(t2.c) FROM pagg_tab3 t1 JOIN pagg_tab3 t2 ON t1.c = t2.c GROUP BY 1 ORDER BY t1.c COLLATE "C";
+
+-- OK when the join clause uses the same collation as the partition key.
+EXPLAIN (COSTS OFF)
+SELECT t1.c COLLATE "C", count(t2.c) FROM pagg_tab3 t1 JOIN pagg_tab3 t2 ON t1.c = t2.c COLLATE "C" GROUP BY t1.c COLLATE "C" ORDER BY t1.c COLLATE "C";
+SELECT t1.c COLLATE "C", count(t2.c) FROM pagg_tab3 t1 JOIN pagg_tab3 t2 ON t1.c = t2.c COLLATE "C" GROUP BY t1.c COLLATE "C" ORDER BY t1.c COLLATE "C";
+
+-- Few other cases where the joined partition keys are matched via equivalence
+-- class, not a join restriction clause.
+
+-- Collations of joined columns match, but the partition keys collation is different
+CREATE TABLE pagg_tab4 (c text collate case_insensitive, b text collate case_insensitive) PARTITION BY LIST (b collate "C");
+CREATE TABLE pagg_tab4_p1 PARTITION OF pagg_tab4 FOR VALUES IN ('a', 'b');
+CREATE TABLE pagg_tab4_p2 PARTITION OF pagg_tab4 FOR VALUES IN ('B', 'A');
+INSERT INTO pagg_tab4 (b, c) SELECT substr('abAB', (i % 4) + 1 , 1), substr('abAB', (i % 2) + 1 , 1) FROM generate_series(0, 11) i;
+ANALYZE pagg_tab4;
+
+EXPLAIN (COSTS OFF)
+SELECT t1.c, count(t2.c) FROM pagg_tab3 t1 JOIN pagg_tab4 t2 ON t1.c = t2.c AND t1.c = t2.b GROUP BY 1 ORDER BY t1.c COLLATE "C";
+SELECT t1.c, count(t2.c) FROM pagg_tab3 t1 JOIN pagg_tab4 t2 ON t1.c = t2.c AND t1.c = t2.b GROUP BY 1 ORDER BY t1.c COLLATE "C";
+
+-- OK when the partition key collation is same as that of the join columns
+CREATE TABLE pagg_tab5 (c text collate case_insensitive, b text collate case_insensitive) PARTITION BY LIST (c collate case_insensitive);
+CREATE TABLE pagg_tab5_p1 PARTITION OF pagg_tab5 FOR VALUES IN ('a', 'b');
+CREATE TABLE pagg_tab5_p2 PARTITION OF pagg_tab5 FOR VALUES IN ('c', 'd');
+INSERT INTO pagg_tab5 (b, c) SELECT substr('abAB', (i % 4) + 1 , 1), substr('abAB', (i % 2) + 1 , 1) FROM generate_series(0, 5) i;
+INSERT INTO pagg_tab5 (b, c) SELECT substr('cdCD', (i % 4) + 1 , 1), substr('cdCD', (i % 2) + 1 , 1) FROM generate_series(0, 5) i;
+ANALYZE pagg_tab5;
+
+CREATE TABLE pagg_tab6 (c text collate case_insensitive, b text collate case_insensitive) PARTITION BY LIST (b collate case_insensitive);
+CREATE TABLE pagg_tab6_p1 PARTITION OF pagg_tab6 FOR VALUES IN ('a', 'b');
+CREATE TABLE pagg_tab6_p2 PARTITION OF pagg_tab6 FOR VALUES IN ('c', 'd');
+INSERT INTO pagg_tab6 (b, c) SELECT substr('abAB', (i % 4) + 1 , 1), substr('abAB', (i % 2) + 1 , 1) FROM generate_series(0, 5) i;
+INSERT INTO pagg_tab6 (b, c) SELECT substr('cdCD', (i % 4) + 1 , 1), substr('cdCD', (i % 2) + 1 , 1) FROM generate_series(0, 5) i;
+ANALYZE pagg_tab5;
+
+EXPLAIN (COSTS OFF)
+SELECT t1.c, count(t2.c) FROM pagg_tab5 t1 JOIN pagg_tab6 t2 ON t1.c = t2.c AND t1.c = t2.b GROUP BY 1 ORDER BY t1.c COLLATE "C";
+SELECT t1.c, count(t2.c) FROM pagg_tab5 t1 JOIN pagg_tab6 t2 ON t1.c = t2.c AND t1.c = t2.b GROUP BY 1 ORDER BY t1.c COLLATE "C";
+
+SET enable_partitionwise_join TO false;
+EXPLAIN (COSTS OFF)
+SELECT t1.c, count(t2.c) FROM pagg_tab5 t1 JOIN pagg_tab6 t2 ON t1.c = t2.c AND t1.c = t2.b GROUP BY 1 ORDER BY t1.c COLLATE "C";
+SELECT t1.c, count(t2.c) FROM pagg_tab5 t1 JOIN pagg_tab6 t2 ON t1.c = t2.c AND t1.c = t2.b GROUP BY 1 ORDER BY t1.c COLLATE "C";
+
DROP TABLE pagg_tab3;
+DROP TABLE pagg_tab4;
+DROP TABLE pagg_tab5;
+DROP TABLE pagg_tab6;
RESET enable_partitionwise_aggregate;
RESET max_parallel_workers_per_gather;
--
2.43.0
[application/octet-stream] v6-0001-Disallow-partitionwise-grouping-when-collation-do.patch (12.4K, ../../CA+HiwqHv+7gp+SPfE-=+KYmvrcVWryz+F5HNG2Rb6=UZL_A+OQ@mail.gmail.com/3-v6-0001-Disallow-partitionwise-grouping-when-collation-do.patch)
download | inline diff:
From 6fa7144a91ca21b0ca6bd6fcc34a57ab517359d8 Mon Sep 17 00:00:00 2001
From: Amit Langote <[email protected]>
Date: Fri, 1 Nov 2024 16:15:50 +0900
Subject: [PATCH v6 1/2] Disallow partitionwise grouping when collation doesn't
match
Insist that the collation used for grouping matches exactly with the
collation used for partitioning to allow using either full or
partial partitionwise grouping.
Bug: #18568
Reported-by: Webbo Han <[email protected]>
Author: Webbo Han <[email protected]>
Reviewed-by: Tender Wang <[email protected]>
Reviewed-by: Aleksander Alekseev <[email protected]>
Reviewed-by: Jian He <[email protected]>
Discussion: https://postgr.es/m/[email protected]
Discussion: https://postgr.es/m/[email protected]
Discussion: https://postgr.es/m/CAHewXNno_HKiQ6PqyLYfuqDtwp7KKHZiH1J7Pqyz0nr+PS2Dwg@mail.gmail.com
---
src/backend/optimizer/plan/planner.c | 67 +++++++---
.../regress/expected/collate.icu.utf8.out | 114 ++++++++++++++++++
src/test/regress/sql/collate.icu.utf8.sql | 43 +++++++
3 files changed, 210 insertions(+), 14 deletions(-)
diff --git a/src/backend/optimizer/plan/planner.c b/src/backend/optimizer/plan/planner.c
index 0f423e9684..24a3ff087e 100644
--- a/src/backend/optimizer/plan/planner.c
+++ b/src/backend/optimizer/plan/planner.c
@@ -253,7 +253,8 @@ static void create_partitionwise_grouping_paths(PlannerInfo *root,
GroupPathExtraData *extra);
static bool group_by_has_partkey(RelOptInfo *input_rel,
List *targetList,
- List *groupClause);
+ List *groupClause,
+ bool *collation_incompatible);
static int common_prefix_cmp(const void *a, const void *b);
static List *generate_setop_child_grouplist(SetOperationStmt *op,
List *targetlist);
@@ -4090,23 +4091,30 @@ create_ordinary_grouping_paths(PlannerInfo *root, RelOptInfo *input_rel,
if (extra->patype != PARTITIONWISE_AGGREGATE_NONE &&
IS_PARTITIONED_REL(input_rel))
{
+ bool collation_incompatible = false;
+ bool group_by_contains_partkey =
+ group_by_has_partkey(input_rel, extra->targetList,
+ root->parse->groupClause,
+ &collation_incompatible);
+
/*
* If this is the topmost relation or if the parent relation is doing
* full partitionwise aggregation, then we can do full partitionwise
* aggregation provided that the GROUP BY clause contains all of the
- * partitioning columns at this level. Otherwise, we can do at most
- * partial partitionwise aggregation. But if partial aggregation is
- * not supported in general then we can't use it for partitionwise
- * aggregation either.
+ * partitioning columns at this level and the collation used by GROUP
+ * BY matches the partitioning collation. Otherwise, we can do at most
+ * partial partitionwise aggregation, but again only if the collation
+ * is compatible. If partial aggregation is not supported in general
+ * then we can't use it for partitionwise aggregation either.
*
* Check parse->groupClause not processed_groupClause, because it's
* okay if some of the partitioning columns were proved redundant.
*/
if (extra->patype == PARTITIONWISE_AGGREGATE_FULL &&
- group_by_has_partkey(input_rel, extra->targetList,
- root->parse->groupClause))
+ group_by_contains_partkey)
patype = PARTITIONWISE_AGGREGATE_FULL;
- else if ((extra->flags & GROUPING_CAN_PARTIAL_AGG) != 0)
+ else if ((extra->flags & GROUPING_CAN_PARTIAL_AGG) != 0 &&
+ !collation_incompatible)
patype = PARTITIONWISE_AGGREGATE_PARTIAL;
else
patype = PARTITIONWISE_AGGREGATE_NONE;
@@ -8105,13 +8113,18 @@ create_partitionwise_grouping_paths(PlannerInfo *root,
/*
* group_by_has_partkey
*
- * Returns true, if all the partition keys of the given relation are part of
- * the GROUP BY clauses, false otherwise.
+ * Returns true if all the partition keys of the given relation are part of
+ * the GROUP BY clauses, including having matching collation, false otherwise.
+ *
+ * Returns false also if a collation mismatch is detected between a partition
+ * key and its corresponding expression in groupClause, in which case.
+ * *collation_incompatible is set to true.
*/
static bool
group_by_has_partkey(RelOptInfo *input_rel,
List *targetList,
- List *groupClause)
+ List *groupClause,
+ bool *collation_incompatible)
{
List *groupexprs = get_sortgrouplist_exprs(groupClause, targetList);
int cnt = 0;
@@ -8134,13 +8147,39 @@ 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 groupcoll = exprCollation((Node *) groupexpr);
+
+ if (IsA(groupexpr, RelabelType))
+ groupexpr = ((RelabelType *) groupexpr)->arg;
+
+ if (equal(groupexpr, partexpr))
+ {
+
+ /*
+ * Reject a match if the grouping collation does not match
+ * the partitioning collation.
+ */
+ if (OidIsValid(partcoll) && OidIsValid(groupcoll) &&
+ partcoll != groupcoll)
+ {
+ *collation_incompatible = true;
+ return false;
+ }
+
+ found = true;
+ break;
+ }
}
+
+ if (found)
+ break;
}
/*
diff --git a/src/test/regress/expected/collate.icu.utf8.out b/src/test/regress/expected/collate.icu.utf8.out
index faa376e060..a4cd9ea085 100644
--- a/src/test/regress/expected/collate.icu.utf8.out
+++ b/src/test/regress/expected/collate.icu.utf8.out
@@ -2054,6 +2054,120 @@ SELECT (SELECT count(*) FROM test33_0) <> (SELECT count(*) FROM test33_1);
t
(1 row)
+--
+-- Bug #18568
+--
+-- Partitionwise aggregate (full or partial) should not be used when a
+-- partition key's collation doesn't match that of the GROUP BY column it is
+-- matched with.
+SET max_parallel_workers_per_gather TO 0;
+SET enable_incremental_sort TO off;
+CREATE TABLE pagg_tab3 (a text, 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');
+CREATE TABLE pagg_tab3_p2 PARTITION OF pagg_tab3 FOR VALUES IN ('B', 'A');
+INSERT INTO pagg_tab3 SELECT i % 4 + 1, substr('abAB', (i % 4) + 1 , 1) FROM generate_series(0, 11) i;
+ANALYZE pagg_tab3;
+SET enable_partitionwise_aggregate TO false;
+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
+ -> HashAggregate
+ Group Key: pagg_tab3.c
+ -> Append
+ -> Seq Scan on pagg_tab3_p2 pagg_tab3_1
+ -> Seq Scan on pagg_tab3_p1 pagg_tab3_2
+(7 rows)
+
+SELECT upper(c collate case_insensitive), count(c) FROM pagg_tab3 GROUP BY c collate case_insensitive ORDER BY 1;
+ upper | count
+-------+-------
+ A | 6
+ B | 6
+(2 rows)
+
+-- No partitionwise aggregation allowed!
+SET enable_partitionwise_aggregate TO true;
+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
+ -> HashAggregate
+ Group Key: pagg_tab3.c
+ -> Append
+ -> Seq Scan on pagg_tab3_p2 pagg_tab3_1
+ -> Seq Scan on pagg_tab3_p1 pagg_tab3_2
+(7 rows)
+
+SELECT upper(c collate case_insensitive), count(c) FROM pagg_tab3 GROUP BY c collate case_insensitive ORDER BY 1;
+ upper | count
+-------+-------
+ A | 6
+ B | 6
+(2 rows)
+
+-- OK to use full partitionwise aggregate after changing the GROUP BY column's
+-- collation to be the same as that of the partition key.
+EXPLAIN (COSTS OFF)
+SELECT c collate "C", count(c) FROM pagg_tab3 GROUP BY c collate "C" ORDER BY 1;
+ QUERY PLAN
+--------------------------------------------------------
+ Sort
+ Sort Key: ((pagg_tab3.c)::text) COLLATE "C"
+ -> Append
+ -> HashAggregate
+ Group Key: (pagg_tab3.c)::text
+ -> Seq Scan on pagg_tab3_p2 pagg_tab3
+ -> HashAggregate
+ Group Key: (pagg_tab3_1.c)::text
+ -> Seq Scan on pagg_tab3_p1 pagg_tab3_1
+(9 rows)
+
+SELECT c collate "C", count(c) FROM pagg_tab3 GROUP BY c collate "C" ORDER BY 1;
+ c | count
+---+-------
+ A | 3
+ B | 3
+ a | 3
+ b | 3
+(4 rows)
+
+-- Also OK to use partial partitionwise aggregate when grouping columns do not
+-- include partition key columns
+EXPLAIN (COSTS OFF)
+SELECT a collate "C", count(c collate "C") FROM pagg_tab3 GROUP BY a collate "C" ORDER BY 1;
+ QUERY PLAN
+--------------------------------------------------------------
+ Finalize GroupAggregate
+ Group Key: ((pagg_tab3.a)::text)
+ -> Sort
+ Sort Key: ((pagg_tab3.a)::text) COLLATE "C"
+ -> Append
+ -> Partial HashAggregate
+ Group Key: (pagg_tab3.a)::text
+ -> Seq Scan on pagg_tab3_p2 pagg_tab3
+ -> Partial HashAggregate
+ Group Key: (pagg_tab3_1.a)::text
+ -> Seq Scan on pagg_tab3_p1 pagg_tab3_1
+(11 rows)
+
+SELECT a collate "C", count(c collate "C") FROM pagg_tab3 GROUP BY a collate "C" ORDER BY 1;
+ a | count
+---+-------
+ 1 | 3
+ 2 | 3
+ 3 | 3
+ 4 | 3
+(4 rows)
+
+DROP TABLE pagg_tab3;
+RESET enable_partitionwise_aggregate;
+RESET max_parallel_workers_per_gather;
+RESET enable_incremental_sort;
-- cleanup
RESET search_path;
SET client_min_messages TO warning;
diff --git a/src/test/regress/sql/collate.icu.utf8.sql b/src/test/regress/sql/collate.icu.utf8.sql
index 80f28a97d7..f523dd73be 100644
--- a/src/test/regress/sql/collate.icu.utf8.sql
+++ b/src/test/regress/sql/collate.icu.utf8.sql
@@ -796,6 +796,49 @@ INSERT INTO test33 VALUES (2, 'DEF');
-- they end up in the same partition (but it's platform-dependent which one)
SELECT (SELECT count(*) FROM test33_0) <> (SELECT count(*) FROM test33_1);
+--
+-- Bug #18568
+--
+-- Partitionwise aggregate (full or partial) should not be used when a
+-- partition key's collation doesn't match that of the GROUP BY column it is
+-- matched with.
+SET max_parallel_workers_per_gather TO 0;
+SET enable_incremental_sort TO off;
+
+CREATE TABLE pagg_tab3 (a text, 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');
+CREATE TABLE pagg_tab3_p2 PARTITION OF pagg_tab3 FOR VALUES IN ('B', 'A');
+INSERT INTO pagg_tab3 SELECT i % 4 + 1, substr('abAB', (i % 4) + 1 , 1) FROM generate_series(0, 11) i;
+ANALYZE pagg_tab3;
+
+SET enable_partitionwise_aggregate TO false;
+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;
+
+-- No partitionwise aggregation allowed!
+SET enable_partitionwise_aggregate TO true;
+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;
+
+-- OK to use full partitionwise aggregate after changing the GROUP BY column's
+-- collation to be the same as that of the partition key.
+EXPLAIN (COSTS OFF)
+SELECT c collate "C", count(c) FROM pagg_tab3 GROUP BY c collate "C" ORDER BY 1;
+SELECT c collate "C", count(c) FROM pagg_tab3 GROUP BY c collate "C" ORDER BY 1;
+
+-- Also OK to use partial partitionwise aggregate when grouping columns do not
+-- include partition key columns
+EXPLAIN (COSTS OFF)
+SELECT a collate "C", count(c collate "C") FROM pagg_tab3 GROUP BY a collate "C" ORDER BY 1;
+SELECT a collate "C", count(c collate "C") FROM pagg_tab3 GROUP BY a collate "C" ORDER BY 1;
+
+DROP TABLE pagg_tab3;
+
+RESET enable_partitionwise_aggregate;
+RESET max_parallel_workers_per_gather;
+RESET enable_incremental_sort;
-- cleanup
RESET search_path;
--
2.43.0
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: Wrong result when enable_partitionwise_join is on if collation of PartitionKey and Column is different.
2024-10-30 03:57 Re: Wrong result when enable_partitionwise_join is on if collation of PartitionKey and Column is different. jian he <[email protected]>
2024-10-30 12:36 ` Re: Wrong result when enable_partitionwise_join is on if collation of PartitionKey and Column is different. Junwang Zhao <[email protected]>
2024-10-31 13:08 ` Re: Wrong result when enable_partitionwise_join is on if collation of PartitionKey and Column is different. Amit Langote <[email protected]>
2024-11-01 05:39 ` Re: Wrong result when enable_partitionwise_join is on if collation of PartitionKey and Column is different. jian he <[email protected]>
2024-11-01 07:25 ` Re: Wrong result when enable_partitionwise_join is on if collation of PartitionKey and Column is different. Amit Langote <[email protected]>
2024-11-01 08:07 ` Re: Wrong result when enable_partitionwise_join is on if collation of PartitionKey and Column is different. jian he <[email protected]>
2024-11-01 10:08 ` Re: Wrong result when enable_partitionwise_join is on if collation of PartitionKey and Column is different. Amit Langote <[email protected]>
2024-11-04 03:27 ` Re: Wrong result when enable_partitionwise_join is on if collation of PartitionKey and Column is different. jian he <[email protected]>
2024-11-05 07:55 ` Re: Wrong result when enable_partitionwise_join is on if collation of PartitionKey and Column is different. Amit Langote <[email protected]>
@ 2024-11-06 03:19 ` jian he <[email protected]>
2024-11-08 09:27 ` Re: Wrong result when enable_partitionwise_join is on if collation of PartitionKey and Column is different. Amit Langote <[email protected]>
0 siblings, 1 reply; 15+ messages in thread
From: jian he @ 2024-11-06 03:19 UTC (permalink / raw)
To: Amit Langote <[email protected]>; +Cc: Junwang Zhao <[email protected]>; Tender Wang <[email protected]>; PostgreSQL Hackers <[email protected]>; Ashutosh Bapat <[email protected]>
looks good to me.
I didn't find any issue.
group_by_has_partkey can even cope with:
EXPLAIN (COSTS OFF, settings)
SELECT c collate "C" collate case_insensitive collate "C", count(c)
FROM pagg_tab3 GROUP BY c collate "C" collate case_insensitive collate
"C" ORDER BY 1;
so i guess in group_by_has_partkey
if (IsA(groupexpr, RelabelType))
groupexpr = ((RelabelType *) groupexpr)->arg;
should be enough.
not need while loop.
while (IsA(groupexpr, RelabelType))
groupexpr = (Expr *) (castNode(RelabelType, groupexpr))->arg;
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: Wrong result when enable_partitionwise_join is on if collation of PartitionKey and Column is different.
2024-10-30 03:57 Re: Wrong result when enable_partitionwise_join is on if collation of PartitionKey and Column is different. jian he <[email protected]>
2024-10-30 12:36 ` Re: Wrong result when enable_partitionwise_join is on if collation of PartitionKey and Column is different. Junwang Zhao <[email protected]>
2024-10-31 13:08 ` Re: Wrong result when enable_partitionwise_join is on if collation of PartitionKey and Column is different. Amit Langote <[email protected]>
2024-11-01 05:39 ` Re: Wrong result when enable_partitionwise_join is on if collation of PartitionKey and Column is different. jian he <[email protected]>
2024-11-01 07:25 ` Re: Wrong result when enable_partitionwise_join is on if collation of PartitionKey and Column is different. Amit Langote <[email protected]>
2024-11-01 08:07 ` Re: Wrong result when enable_partitionwise_join is on if collation of PartitionKey and Column is different. jian he <[email protected]>
2024-11-01 10:08 ` Re: Wrong result when enable_partitionwise_join is on if collation of PartitionKey and Column is different. Amit Langote <[email protected]>
2024-11-04 03:27 ` Re: Wrong result when enable_partitionwise_join is on if collation of PartitionKey and Column is different. jian he <[email protected]>
2024-11-05 07:55 ` Re: Wrong result when enable_partitionwise_join is on if collation of PartitionKey and Column is different. Amit Langote <[email protected]>
2024-11-06 03:19 ` Re: Wrong result when enable_partitionwise_join is on if collation of PartitionKey and Column is different. jian he <[email protected]>
@ 2024-11-08 09:27 ` Amit Langote <[email protected]>
0 siblings, 0 replies; 15+ messages in thread
From: Amit Langote @ 2024-11-08 09:27 UTC (permalink / raw)
To: jian he <[email protected]>; +Cc: Junwang Zhao <[email protected]>; Tender Wang <[email protected]>; PostgreSQL Hackers <[email protected]>; Ashutosh Bapat <[email protected]>
Hi,
On Wed, Nov 6, 2024 at 12:19 PM jian he <[email protected]> wrote:
> looks good to me.
> I didn't find any issue.
Thanks for the review.
> group_by_has_partkey can even cope with:
> EXPLAIN (COSTS OFF, settings)
> SELECT c collate "C" collate case_insensitive collate "C", count(c)
> FROM pagg_tab3 GROUP BY c collate "C" collate case_insensitive collate
> "C" ORDER BY 1;
>
> so i guess in group_by_has_partkey
> if (IsA(groupexpr, RelabelType))
> groupexpr = ((RelabelType *) groupexpr)->arg;
> should be enough.
>
> not need while loop.
>
> while (IsA(groupexpr, RelabelType))
> groupexpr = (Expr *) (castNode(RelabelType, groupexpr))->arg;
Added a comment about that.
Pushed both patches after making changes to 0001 to allow "partial"
partitionwise aggregation after all. The differences in output with
partial partitionwise aggregation and no partitionwise aggregation
that I mentioned before don't seem to have anything to do with
partitionwise aggregation, but apparently with whether aggregation was
hashed or not. I confirmed that by turning enable_hashagg on and off
to see the difference. Changing enable_partitionwise_aggregate for
either of the values of enable_hashagg didn't change the plan.
Thank you all for working on this.
--
Thanks, Amit Langote
^ permalink raw reply [nested|flat] 15+ messages in thread
end of thread, other threads:[~2024-11-08 09:27 UTC | newest]
Thread overview: 15+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2024-09-19 04:48 [PATCH v22 6/8] Row pattern recognition patch (docs). Tatsuo Ishii <[email protected]>
2024-10-30 03:57 Re: Wrong result when enable_partitionwise_join is on if collation of PartitionKey and Column is different. jian he <[email protected]>
2024-10-30 12:36 ` Re: Wrong result when enable_partitionwise_join is on if collation of PartitionKey and Column is different. Junwang Zhao <[email protected]>
2024-10-31 13:08 ` Re: Wrong result when enable_partitionwise_join is on if collation of PartitionKey and Column is different. Amit Langote <[email protected]>
2024-11-01 02:39 ` Re: Wrong result when enable_partitionwise_join is on if collation of PartitionKey and Column is different. Tender Wang <[email protected]>
2024-11-01 06:55 ` Re: Wrong result when enable_partitionwise_join is on if collation of PartitionKey and Column is different. Amit Langote <[email protected]>
2024-11-01 02:59 ` Re: Wrong result when enable_partitionwise_join is on if collation of PartitionKey and Column is different. Tender Wang <[email protected]>
2024-11-01 05:39 ` Re: Wrong result when enable_partitionwise_join is on if collation of PartitionKey and Column is different. jian he <[email protected]>
2024-11-01 07:25 ` Re: Wrong result when enable_partitionwise_join is on if collation of PartitionKey and Column is different. Amit Langote <[email protected]>
2024-11-01 08:07 ` Re: Wrong result when enable_partitionwise_join is on if collation of PartitionKey and Column is different. jian he <[email protected]>
2024-11-01 10:08 ` Re: Wrong result when enable_partitionwise_join is on if collation of PartitionKey and Column is different. Amit Langote <[email protected]>
2024-11-04 03:27 ` Re: Wrong result when enable_partitionwise_join is on if collation of PartitionKey and Column is different. jian he <[email protected]>
2024-11-05 07:55 ` Re: Wrong result when enable_partitionwise_join is on if collation of PartitionKey and Column is different. Amit Langote <[email protected]>
2024-11-06 03:19 ` Re: Wrong result when enable_partitionwise_join is on if collation of PartitionKey and Column is different. jian he <[email protected]>
2024-11-08 09:27 ` Re: Wrong result when enable_partitionwise_join is on if collation of PartitionKey and Column is different. Amit Langote <[email protected]>
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox