On Thu, Sep 19, 2019 at 4:15 PM Amit Langote <amitlangot...@gmail.com> wrote:
> Hi Richard, > > 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 <ri...@pivotal.io> wrote: > >> On Wed, Sep 4, 2019 at 10:01 AM Amit Langote <amitlangot...@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. > > Please find attached updated patches. > Thank you for the fix. Will review. Thanks Richard
