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

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

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

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;

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

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
?

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;

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