Hi Tomas and Ashutosh, On Thu, Apr 2, 2020 at 1:51 AM Ashutosh Bapat <ashutosh.ba...@2ndquadrant.com> wrote: > On Thu, 26 Mar 2020 at 05:47, Tomas Vondra <tomas.von...@2ndquadrant.com> > wrote:
>> three more comments after eye-balling the code for a bit longer. >> >> 1) The patch probably needs to tweak config.sgml which says this about >> the enable_partitionwise_join GUC: >> >> .. Partitionwise join currently applies only when the join conditions >> include all the partition keys, which must be of the same data type >> and have exactly matching sets of child partitions. .. > > > Done. Actually this wasn't updated when partition pruning was introduced, > which could cause a partitionwise join to be not used even when those > conditions were met. Similarly when a query involved whole row reference. > It's hard to explain all the conditions under which partitionwise join > technique will be used. But I have tried to keep it easy to understand. > >> >> >> Which is probably incorrect, because the point of this patch is not to >> require exact match of the partitions, right? >> >> 2) Do we really need the 'merged' flag in try_partitionwise_join? Can't >> we simply use the joinrel->merged flag directly? ISTM the we always >> start with joinrel->merged=false, and then only ever set it to true in >> some cases. I've tried doing that, per the attached 0002 patch. The >> regression tests seem to work fine. > > > Thanks. I just added a small prologue to compute_partition_bounds(). > >> >> >> I noticed this because I've tried moving part of the function into a >> separate function, and removing the variable makes that simpler. >> >> The patch also does a couple additional minor tweaks. >> >> 3) I think the for nparts comment is somewhat misleading: >> >> int nparts; /* number of partitions; 0 = not partitioned; >> * -1 = not yet set */ >> >> which says that nparts=0 means not partitioned. But then there are >> conditions like this: >> >> /* Nothing to do, if the join relation is not partitioned. */ >> if (joinrel->part_scheme == NULL || joinrel->nparts == 0) >> return; >> >> which (ignoring the obsolete comment) seems to say we can have nparts==0 >> even for partitioned tables, no? > > > See my previous mail. > >> >> >> Anyway, attached is the original patch (0001) and two patches with >> proposed changes. 0002 removes the "merged" flag as explained in (2), >> 0003 splits the try_partitionwise_join() function into two parts. >> >> I'm saying these changes have to happen and it's a bit crude (and it >> might be a bit of a bikeshedding). > > > I have added 0005 with the changes I described in this as well as the > previous mail. 0004 is just some white space fixes. Thanks for the comments, Tomas! Thanks for the patch, Ashutosh! I will look at the patch. Best regards, Etsuro Fujita
