Re: [HACKERS] advanced partition matching algorithm forpartition-wise join

Hi,

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. ..

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.

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?

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).


regards

-- 
Tomas Vondra                  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services