Date: Sun, 4 Aug 2019 02:29:25 -0400
From: Alvaro Herrera <alvherre@2ndquadrant.com>
To: Amit Langote <amitlangote09@gmail.com>
Cc: yuzuko <yuzukohosoya@gmail.com>, shawn wang <shawn.wang.pg@gmail.com>,
	Shawn Wang <shawn.wang@highgo.ca>,
	PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: Problem with default partition pruning
Message-ID: <20190804062925.GA5861@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="YZ5djTAD1cGYuMQK"
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: 
 <CA+HiwqG+nSD0vcJacArYgYcFVtpTJQ0fx1gBgoZkA_isKd6Z2w@mail.gmail.com>
User-Agent: Mutt/1.9.4 (2018-02-28)
Precedence: bulk


--YZ5djTAD1cGYuMQK
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit

On 2019-Jul-03, Amit Langote wrote:

> Hosoya-san,
> 
> Thanks for updating the patches.
> 
> I have no comment in particular about
> v4_default_partition_pruning.patch,

Cool, thanks.  I spent some time reviewing this patch (the first one)
and I propose the attached cosmetic changes.  Mostly they consist of a
few comment rewordings.

There is one Assert() that changed in a pretty significant way ...
innocent though the change looks.  The original (not Hosoya-san's
patch's fault) had an assertion which is being changed thus:

    minoff = 0;
    maxoff = boundinfo->ndatums;
	...
        if (partindices[minoff] < 0)
            minoff++;
        if (partindices[maxoff] < 0)
            maxoff--;
 
        result->scan_default = partition_bound_has_default(boundinfo);
-       Assert(minoff >= 0 && maxoff >= 0);
+       Assert(partindices[minoff] >= 0 &&
+              partindices[maxoff] >= 0);

Note that the original Assert() here was verifying whether minoff and
maxoff are both >= 0.  But that seems pretty useless since it seems
almost impossible to have them become that given what we do to them.
What I think this code *really* wants to check is whether *the partition
indexes* that they point to are not negative: the transformation that
the two "if" lines do means to ignore bounds that correspond to value
ranges uncovered by any partition.  And after the incr/decr operators,
we expect that the bounds will be those of existing partitions ... so
they shouldn't be -1.


Other changes are addition of braces to some one-line blocks that had
significant comments, and minor code rearrangements to make things look
more easily understandable.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

--YZ5djTAD1cGYuMQK
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment; filename="v4-delta.patch"

diff --git a/src/backend/partitioning/partprune.c b/src/backend/partitioning/partprune.c
index 533818ce34..bfc16409dd 100644
--- a/src/backend/partitioning/partprune.c
+++ b/src/backend/partitioning/partprune.c
@@ -714,7 +714,7 @@ get_matching_partitions(PartitionPruneContext *context, List *pruning_steps)
 	PruneStepResult **results,
 			   *final_result;
 	ListCell   *lc;
-	bool       scan_default;
+	bool		scan_default;
 
 	/* If there are no pruning steps then all partitions match. */
 	if (num_steps == 0)
@@ -771,21 +771,27 @@ get_matching_partitions(PartitionPruneContext *context, List *pruning_steps)
 	{
 		int			partindex = context->boundinfo->indexes[i];
 
-		/*
-		 * In range and hash partitioning cases, some index values may be -1,
-		 * indicating that no partition has been defined to accept a given
-		 * range of values (that is, the bound at given offset is the upper
-		 * bound of this unassigned range of values) or for a given remainder,
-		 * respectively.  As it's still part of the queried range of values,
-		 * add the default partition, if any.
-		 */
-		if (partindex >= 0)
-			result = bms_add_member(result, partindex);
-		else
+		if (partindex < 0)
+		{
+			/*
+			 * In range partitioning cases, if a partition index is -1 it
+			 * means that the bound at the offset is the upper bound for a
+			 * range not covered by any partition (other than a possible
+			 * default partition).  In hash partitioning, the same means no
+			 * partition has been defined for the corresponding remainder
+			 * value.
+			 *
+			 * In either case, the value is still part of the queried range of
+			 * values, so mark to scan the default partition if one exists.
+			 */
 			scan_default |= partition_bound_has_default(context->boundinfo);
+			continue;
+		}
+
+		result = bms_add_member(result, partindex);
 	}
 
-	/* Add the null and/or default partition if needed and if present. */
+	/* Add the null and/or default partition if needed and present. */
 	if (final_result->scan_null)
 	{
 		Assert(context->strategy == PARTITION_STRATEGY_LIST);
@@ -2623,17 +2629,13 @@ get_matching_list_bounds(PartitionPruneContext *context,
  * Each datum whose offset is in result is to be treated as the upper bound of
  * the partition that will contain the desired values.
  *
- * scan_default will be set in the returned struct, if the default partition
- * needs to be scanned, provided one exists at all.  Although note that we
- * intentionally don't set scan_default in this function if only because the
- * matching set of values, found by comparing the input values using the
- * specified opstrategy, contains unassigned portions of key space, which
- * we normally assume to belong to the default partition.  We don't infer
- * that until all steps have been executed, including any combination steps,
- * because even if such unassinged portion of key space appears to be part of
- * the set of queried values based on comparisons in the individual OpExprs,
- * the final set of bounds obtained after combining multiple OpExprs may
- * exclude it.
+ * scan_default is set in the returned struct if a default partition exists
+ * and we're absolutely certain that it needs to be scanned.  We do *not* set
+ * it just because values match portions of the key space uncovered by
+ * partitions other than default (space which we normally assume to belong to
+ * the default partition): the final set of bounds obtained after combining
+ * multiple pruning steps might exclude it, so we infer its inclusion
+ * elsewhere.
  *
  * 'opstrategy' if non-zero must be a btree strategy number.
  *
@@ -2689,13 +2691,15 @@ get_matching_range_bounds(PartitionPruneContext *context,
 	 */
 	if (nvalues == 0)
 	{
+		/* ignore key space not covered by any partitions */
 		if (partindices[minoff] < 0)
 			minoff++;
 		if (partindices[maxoff] < 0)
 			maxoff--;
 
 		result->scan_default = partition_bound_has_default(boundinfo);
-		Assert(minoff >= 0 && maxoff >= 0);
+		Assert(partindices[minoff] >= 0 &&
+			   partindices[maxoff] >= 0);
 		result->bound_offsets = bms_add_range(NULL, minoff, maxoff);
 
 		return result;
@@ -2896,16 +2900,18 @@ get_matching_range_bounds(PartitionPruneContext *context,
 
 					minoff = inclusive ? off : off + 1;
 				}
-
-				/*
-				 * lookup value falls in the range between some bounds in
-				 * boundinfo.  off would be the offset of the greatest bound
-				 * that is <= lookup value, so add off + 1 to the result
-				 * instead as the offset of the upper bound of the smallest
-				 * partition that may contain the lookup value.
-				 */
 				else
+				{
+
+					/*
+					 * lookup value falls in the range between some bounds in
+					 * boundinfo.  off would be the offset of the greatest
+					 * bound that is <= lookup value, so add off + 1 to the
+					 * result instead as the offset of the upper bound of the
+					 * smallest partition that may contain the lookup value.
+					 */
 					minoff = off + 1;
+				}
 			}
 			break;
 
@@ -2972,11 +2978,13 @@ get_matching_range_bounds(PartitionPruneContext *context,
 					maxoff = off;
 			}
 			else
+			{
 				/*
 				 * 'off' is -1 indicating that all bounds are greater, so just
 				 * set the first bound's offset as maxoff.
 				 */
 				maxoff = off + 1;
+			}
 			break;
 
 		default:
@@ -3265,23 +3273,24 @@ perform_pruning_combine_step(PartitionPruneContext *context,
 
 	/*
 	 * A combine step without any source steps is an indication to not perform
-	 * any partition pruning, we just return all datum offsets.
+	 * any partition pruning.  Return all datum indexes in that case.
 	 */
 	result = (PruneStepResult *) palloc0(sizeof(PruneStepResult));
 	if (list_length(cstep->source_stepids) == 0)
 	{
 		PartitionBoundInfo boundinfo = context->boundinfo;
+		int			rangemax;
 
 		/*
 		 * Add all valid offsets into the boundinfo->indexes array.  For range
 		 * partitioning, boundinfo->indexes contains (boundinfo->ndatums + 1)
-		 * valid entries.
+		 * valid entries; otherwise there are boundinfo->ndatums.
 		 */
-		if (context->strategy == PARTITION_STRATEGY_RANGE)
-			result->bound_offsets = bms_add_range(NULL, 0, boundinfo->ndatums);
-		else
-			result->bound_offsets = bms_add_range(NULL, 0,
-												  boundinfo->ndatums - 1);
+		rangemax = context->strategy == PARTITION_STRATEGY_RANGE ?
+			boundinfo->ndatums : boundinfo->ndatums - 1;
+
+		result->bound_offsets =
+			bms_add_range(result->bound_offsets, 0, rangemax);
 		result->scan_default = partition_bound_has_default(boundinfo);
 		result->scan_null = partition_bound_accepts_nulls(boundinfo);
 		return result;
diff --git a/src/include/partitioning/partbounds.h b/src/include/partitioning/partbounds.h
index c7535e32fc..160b319e0a 100644
--- a/src/include/partitioning/partbounds.h
+++ b/src/include/partitioning/partbounds.h
@@ -56,7 +56,6 @@
  * pointed by remainder produced when hash value of the datum-tuple is divided
  * by the greatest modulus.
  */
-
 typedef struct PartitionBoundInfoData
 {
 	char		strategy;		/* hash, list or range? */

--YZ5djTAD1cGYuMQK--