Horton

Thread

Commits

  1. Disallow partitionwise join when collations don't match

  2. Disallow partitionwise grouping when collations don't match

  1. Wrong result when enable_partitionwise_join is on if collation of PartitionKey and Column is different.

    Tender Wang <tndrwang@gmail.com> — 2024-10-23T13:48:11Z

    Hi all,

I find another issue as $SUBJECT when I work on [1].
I will use [1] SQL to show the problem.
CREATE COLLATION case_insensitive (
provider = icu,
locale = 'und-u-ks-level2',
deterministic = false
);
CREATE TABLE pagg_tab (c text collate case_insensitive) PARTITION BY LIST(c
collate "C");
CREATE TABLE pagg_tab_p1 PARTITION OF pagg_tab FOR VALUES IN ('a', 'b',
'c', 'd');
CREATE TABLE pagg_tab_p2 PARTITION OF pagg_tab FOR VALUES IN ('e', 'f',
'A');
CREATE TABLE pagg_tab_p3 PARTITION OF pagg_tab FOR VALUES IN ('B', 'C',
'D', 'E');
INSERT INTO pagg_tab SELECT substr('abcdeABCDE', (i % 10) +1 , 1) FROM
generate_series(0, 2999) i;
ANALYZE pagg_tab;

set max_parallel_workers_per_gather = 0;
postgres=# show enable_partitionwise_join ;
 enable_partitionwise_join
---------------------------
 off
(1 row)
postgres=# explain select count(*) from pagg_tab t1 join pagg_tab t2 on
t1.c = t2.c;
                                         QUERY PLAN
--------------------------------------------------------------------------------------------
 Aggregate  (cost=24915.00..24915.01 rows=1 width=8)
   ->  Hash Join  (cost=97.50..20415.00 rows=1800000 width=0)
         Hash Cond: (t1.c = t2.c)
         ->  Append  (cost=0.00..60.00 rows=3000 width=2)
               ->  Seq Scan on pagg_tab_p2 t1_1  (cost=0.00..9.00 rows=600
width=2)
               ->  Seq Scan on pagg_tab_p3 t1_2  (cost=0.00..18.00
rows=1200 width=2)
               ->  Seq Scan on pagg_tab_p1 t1_3  (cost=0.00..18.00
rows=1200 width=2)
         ->  Hash  (cost=60.00..60.00 rows=3000 width=2)
               ->  Append  (cost=0.00..60.00 rows=3000 width=2)
                     ->  Seq Scan on pagg_tab_p2 t2_1  (cost=0.00..9.00
rows=600 width=2)
                     ->  Seq Scan on pagg_tab_p3 t2_2  (cost=0.00..18.00
rows=1200 width=2)
                     ->  Seq Scan on pagg_tab_p1 t2_3  (cost=0.00..18.00
rows=1200 width=2)
(12 rows)

postgres=#  select count(*) from pagg_tab t1 join pagg_tab t2 on t1.c =
t2.c;
  count
---------
 1800000
(1 row)

But if we set enable_partitionwise_join is on, the result is different, as
below.
postgres=# set enable_partitionwise_join = on;
SET
postgres=# explain select count(*) from pagg_tab t1 join pagg_tab t2 on
t1.c = t2.c;
                                         QUERY PLAN
--------------------------------------------------------------------------------------------
 Aggregate  (cost=17010.00..17010.01 rows=1 width=8)
   ->  Append  (cost=16.50..14760.00 rows=900000 width=0)
         ->  Hash Join  (cost=16.50..2052.00 rows=180000 width=0)
               Hash Cond: (t1_1.c = t2_1.c)
               ->  Seq Scan on pagg_tab_p2 t1_1  (cost=0.00..9.00 rows=600
width=2)
               ->  Hash  (cost=9.00..9.00 rows=600 width=2)
                     ->  Seq Scan on pagg_tab_p2 t2_1  (cost=0.00..9.00
rows=600 width=2)
         ->  Hash Join  (cost=33.00..4104.00 rows=360000 width=0)
               Hash Cond: (t1_2.c = t2_2.c)
               ->  Seq Scan on pagg_tab_p3 t1_2  (cost=0.00..18.00
rows=1200 width=2)
               ->  Hash  (cost=18.00..18.00 rows=1200 width=2)
                     ->  Seq Scan on pagg_tab_p3 t2_2  (cost=0.00..18.00
rows=1200 width=2)
         ->  Hash Join  (cost=33.00..4104.00 rows=360000 width=0)
               Hash Cond: (t1_3.c = t2_3.c)
               ->  Seq Scan on pagg_tab_p1 t1_3  (cost=0.00..18.00
rows=1200 width=2)
               ->  Hash  (cost=18.00..18.00 rows=1200 width=2)
                     ->  Seq Scan on pagg_tab_p1 t2_3  (cost=0.00..18.00
rows=1200 width=2)
(17 rows)

postgres=#  select count(*) from pagg_tab t1 join pagg_tab t2 on t1.c =
t2.c;
 count
--------
 900000
(1 row)

I think the root cause of this thread and [1] are same.  We don't use the
Partition Key collation but column's
collation to fill the RelOptInfo partexprs field in
set_baserel_partition_key_exprs().
If the Partition Key definition is same as column definition,  which most
times is,
that will be ok. But if it's not, this thread issue will arise.

As far as I know, only partition pruning logic considers not only call
equal(), but also check collation match.
Other codes only call equal() to check if the exprs match the partition key.
For example, in this thread case, match_expr_to_partition_keys() think the
expr match the partition key:
if (equal(lfirst(lc), expr))
    return cnt;

Although We can fix this issue like [1], I think why not directly use the
partkey->partcollation[cnt], which its value is
same with pg_partitioned_table's partcollation. I tried this to fix [1],
but at that time, I was unsure if it was the correct fix.

Until I find this thread issue, I think we should do it this way.
In the attached patch, I include this thread test and [1] test case.

I have two questions in my head:
1.  Does partition pruning logic still check the collation match with this
patch.
2. icu can work on all platform?

Any thoughts?

[1]
https://www.postgresql.org/message-id/18568-2a9afb6b9f7e6ed3%40postgresql.org
-- 
Thanks,
Tender Wang

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

    Tender Wang <tndrwang@gmail.com> — 2024-10-24T02:10:38Z

    Tender Wang <tndrwang@gmail.com> 于2024年10月23日周三 21:48写道：

> Hi all,
>
>
> I think the root cause of this thread and [1] are same.  We don't use the
> Partition Key collation but column's
> collation to fill the RelOptInfo partexprs field in
> set_baserel_partition_key_exprs().
> If the Partition Key definition is same as column definition,  which most
> times is,
> that will be ok. But if it's not, this thread issue will arise.
>
> As far as I know, only partition pruning logic considers not only call
> equal(), but also check collation match.
> Other codes only call equal() to check if the exprs match the partition
> key.
> For example, in this thread case, match_expr_to_partition_keys() think the
> expr match the partition key:
> if (equal(lfirst(lc), expr))
>     return cnt;
>
> Although We can fix this issue like [1], I think why not directly use the
> partkey->partcollation[cnt], which its value is
> same with pg_partitioned_table's partcollation. I tried this to fix [1],
> but at that time, I was unsure if it was the correct fix.
>
> Until I find this thread issue, I think we should do it this way.
> In the attached patch, I include this thread test and [1] test case.
>
>
Hi
In the last email, I proposed to use partcollation directly. But I ignore
the case that partkey is not a simple column reference.
for expample:
create table coll_pruning_multi (a text) partition by range (substr(a,
1) collate "POSIX", substr(a, 1) collate "C");

The partexprs in RelOptInfo store substr(a,1), a FuncExpr, and its
collation is same with column a not collate "posix".

I'm now thinking how we can use a uniform solution to fix this thread issue
and issue in [1]

[1]
https://www.postgresql.org/message-id/18568-2a9afb6b9f7e6ed3%40postgresql.org
-- 
Thanks,
Tender Wang

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

    Tender Wang <tndrwang@gmail.com> — 2024-10-24T04:46:23Z

    Tender Wang <tndrwang@gmail.com> 于2024年10月23日周三 21:48写道：

> Hi all,
>
> I find another issue as $SUBJECT when I work on [1].
>

When I continue to work on this, I find below issue. But I'm not sure
whether it is a bug.

postgres=# create table part_index(a text primary key) partition by list (
a collate "POSIX");
ERROR:  unique constraint on partitioned table must include all
partitioning columns
DETAIL:  PRIMARY KEY constraint on table "part_index" lacks column "a"
which is part of the partition key.
postgres=# create table part_index(a text) partition by list ( a collate
"POSIX");
CREATE TABLE
postgres=# alter table part_index add primary key (a);
ERROR:  unique constraint on partitioned table must include all
partitioning columns
DETAIL:  PRIMARY KEY constraint on table "part_index" lacks column "a"
which is part of the partition key.

It seems we can't create a primary key if the collation is different
between columnDef and PartitionKey.

By the way, I think the error message is misleading to users.
ostgres=# alter table part_index add primary key (a);
ERROR:  unique constraint on partitioned table must include all
partitioning columns
DETAIL:  PRIMARY KEY constraint on table "part_index" lacks column "a"
which is part of the partition key.



-- 
Thanks,
Tender Wang

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

    Amit Langote <amitlangote09@gmail.com> — 2024-10-24T06:33:13Z

    Hi,

On Thu, Oct 24, 2024 at 1:46 PM Tender Wang <tndrwang@gmail.com> wrote:
> Tender Wang <tndrwang@gmail.com> 于2024年10月23日周三 21:48写道：
>>
>> Hi all,
>>
>> I find another issue as $SUBJECT when I work on [1].
>
> When I continue to work on this, I find below issue. But I'm not sure whether it is a bug.
>
> postgres=# create table part_index(a text primary key) partition by list ( a collate "POSIX");
> ERROR:  unique constraint on partitioned table must include all partitioning columns
> DETAIL:  PRIMARY KEY constraint on table "part_index" lacks column "a" which is part of the partition key.
> postgres=# create table part_index(a text) partition by list ( a collate "POSIX");
> CREATE TABLE
> postgres=# alter table part_index add primary key (a);
> ERROR:  unique constraint on partitioned table must include all partitioning columns
> DETAIL:  PRIMARY KEY constraint on table "part_index" lacks column "a" which is part of the partition key.
>
> It seems we can't create a primary key if the collation is different between columnDef and PartitionKey.

Yeah, you don't want to have the PK index and the partitioning logic
to not be in sync about the collation rules applied to the individual
rows.

> By the way, I think the error message is misleading to users.
> ostgres=# alter table part_index add primary key (a);
> ERROR:  unique constraint on partitioned table must include all partitioning columns
> DETAIL:  PRIMARY KEY constraint on table "part_index" lacks column "a" which is part of the partition key.

I think it's kind of similar to the message you get when a GROUP BY
column's collation doesn't match the column appearing in the SELECT
list:

explain SELECT c collate case_insensitive, count(c) FROM
pagg_tab_case_s GROUP BY c collate "C";
ERROR:  column "pagg_tab_case_s.c" must appear in the GROUP BY clause
or be used in an aggregate function
LINE 1: explain SELECT c collate case_insensitive, count(c) FROM pag...

Perhaps it would be more helpful for the error message or hint or
detail to mention the actual discrepancy (collation mismatch) that's
causing the error.

There might be other instances of such an error and I am not sure it
would be worthwhile to find and fix them all.

-- 
Thanks, Amit Langote

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

    Tender Wang <tndrwang@gmail.com> — 2024-10-24T07:01:07Z

    Amit Langote <amitlangote09@gmail.com> 于2024年10月24日周四 14:33写道：

> Hi,
>
> On Thu, Oct 24, 2024 at 1:46 PM Tender Wang <tndrwang@gmail.com> wrote:
> > Tender Wang <tndrwang@gmail.com> 于2024年10月23日周三 21:48写道：
> >>
> >> Hi all,
> >>
> >> I find another issue as $SUBJECT when I work on [1].
> >
> > When I continue to work on this, I find below issue. But I'm not sure
> whether it is a bug.
> >
> > postgres=# create table part_index(a text primary key) partition by list
> ( a collate "POSIX");
> > ERROR:  unique constraint on partitioned table must include all
> partitioning columns
> > DETAIL:  PRIMARY KEY constraint on table "part_index" lacks column "a"
> which is part of the partition key.
> > postgres=# create table part_index(a text) partition by list ( a collate
> "POSIX");
> > CREATE TABLE
> > postgres=# alter table part_index add primary key (a);
> > ERROR:  unique constraint on partitioned table must include all
> partitioning columns
> > DETAIL:  PRIMARY KEY constraint on table "part_index" lacks column "a"
> which is part of the partition key.
> >
> > It seems we can't create a primary key if the collation is different
> between columnDef and PartitionKey.
>
> Yeah, you don't want to have the PK index and the partitioning logic
> to not be in sync about the collation rules applied to the individual
> rows.
>
> > By the way, I think the error message is misleading to users.
> > ostgres=# alter table part_index add primary key (a);
> > ERROR:  unique constraint on partitioned table must include all
> partitioning columns
> > DETAIL:  PRIMARY KEY constraint on table "part_index" lacks column "a"
> which is part of the partition key.
>
> I think it's kind of similar to the message you get when a GROUP BY
> column's collation doesn't match the column appearing in the SELECT
> list:
>
> explain SELECT c collate case_insensitive, count(c) FROM
> pagg_tab_case_s GROUP BY c collate "C";
> ERROR:  column "pagg_tab_case_s.c" must appear in the GROUP BY clause
> or be used in an aggregate function
> LINE 1: explain SELECT c collate case_insensitive, count(c) FROM pag...
>
> Perhaps it would be more helpful for the error message or hint or
> detail to mention the actual discrepancy (collation mismatch) that's
> causing the error.
>
> There might be other instances of such an error and I am not sure it
> would be worthwhile to find and fix them all.
>
>
Thanks for the explanation.   We had better focus on the wrong result issue.

I feel that it's hard only to use one struct(for example, X), which just
calls equal(X, expr)
can check both the expression match and the collation match.

Maybe we should add another collation match checks in
match_clause_to_partition_key(), like
partition pruning logic does.

Any thoughts?

-- 
Thanks,
Tender Wang

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

    jian he <jian.universality@gmail.com> — 2024-10-29T06:15:27Z

    On Thu, Oct 24, 2024 at 3:01 PM Tender Wang <tndrwang@gmail.com> wrote:
>
> I feel that it's hard only to use one struct(for example, X), which just calls equal(X, expr)
> can check both the expression match and the collation match.
>

in RelOptInfo->partexprs, maybe we should mention that the partition
key collation is stored
in RelOptInfo->part_scheme, not here.

> Maybe we should add another collation match checks in match_clause_to_partition_key(), like
> partition pruning logic does.
>
in match_clause_to_partition_key
we already have

else if (IsA(clause, OpExpr) &&
list_length(((OpExpr *) clause)->args) == 2)
{
        /*
         * Partition key match also requires collation match.  There may be
         * multiple partkeys with the same expression but different
         * collations, so failure is NOMATCH.
         */
        if (!PartCollMatchesExprColl(partcoll, opclause->inputcollid))
            return PARTCLAUSE_NOMATCH;
}
else if (IsA(clause, ScalarArrayOpExpr))
{
        if (!equal(leftop, partkey) ||
            !PartCollMatchesExprColl(partcoll, saop->inputcollid))
            return PARTCLAUSE_NOMATCH;
}
So I think match_clause_to_partition_key handling collation is fine.

I think the problem is match_expr_to_partition_keys
don't have a collation related check.

CREATE TABLE pagg_join1 (c text collate case_insensitive) PARTITION BY
LIST(c collate "C");
CREATE TABLE pagg_join2 (c text collate "C") PARTITION BY LIST(c
collate case_insensitive);
CREATE TABLE pagg_join3 (c text collate "POSIX") PARTITION BY LIST(c
collate "C");
CREATE TABLE pagg_join4 (c text collate case_insensitive) PARTITION BY
LIST(c collate ignore_accents);

Our partition-wise join is based on Equi-join [1].
In some cases,column and partitionkey collation are different,
but if these two collations are deterministic, then texteq should work
as expected.
So I think, pagg_join3 can do partition-wise join,
I think pagg_join2 can do partition-wise join also.

we can let all (pagg_join1, pagg_join2, pagg_join3, pagg_join4) cannot
do partition-wise join (join with themself),
or we can let pagg_join2, pagg_join3 do partition-wise join (join with
themself).


POC attached, will let pagg_join2, pagg_join3 do partition-wise join.


[1] https://en.wikipedia.org/wiki/Join_%28SQL%29#Equi-join

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

    Tender Wang <tndrwang@gmail.com> — 2024-10-29T06:38:30Z

    jian he <jian.universality@gmail.com> 于2024年10月29日周二 14:15写道：

> On Thu, Oct 24, 2024 at 3:01 PM Tender Wang <tndrwang@gmail.com> wrote:
> >
> > I feel that it's hard only to use one struct(for example, X), which just
> calls equal(X, expr)
> > can check both the expression match and the collation match.
> >
>
> in RelOptInfo->partexprs, maybe we should mention that the partition
> key collation is stored
> in RelOptInfo->part_scheme, not here.
>
> > Maybe we should add another collation match checks in
> match_clause_to_partition_key(), like
> > partition pruning logic does.
> >
> in match_clause_to_partition_key
> we already have
>
> else if (IsA(clause, OpExpr) &&
> list_length(((OpExpr *) clause)->args) == 2)
> {
>         /*
>          * Partition key match also requires collation match.  There may be
>          * multiple partkeys with the same expression but different
>          * collations, so failure is NOMATCH.
>          */
>         if (!PartCollMatchesExprColl(partcoll, opclause->inputcollid))
>             return PARTCLAUSE_NOMATCH;
> }
> else if (IsA(clause, ScalarArrayOpExpr))
> {
>         if (!equal(leftop, partkey) ||
>             !PartCollMatchesExprColl(partcoll, saop->inputcollid))
>             return PARTCLAUSE_NOMATCH;
> }
> So I think match_clause_to_partition_key handling collation is fine.
>
> I think the problem is match_expr_to_partition_keys
> don't have a collation related check.
>

Sorry, it's a typo. It should be  match_expr_to_partition_keys().


> CREATE TABLE pagg_join1 (c text collate case_insensitive) PARTITION BY
> LIST(c collate "C");
> CREATE TABLE pagg_join2 (c text collate "C") PARTITION BY LIST(c
> collate case_insensitive);
> CREATE TABLE pagg_join3 (c text collate "POSIX") PARTITION BY LIST(c
> collate "C");
> CREATE TABLE pagg_join4 (c text collate case_insensitive) PARTITION BY
> LIST(c collate ignore_accents);
>
> Our partition-wise join is based on Equi-join [1].
> In some cases,column and partitionkey collation are different,
> but if these two collations are deterministic, then texteq should work
> as expected.
> So I think, pagg_join3 can do partition-wise join,
> I think pagg_join2 can do partition-wise join also.
>
> we can let all (pagg_join1, pagg_join2, pagg_join3, pagg_join4) cannot
> do partition-wise join (join with themself),
> or we can let pagg_join2, pagg_join3 do partition-wise join (join with
> themself).
>
>
> POC attached, will let pagg_join2, pagg_join3 do partition-wise join.
>

Hmm, I'm not sure


>
> [1] https://en.wikipedia.org/wiki/Join_%28SQL%29#Equi-join
>


-- 
Thanks,
Tender Wang

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

    jian he <jian.universality@gmail.com> — 2024-10-30T03:57:35Z

    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.

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

    Junwang Zhao <zhjwpku@gmail.com> — 2024-10-30T12:36:25Z

    On Wed, Oct 30, 2024 at 11:58 AM jian he <jian.universality@gmail.com> 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

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

    Amit Langote <amitlangote09@gmail.com> — 2024-10-31T13:08:58Z

    On Wed, Oct 30, 2024 at 9:36 PM Junwang Zhao <zhjwpku@gmail.com> wrote:
> On Wed, Oct 30, 2024 at 11:58 AM jian he <jian.universality@gmail.com> 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

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

    Tender Wang <tndrwang@gmail.com> — 2024-11-01T02:39:35Z

    Amit Langote <amitlangote09@gmail.com> 于2024年10月31日周四 21:09写道：

> On Wed, Oct 30, 2024 at 9:36 PM Junwang Zhao <zhjwpku@gmail.com> wrote:
> > On Wed, Oct 30, 2024 at 11:58 AM jian he <jian.universality@gmail.com>
> 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

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

    Tender Wang <tndrwang@gmail.com> — 2024-11-01T02:59:21Z

    Amit Langote <amitlangote09@gmail.com> 于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

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

    jian he <jian.universality@gmail.com> — 2024-11-01T05:39:11Z

    On Thu, Oct 31, 2024 at 9:09 PM Amit Langote <amitlangote09@gmail.com> 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;

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

    Amit Langote <amitlangote09@gmail.com> — 2024-11-01T06:55:19Z

    Hi,

On Fri, Nov 1, 2024 at 11:39 AM Tender Wang <tndrwang@gmail.com> wrote:
> Amit Langote <amitlangote09@gmail.com> 于2024年10月31日周四 21:09写道：
>> On Wed, Oct 30, 2024 at 9:36 PM Junwang Zhao <zhjwpku@gmail.com> wrote:
>> > On Wed, Oct 30, 2024 at 11:58 AM jian he <jian.universality@gmail.com> 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

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

    Amit Langote <amitlangote09@gmail.com> — 2024-11-01T07:25:28Z

    Hi,

On Fri, Nov 1, 2024 at 2:39 PM jian he <jian.universality@gmail.com> wrote:
> On Thu, Oct 31, 2024 at 9:09 PM Amit Langote <amitlangote09@gmail.com> 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

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

    Amit Langote <amitlangote09@gmail.com> — 2024-11-01T07:26:45Z

    Hi,

On Wed, Oct 23, 2024 at 10:48 PM Tender Wang <tndrwang@gmail.com> wrote:
> I think the root cause of this thread and [1] are same.  We don't use the Partition Key collation but column's
> collation to fill the RelOptInfo partexprs field in set_baserel_partition_key_exprs().
> If the Partition Key definition is same as column definition,  which most times is,
> that will be ok. But if it's not, this thread issue will arise.
>
> As far as I know, only partition pruning logic considers not only call equal(), but also check collation match.
> Other codes only call equal() to check if the exprs match the partition key.
> For example, in this thread case, match_expr_to_partition_keys() think the expr match the partition key:
> if (equal(lfirst(lc), expr))
>     return cnt;
>
> Although We can fix this issue like [1], I think why not directly use the partkey->partcollation[cnt], which its value is
> same with pg_partitioned_table's partcollation. I tried this to fix [1], but at that time, I was unsure if it was the correct fix.

I think it would be better to keep RelOptInfo.partexprs unchanged in
these fixes.  I haven't been able to come up with a way to "assign"
the correct collation to partition keys that are not simple column
references.  Checking PartitionScheme.partcollation separately is
enough to fix these bugs and aligns with the style of existing code,
such as match_clause_to_partition_key() in partprune.c.


--
Thanks, Amit Langote

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

    Tender Wang <tndrwang@gmail.com> — 2024-11-01T07:39:59Z

    Amit Langote <amitlangote09@gmail.com> 于2024年11月1日周五 15:27写道：

> Hi,
>
> On Wed, Oct 23, 2024 at 10:48 PM Tender Wang <tndrwang@gmail.com> wrote:
> > I think the root cause of this thread and [1] are same.  We don't use
> the Partition Key collation but column's
> > collation to fill the RelOptInfo partexprs field in
> set_baserel_partition_key_exprs().
> > If the Partition Key definition is same as column definition,  which
> most times is,
> > that will be ok. But if it's not, this thread issue will arise.
> >
> > As far as I know, only partition pruning logic considers not only call
> equal(), but also check collation match.
> > Other codes only call equal() to check if the exprs match the partition
> key.
> > For example, in this thread case, match_expr_to_partition_keys() think
> the expr match the partition key:
> > if (equal(lfirst(lc), expr))
> >     return cnt;
> >
> > Although We can fix this issue like [1], I think why not directly use
> the partkey->partcollation[cnt], which its value is
> > same with pg_partitioned_table's partcollation. I tried this to fix [1],
> but at that time, I was unsure if it was the correct fix.
>
> I think it would be better to keep RelOptInfo.partexprs unchanged in
> these fixes.  I haven't been able to come up with a way to "assign"
> the correct collation to partition keys that are not simple column
> references.  Checking PartitionScheme.partcollation separately is
> enough to fix these bugs and aligns with the style of existing code,
> such as match_clause_to_partition_key() in partprune.c.
>

Agree. I can't figure out another solution.


-- 
Thanks,
Tender Wang

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

    jian he <jian.universality@gmail.com> — 2024-11-01T08:07:00Z

    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
?

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

    Amit Langote <amitlangote09@gmail.com> — 2024-11-01T10:08:00Z

    On Fri, Nov 1, 2024 at 5:08 PM jian he <jian.universality@gmail.com> 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

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

    jian he <jian.universality@gmail.com> — 2024-11-04T03:27:49Z

    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.

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

    Amit Langote <amitlangote09@gmail.com> — 2024-11-05T07:55:25Z

    Hi,

On Mon, Nov 4, 2024 at 12:28 PM jian he <jian.universality@gmail.com> 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

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

    jian he <jian.universality@gmail.com> — 2024-11-06T03:19:45Z

    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;

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

    Amit Langote <amitlangote09@gmail.com> — 2024-11-08T09:27:32Z

    Hi,

On Wed, Nov 6, 2024 at 12:19 PM jian he <jian.universality@gmail.com> 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