Date: Wed, 25 Mar 2020 20:05:13 +0100
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>
Subject: Re: [HACKERS] advanced partition matching algorithm for
 partition-wise join
Message-ID: <20200325190513.rmxr7swainc64io5@development>
References: <4F6838E1-0FE8-4B92-B126-809866A064DE@enterprisedb.com>
 <CAPmGK14H4UFcrDFDS-vviBptxD356YvnFZjdtR5cbELoK4_cKA@mail.gmail.com>
 <CAPmGK14gTKUL4Enq-sYX1dVkySOK7JVPuEfUDcrr5P+3NHVQGg@mail.gmail.com>
 <768B0ED5-F2AA-4CB8-8DA3-41E2148CF42A@enterprisedb.com>
 <CAPmGK15kZug9=SfwCjtRsDH5epxH2pUkky2g3TMszuNZMvwkZQ@mail.gmail.com>
 <CAPmGK15ZpZ_CTDhAuLjFRAZNA0k0jM2WEej8jOQhabq_ZCVx4Q@mail.gmail.com>
 <CAExHW5uZAzhNYj9hhgoKj1d3mqJZH6yGqvY=ecBFk9W=7zL9yA@mail.gmail.com>
 <CAPmGK15L2RshRJA2y3zSi6EKKPe2wOKT56od2KRR8H3M4cqOFg@mail.gmail.com>
 <CAExHW5staLVrZ_B+ks-S51VPBexQX2rPa7vuaYnj1WdoKhROKQ@mail.gmail.com>
 <CAPmGK14dnW-yhwvCTjC7dACvMWefwASiAKxbuUOxxjdRLcsLAg@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii; format=flowed
Content-Disposition: inline
In-Reply-To: 
 <CAPmGK14dnW-yhwvCTjC7dACvMWefwASiAKxbuUOxxjdRLcsLAg@mail.gmail.com>
Precedence: bulk

Hi,

I've started reviewing the patch a couple days ago. I haven't done any
extensive testing, but I do have a bunch of initial comments that I can
share now.

1) I wonder if this needs to update src/backend/optimizer/README, which
does have a section about partitionwise joins. It seems formulated in a
way that that probably covers even this more advanced algorithm, but
maybe it should mention handling of default partitions etc.?

There certainly needs to be some description of the algorithm somewhere,
either in a README or before a suitable function. It doesn't have to be
particularly detailed, a rough outline of the matching would be enough,
so that readers don't have to rebuild the knowledge from pieces
scattered around various comments.

2) Do we need another GUC enabling this more complex algorithm? In PG11
the partitionwise join is disabled by default, on the grounds that it's
expensive and not worth it by default. How much more expensive is this?
Maybe it makes sense to allow enabling only the "simple" approach?

3) This comment in try_partitionwise_join is now incorrect, because the
condition may be true even for partitioned tables with (nparts == 0).

     /* Nothing to do, if the join relation is not partitioned. */
     if (joinrel->part_scheme == NULL || joinrel->nparts == 0)
         return;

Moreover, the condition used to be 

     if (!IS_PARTITIONED_REL(joinrel))
         return;

which is way more readable. I think it's net negative to replace these
"nice" macros with clear meaning with complex conditions. If needed, we
can invent new macros. There are many other places where the patch
replaces macros with less readable conditions.


4) I'm a bit puzzled how we could get here with non-partitioned rels?

     /*
      * We can not perform partitionwise join if either of the joining relations
      * is not partitioned.
      */
     if (!IS_PARTITIONED_REL(rel1) || !IS_PARTITIONED_REL(rel2))
         return;

5) I find the "merged" flag in RelOptInfo rather unclear, because it
does not clearly indicate what was merged. Maybe something like
partbounds_merged would be better?

6) The try_partitionwise_join function is getting a bit too long and
harder to understand. The whole block in

     if (joinrel->nparts == -1)
     {
         ...
     }

seems rather well isolated, so I propose to move it to a separate
function responsible only for the merging. We can simply call it on the
joinrel, and make it return right away if (joinrel->nparts == -1).

7) I'd suggest not to reference exact functions in comments unless
abolutely necessary, because it's harder to maintain and it does not
really explain purpose of the struct/code. E.g. consider this:

     /* Per-partitioned-relation data for merge_list_bounds()/merge_range_bounds() */
     typedef struct PartitionMap
     { ... }

Why does it matter where is the struct used? That's pretty trivial to
find using 'git grep' or something. Instead the comment should explain
the purpose of the struct.

regards

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