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