Horton

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

Tomas Vondra <tomas.vondra@2ndquadrant.com>

View thread
From: Tomas Vondra <tomas.vondra@2ndquadrant.com>
To: Etsuro Fujita <etsuro.fujita@gmail.com>
Cc: Ashutosh Bapat <ashutosh.bapat.oss@gmail.com>, Mark Dilger <mark.dilger@enterprisedb.com>, amul sul <sulamul@gmail.com>, Robert Haas <robertmhaas@gmail.com>, Thomas Munro <thomas.munro@gmail.com>, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp>, Rajkumar Raghuwanshi <rajkumar.raghuwanshi@enterprisedb.com>, Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp>, Dmitry Dolgov <9erthalion6@gmail.com>, Antonin Houska <ah@cybertec.at>, PostgreSQL-development <pgsql-hackers@postgresql.org>, Ashutosh Bapat <ashutosh.bapat@2ndquadrant.com>
Date: 2020-03-26T00:17:17Z
Lists: pgsql-hackers

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Suppress unused-variable warning.

  2. Allow partitionwise joins in more cases.

  3. Avoid crash in partitionwise join planning under GEQO.

  4. Disable support for partitionwise joins in problematic cases.

  5. Add plan_cache_mode setting

  6. Add test for partitionwise join involving default partition.

  7. Revise API for partition_rbound_cmp/partition_rbound_datum_cmp.

Attachments

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