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

On Thu, 26 Mar 2020 at 05:47, Tomas Vondra <tomas(dot)vondra(at)2ndquadrant(dot)com>
wrote:

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

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.

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

--
Best Wishes,
Ashutosh