Subject: Re: Wrong result when enable_partitionwise_join is on if collation of PartitionKey and Column is different.

From: jian he <[email-masked]>
Date: Wed, 30 Oct 2024 11:57:35 +0800

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.
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;
+			}
 		}
 	}

---

From: Junwang Zhao <[email-masked]>
Date: Wed, 30 Oct 2024 20:36:25 +0800

> 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

---

From: Amit Langote <[email-masked]>
Date: Thu, 31 Oct 2024 22:08:58 +0900

> 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.

+           {

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

---

From: Tender Wang <[email-masked]>
Date: Fri, 1 Nov 2024 10:39:35 +0800

> expression collation is deterministic after confirming that it's not

Me, too.

> what Jian proposed is that match_expr_to_partition_keys() returns -1

Agree.

> 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

---

From: Tender Wang <[email-masked]>
Date: Fri, 1 Nov 2024 10:59:21 +0800

> enable_partitionwise_aggregate is off.

LGTM

-- 
Thanks,
Tender Wang

---

From: jian he <[email-masked]>
Date: Fri, 1 Nov 2024 13:39:11 +0800

> 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;

---

From: Amit Langote <[email-masked]>
Date: Fri, 1 Nov 2024 15:55:19 +0900

Hi,

> 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

---

From: Amit Langote <[email-masked]>
Date: Fri, 1 Nov 2024 16:25:28 +0900

Hi,

> join key collation and the partition key collation same sure would

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.

> +               if (rel1->part_scheme->partcollation[ipk1] !=

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

---

From: jian he <[email-masked]>
Date: Fri, 1 Nov 2024 16:07:00 +0800

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
?

---

From: Amit Langote <[email-masked]>
Date: Fri, 1 Nov 2024 19:08:00 +0900

> CREATE TABLE pagg_tab_p1 PARTITION OF pagg_tab FOR VALUES IN ('0000',

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

---

From: jian he <[email-masked]>
Date: Mon, 4 Nov 2024 11:27:49 +0800

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.

---

From: Amit Langote <[email-masked]>
Date: Tue, 5 Nov 2024 16:55:25 +0900

Hi,

> Other than the comments that confused me, the test and the results

Thanks, yes, a test case that exercises the partcoll1 == exprcoll1
code was missing.

> So I did some minor refactoring to make the "ORDER BY" deterministic.

Thanks, merged.

-- 
Thanks, Amit Langote

---

From: jian he <[email-masked]>
Date: Wed, 6 Nov 2024 11:19:45 +0800

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;

---

From: Amit Langote <[email-masked]>
Date: Fri, 8 Nov 2024 18:27:32 +0900

Hi,

> I didn't find any issue.

Thanks for the review.

>         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