Thanks a lot for taking a close look at the patch and sorry about the delay.
On Wed, Sep 4, 2019 at 5:29 PM Richard Guo <riguo@pivotal.io> wrote: >> On Wed, Sep 4, 2019 at 10:01 AM Amit Langote <amitlangote09@gmail.com> wrote: > I'm reviewing v2-0002 and I have concern about how COALESCE expr is > processed in match_join_arg_to_partition_keys(). > > If there is a COALESCE expr with first arg being non-partition key expr > and second arg being partition key, the patch would match it to the > partition key, which may result in wrong results in some cases. > > For instance, consider the partition table below: > > create table p (k int, val int) partition by range(k); > create table p_1 partition of p for values from (1) to (10); > create table p_2 partition of p for values from (10) to (100); > > So with patch v2-0002, the following query will be planned with > partitionwise join. > > # explain (costs off) > select * from (p as t1 full join p as t2 on t1.k = t2.k) as t12(k1,val1,k2,val2) > full join p as t3 on COALESCE(t12.val1, t12.k1) = t3.k; > QUERY PLAN > ---------------------------------------------------------- > Append > -> Hash Full Join > Hash Cond: (COALESCE(t1.val, t1.k) = t3.k) > -> Hash Full Join > Hash Cond: (t1.k = t2.k) > -> Seq Scan on p_1 t1 > -> Hash > -> Seq Scan on p_1 t2 > -> Hash > -> Seq Scan on p_1 t3 > -> Hash Full Join > Hash Cond: (COALESCE(t1_1.val, t1_1.k) = t3_1.k) > -> Hash Full Join > Hash Cond: (t1_1.k = t2_1.k) > -> Seq Scan on p_2 t1_1 > -> Hash > -> Seq Scan on p_2 t2_1 > -> Hash > -> Seq Scan on p_2 t3_1 > (19 rows) > > But as t1.val is not a partition key, actually we cannot use > partitionwise join here. > > If we insert below data into the table, we will get wrong results for > the query above. > > insert into p select 5,15; > insert into p select 15,5;
Good catch! It's quite wrong to use COALESCE(t12.val1, t12.k1) = t3.k for partitionwise join as the COALESCE expression might as well output the value of val1 which doesn't conform to partitioning.
I've fixed match_join_arg_to_partition_keys() to catch that case and fail. Added a test case as well.