On Wed, Oct 16, 2019 at 6:20 PM Etsuro Fujita <etsuro.fuj...@gmail.com>
wrote:
> On Wed, Sep 25, 2019 at 12:59 AM Etsuro Fujita <etsuro.fuj...@gmail.com>
> wrote:
> > I will continue to review the rest of the patch.
>
> I've been reviewing the rest of the patch. Here are my review comments:
>
[....]
> So I'd like to propose to introduce separate functions like
> process_outer_partition() and process_inner_partition() in the
> attached, instead of handle_missing_partition(). (I added a fast path
> to these functions that if both outer/inner sides have the default
> partitions, give up on merging partitions. Also, I modified the
> caller sites of proposed functions so that they call these if
> necessary.)
>
Agree -- process_outer_partition() & process_inner_partition() approach
looks
much cleaner than before.
Here are the few comments:
Note that LHS numbers are the line numbers in your previously posted
patch[1].
455 + if (inner_has_default ||
456 + jointype == JOIN_LEFT ||
457 + jointype == JOIN_ANTI ||
458 + jointype == JOIN_FULL)
459 + {
460 + if (!process_outer_partition(&outer_map,
512 + if (outer_has_default || jointype == JOIN_FULL)
513 + {
514 + if (!process_inner_partition(&outer_map,
How about adding these conditions to the else block of
process_outer_partition()
& process_inner_partition() function respectively so that these functions
can be
called unconditionally? Thoughts/Comments?
---
456 + jointype == JOIN_LEFT ||
457 + jointype == JOIN_ANTI ||
458 + jointype == JOIN_FULL)
Also, how about using IS_OUTER_JOIN() instead. But we need an assertion to
restrict JOIN_RIGHT or something.
For the convenience, I did both aforesaid changes in the attached delta
patch that
can be applied atop of your previously posted patch[1]. Kindly have a look
& share
your thoughts, thanks.
--
1273 + * *next_index is incremented when creating a new merged partition
associated
1274 + * with the given outer partition.
1275 + */
Correction: s/outer/inner
---
1338 + * In range partitioning, if the given outer partition is
already
1339 + * merged (eg, because we found an overlapping range earlier),
we know
1340 + * where it fits in the join result; nothing to do in that
case. Else
1341 + * create a new merged partition.
Correction: s/outer/inner.
---
1712 /*
1713 * If the NULL partition was missing from the inner side of
the join,
s/inner side/outer side
--
Regards,
Amul
1]
https://postgr.es/m/CAPmGK145V8DNCNQ2gQBgNE3QqoJGjsmK5WMwaA3FMirNM723KQ%40mail.gmail.com
From c7f165b575fd984ca3053ce7162bdd8e4bf56be8 Mon Sep 17 00:00:00 2001
From: Amul Sul <amul.sul@enterprisedb.com>
Date: Thu, 24 Oct 2019 05:38:11 -0400
Subject: [PATCH] delta
Changes :
1. Call process_outer_partition & process_inner_partition
unconditionally.
2. Used IS_OUTER_JOIN() instead of listing individual join type.
---
src/backend/partitioning/partbounds.c | 209 ++++++++++++--------------
1 file changed, 93 insertions(+), 116 deletions(-)
diff --git a/src/backend/partitioning/partbounds.c b/src/backend/partitioning/partbounds.c
index 4c3a31c82c0..b29e44e2f28 100644
--- a/src/backend/partitioning/partbounds.c
+++ b/src/backend/partitioning/partbounds.c
@@ -3601,24 +3601,18 @@ partition_range_bounds_merge(int partnatts, FmgrInfo *partsupfuncs,
}
else
{
- if (inner_has_default ||
- jointype == JOIN_LEFT ||
- jointype == JOIN_ANTI ||
- jointype == JOIN_FULL)
- {
- if (!process_outer_partition(&outer_map,
- &inner_map,
- outer_has_default,
- inner_has_default,
- outer_part,
- inner_default,
- jointype,
- outer_bi->strategy,
- &next_index,
- &default_index,
- &merged_index))
- return NULL;
- }
+ if (!process_outer_partition(&outer_map,
+ &inner_map,
+ outer_has_default,
+ inner_has_default,
+ outer_part,
+ inner_default,
+ jointype,
+ outer_bi->strategy,
+ &next_index,
+ &default_index,
+ &merged_index))
+ return NULL;
merged_lb = &outer_lb;
merged_ub = &outer_ub;
@@ -3640,21 +3634,18 @@ partition_range_bounds_merge(int partnatts, FmgrInfo *partsupfuncs,
}
else
{
- if (outer_has_default || jointype == JOIN_FULL)
- {
- if (!process_inner_partition(&outer_map,
- &inner_map,
- outer_has_default,
- inner_has_default,
- inner_part,
- outer_default,
- jointype,
- outer_bi->strategy,
- &next_index,
- &default_index,
- &merged_index))
- return NULL;
- }
+ if (!process_inner_partition(&outer_map,
+ &inner_map,
+ outer_has_default,
+ inner_has_default,
+ inner_part,
+ outer_default,
+ jointype,
+ outer_bi->strategy,
+ &next_index,
+ &default_index,
+ &merged_index))
+ return NULL;
merged_lb = &inner_lb;
merged_ub = &inner_ub;
@@ -3857,27 +3848,18 @@ partition_list_bounds_merge(FmgrInfo *partsupfunc, Oid *partcollation,
/* A datum missing from the inner side. */
merged_datum = odatums;
- if (inner_has_default ||
- jointype == JOIN_LEFT ||
- jointype == JOIN_ANTI ||
- jointype == JOIN_FULL)
- {
- int o_index = outer_bi->indexes[cnto];
-
- Assert(o_index >= 0);
- if (!process_outer_partition(&outer_map,
- &inner_map,
- outer_has_default,
- inner_has_default,
- o_index,
- inner_default,
- jointype,
- outer_bi->strategy,
- &next_index,
- &default_index,
- &merged_index))
- return NULL;
- }
+ if (!process_outer_partition(&outer_map,
+ &inner_map,
+ outer_has_default,
+ inner_has_default,
+ outer_bi->indexes[cnto],
+ inner_default,
+ jointype,
+ outer_bi->strategy,
+ &next_index,
+ &default_index,
+ &merged_index))
+ return NULL;
/* Move to the next datum on the outer side. */
cnto++;
@@ -3890,24 +3872,18 @@ partition_list_bounds_merge(FmgrInfo *partsupfunc, Oid *partcollation,
/* A datum missing from the outer side. */
merged_datum = idatums;
- if (outer_has_default || jointype == JOIN_FULL)
- {
- int i_index = inner_bi->indexes[cnti];
-
- Assert(i_index >= 0);
- if (!process_inner_partition(&outer_map,
- &inner_map,
- outer_has_default,
- inner_has_default,
- i_index,
- outer_default,
- jointype,
- outer_bi->strategy,
- &next_index,
- &default_index,
- &merged_index))
- return NULL;
- }
+ if (!process_inner_partition(&outer_map,
+ &inner_map,
+ outer_has_default,
+ inner_has_default,
+ inner_bi->indexes[cnti],
+ outer_default,
+ jointype,
+ outer_bi->strategy,
+ &next_index,
+ &default_index,
+ &merged_index))
+ return NULL;
/* Move to the next datum on the inner side. */
cnti++;
@@ -4171,6 +4147,8 @@ process_outer_partition(PartitionMap *outer_map,
int *default_index,
int *merged_index)
{
+ Assert(*merged_index == -1);
+
/*
* If the inner side has the default partition, the outer partition has to
* be joined with the default partition; try merging them. Otherwise, we
@@ -4189,6 +4167,7 @@ process_outer_partition(PartitionMap *outer_map,
if (outer_has_default)
return false;
+ Assert(outer_index >= 0);
*merged_index = map_and_merge_partitions(outer_map, inner_map,
outer_index, inner_default,
next_index);
@@ -4210,17 +4189,17 @@ process_outer_partition(PartitionMap *outer_map,
*merged_index = -1;
}
}
- else
+ else if (IS_OUTER_JOIN(jointype))
{
- Assert(jointype == JOIN_LEFT || jointype == JOIN_ANTI ||
- jointype == JOIN_FULL);
-
+ Assert (jointype == JOIN_LEFT || jointype == JOIN_ANTI ||
+ jointype == JOIN_FULL);
/*
* In range partitioning, if the given outer partition is already
* merged (eg, because we found an overlapping range earlier), we know
* where it fits in the join result; nothing to do in that case. Else
* create a new merged partition.
*/
+ Assert(outer_index >= 0);
if (outer_map->merged_indexes[outer_index] >= 0)
{
if (strategy == PARTITION_STRATEGY_LIST)
@@ -4259,6 +4238,8 @@ process_inner_partition(PartitionMap *outer_map,
int *default_index,
int *merged_index)
{
+ Assert(*merged_index == -1);
+
/*
* If the outer side has the default partition, the inner partition has to
* be joined with the default partition; try merging them. Otherwise, we
@@ -4277,6 +4258,7 @@ process_inner_partition(PartitionMap *outer_map,
if (inner_has_default)
return false;
+ Assert(inner_index >= 0);
*merged_index = map_and_merge_partitions(outer_map, inner_map,
outer_default, inner_index,
next_index);
@@ -4288,9 +4270,11 @@ process_inner_partition(PartitionMap *outer_map,
* default partition of the join; record the index in *default_index
* if not done yet.
*/
- if (jointype == JOIN_LEFT || jointype == JOIN_ANTI ||
- jointype == JOIN_FULL)
+ if (IS_OUTER_JOIN(jointype))
{
+ Assert(jointype == JOIN_LEFT || jointype == JOIN_ANTI ||
+ jointype == JOIN_FULL);
+
if (*default_index == -1)
*default_index = *merged_index;
else
@@ -4299,16 +4283,15 @@ process_inner_partition(PartitionMap *outer_map,
*merged_index = -1;
}
}
- else
+ else if (jointype == JOIN_FULL)
{
- Assert(jointype == JOIN_FULL);
-
/*
* In range partitioning, if the given outer partition is already
* merged (eg, because we found an overlapping range earlier), we know
* where it fits in the join result; nothing to do in that case. Else
* create a new merged partition.
*/
+ Assert(inner_index >= 0);
if (inner_map->merged_indexes[inner_index] >= 0)
{
if (strategy == PARTITION_STRATEGY_LIST)
@@ -4528,11 +4511,12 @@ merge_default_partitions(PartitionMap *outer_map, PartitionMap *inner_map,
if (outer_has_default && !inner_has_default)
{
- if (jointype == JOIN_LEFT || jointype == JOIN_FULL ||
- jointype == JOIN_ANTI)
+ if (IS_OUTER_JOIN(jointype))
{
int merged_index;
+ Assert (jointype == JOIN_LEFT || jointype == JOIN_FULL ||
+ jointype == JOIN_ANTI);
Assert(outer_default >= 0 && outer_default < outer_map->nparts);
merged_index = outer_map->merged_indexes[outer_default];
if (merged_index == -1)
@@ -4621,24 +4605,19 @@ merge_null_partitions(PartitionBoundInfo outer_bi, PartitionBoundInfo inner_bi,
* contain the NULL values and thus become the NULL partition of the
* the join.
*/
- if (inner_has_default ||
- jointype == JOIN_LEFT ||
- jointype == JOIN_ANTI ||
- jointype == JOIN_FULL)
- {
- if (!process_outer_partition(outer_map,
- inner_map,
- outer_has_default,
- inner_has_default,
- outer_bi->null_index,
- inner_bi->default_index,
- jointype,
- outer_bi->strategy,
- next_index,
- default_index,
- &merged_index))
- return false;
- }
+ if (!process_outer_partition(outer_map,
+ inner_map,
+ outer_has_default,
+ inner_has_default,
+ outer_bi->null_index,
+ inner_bi->default_index,
+ jointype,
+ outer_bi->strategy,
+ next_index,
+ default_index,
+ &merged_index))
+ return false;
+
*null_index = merged_index;
}
else if (!outer_has_null && inner_has_null)
@@ -4651,21 +4630,19 @@ merge_null_partitions(PartitionBoundInfo outer_bi, PartitionBoundInfo inner_bi,
* contain the NULL values and thus become the NULL partition of the
* the join.
*/
- if (outer_has_default || jointype == JOIN_FULL)
- {
- if (!process_inner_partition(outer_map,
- inner_map,
- outer_has_default,
- inner_has_default,
- inner_bi->null_index,
- outer_bi->default_index,
- jointype,
- outer_bi->strategy,
- next_index,
- default_index,
- &merged_index))
- return false;
- }
+ if (!process_inner_partition(outer_map,
+ inner_map,
+ outer_has_default,
+ inner_has_default,
+ inner_bi->null_index,
+ outer_bi->default_index,
+ jointype,
+ outer_bi->strategy,
+ next_index,
+ default_index,
+ &merged_index))
+ return false;
+
*null_index = merged_index;
}
else
--
2.18.0
