pg.ddx.io  pgsql-hackers@postgresql.org mailing list archive  
help / color / mirror / Atom feed
From: Yuzuko Hosoya <hosoya.yuzuko@lab.ntt.co.jp>
To: 'Amit Langote' <Langote_Amit_f8@lab.ntt.co.jp>
To: 'Thibaut' <thibaut.madelaine@dalibo.com>
To: 'Imai, Yoshikazu' <imai.yoshikazu@jp.fujitsu.com>
Cc: 'PostgreSQL Hackers' <pgsql-hackers@lists.postgresql.org>
Subject: RE: Problem with default partition pruning
Date: Tue, 2 Apr 2019 14:02:08 +0900
Message-ID: <008f01d4e911$39aa87a0$acff96e0$@lab.ntt.co.jp> (raw)
In-Reply-To: <001ea23e-638f-9ab2-143d-2cf1105bf8d6@lab.ntt.co.jp>
References: <00e601d4ca86$932b8bc0$b982a340$@lab.ntt.co.jp>
	<afef4fb8-c448-d54a-9628-7c26277d4895@lab.ntt.co.jp>
	<010e01d4ce68$c0427b10$40c77130$@lab.ntt.co.jp>
	<0F97FA9ABBDBE54F91744A9B37151A5129A78A@g01jpexmbkw24>
	<a4968068-6401-7a9c-8bd4-6a3bc9164a86@dalibo.com>
	<001901d4daf5$1ef4f640$5cdee2c0$@lab.ntt.co.jp>
	<9bed6b79-f264-6976-b880-e2a5d23e9d85@lab.ntt.co.jp>
	<003501d4de21$8816f540$9844dfc0$@lab.ntt.co.jp>
	<bd03f475-30d4-c4d0-3d7f-d2fbde755971@dalibo.com>
	<9bb31dfe-b0d0-53f3-3ea6-e64b811424cf@lab.ntt.co.jp>
	<c6b9557e-c029-e72c-c697-3f216e559806@dalibo.com>
	<005101d4e074$e24d1e40$a6e75ac0$@lab.ntt.co.jp>
	<001ea23e-638f-9ab2-143d-2cf1105bf8d6@lab.ntt.co.jp>

Hi,

> Maybe we should have two patches as we seem to be improving two things:
> 
> 1. Patch to fix problems with default partition pruning originally reported by Hosoya-san
> 
> 2. Patch to determine if a given clause contradicts a sub-partitioned table's partition constraint,
> fixing problems unearthed by Thibaut's tests

I attached the latest patches according to Amit comment.
v3_default_partition_pruning.patch fixes default partition pruning problems
and ignore_contradictory_where_clauses_at_partprune_step.patch fixes
sub-partition problems Thibaut tested.

Best regards,
Yuzuko Hosoya

Attachments:

  [application/octet-stream] ignore_contradictory_where_clauses_at_partprune_step.patch (2.6K, ../008f01d4e911$39aa87a0$acff96e0$@lab.ntt.co.jp/2-ignore_contradictory_where_clauses_at_partprune_step.patch)
  download | inline diff:
diff --git a/src/backend/partitioning/partprune.c b/src/backend/partitioning/partprune.c
index 5cd740a..c45cd48 100644
--- a/src/backend/partitioning/partprune.c
+++ b/src/backend/partitioning/partprune.c
@@ -850,32 +850,13 @@ gen_partprune_steps_internal(GeneratePruningStepsContext *context,
 						 * an arg.  To indicate that to the pruning code, we
 						 * must construct a dummy PartitionPruneStepCombine
 						 * whose source_stepids is set to an empty List.
-						 * However, if we can prove using constraint exclusion
-						 * that the clause refutes the table's partition
-						 * constraint (if it's sub-partitioned), we need not
-						 * bother with that.  That is, we effectively ignore
-						 * this OR arm.
 						 */
-						List	   *partconstr = rel->partition_qual;
 						PartitionPruneStep *orstep;
 
 						/* Just ignore this argument. */
 						if (arg_contradictory)
 							continue;
 
-						if (partconstr)
-						{
-							partconstr = (List *)
-								expression_planner((Expr *) partconstr);
-							if (rel->relid != 1)
-								ChangeVarNodes((Node *) partconstr, 1,
-											   rel->relid, 0);
-							if (predicate_refuted_by(partconstr,
-													 list_make1(arg),
-													 false))
-								continue;
-						}
-
 						orstep = gen_prune_step_combine(context, NIL,
 														PARTPRUNE_COMBINE_UNION);
 						arg_stepids = lappend_int(arg_stepids, orstep->step_id);
@@ -951,6 +932,7 @@ gen_partprune_steps_internal(GeneratePruningStepsContext *context,
 			bool		clause_is_not_null = false;
 			PartClauseInfo *pc = NULL;
 			List	   *clause_steps = NIL;
+			List	   *partconstr = rel->partition_qual;
 
 			switch (match_clause_to_partition_key(rel, context,
 												  clause, partkey, i,
@@ -961,6 +943,29 @@ gen_partprune_steps_internal(GeneratePruningStepsContext *context,
 					Assert(pc != NULL);
 
 					/*
+					 * If this clause can be proved false for this partition,
+					 * given its partition constraint, we can ignore it, 
+					 * that is not try to pass it to the pruning code.  
+					 * We should do that especially to avoid pruning code 
+					 * wrongly failing to prune the default partition.
+					 */
+					if (partconstr)
+					{
+						partconstr = (List *)
+							expression_planner((Expr *) partconstr);
+						if (rel->relid != 1)
+							ChangeVarNodes((Node *) partconstr, 1,
+										   rel->relid, 0);
+						if (predicate_refuted_by(partconstr,
+												 list_make1(clause),
+												 false))
+						{
+							*contradictory = true;
+							return NIL;
+						}
+					}
+					
+					/*
 					 * Since we only allow strict operators, check for any
 					 * contradicting IS NULL.
 					 */
=

  [application/octet-stream] v3_default_partition_pruning.patch (12.1K, ../008f01d4e911$39aa87a0$acff96e0$@lab.ntt.co.jp/3-v3_default_partition_pruning.patch)
  download | inline diff:
diff --git a/src/backend/partitioning/partprune.c b/src/backend/partitioning/partprune.c
index 59f34f5..5cd740a 100644
--- a/src/backend/partitioning/partprune.c
+++ b/src/backend/partitioning/partprune.c
@@ -647,6 +647,7 @@ get_matching_partitions(PartitionPruneContext *context, List *pruning_steps)
 	PruneStepResult **results,
 			   *final_result;
 	ListCell   *lc;
+	bool       scan_default;
 
 	/* If there are no pruning steps then all partitions match. */
 	if (num_steps == 0)
@@ -698,20 +699,23 @@ get_matching_partitions(PartitionPruneContext *context, List *pruning_steps)
 	Assert(final_result != NULL);
 	i = -1;
 	result = NULL;
+	scan_default = final_result->scan_default;
 	while ((i = bms_next_member(final_result->bound_offsets, i)) >= 0)
 	{
 		int			partindex = context->boundinfo->indexes[i];
 
 		/*
-		 * In range and hash partitioning cases, some slots may contain -1,
+		 * 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 data or for a given remainder, respectively. The default
-		 * partition, if any, in case of range partitioning, will be added to
-		 * the result, because the specified range still satisfies the query's
-		 * conditions.
+		 * 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
+			scan_default |= partition_bound_has_default(context->boundinfo);
 	}
 
 	/* Add the null and/or default partition if needed and if present. */
@@ -721,7 +725,7 @@ get_matching_partitions(PartitionPruneContext *context, List *pruning_steps)
 		Assert(partition_bound_accepts_nulls(context->boundinfo));
 		result = bms_add_member(result, context->boundinfo->null_index);
 	}
-	if (final_result->scan_default)
+	if (scan_default)
 	{
 		Assert(context->strategy == PARTITION_STRATEGY_LIST ||
 			   context->strategy == PARTITION_STRATEGY_RANGE);
@@ -2236,6 +2240,11 @@ get_matching_hash_bounds(PartitionPruneContext *context,
  * get_matching_list_bounds
  *		Determine the offsets of list bounds matching the specified value,
  *		according to the semantics of the given operator strategy
+ *
+ * scan_default will be set in the returned struct, if the default partition
+ * needs to be scanned, provided one exists at all.  scan_null will be set if
+ * the special null-accepting partition needs to be scanned.
+ *
  * 'opstrategy' if non-zero must be a btree strategy number.
  *
  * 'value' contains the value to use for pruning.
@@ -2438,8 +2447,17 @@ 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.
  *
- * If default partition needs to be scanned for given values, set scan_default
- * in result if present.
+ * 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.
  *
  * 'opstrategy' if non-zero must be a btree strategy number.
  *
@@ -2465,8 +2483,7 @@ get_matching_range_bounds(PartitionPruneContext *context,
 	int		   *partindices = boundinfo->indexes;
 	int			off,
 				minoff,
-				maxoff,
-				i;
+				maxoff;
 	bool		is_equal;
 	bool		inclusive = false;
 
@@ -2530,11 +2547,7 @@ get_matching_range_bounds(PartitionPruneContext *context,
 				if (nvalues == partnatts)
 				{
 					/* There can only be zero or one matching partition. */
-					if (partindices[off + 1] >= 0)
-						result->bound_offsets = bms_make_singleton(off + 1);
-					else
-						result->scan_default =
-							partition_bound_has_default(boundinfo);
+					result->bound_offsets = bms_make_singleton(off + 1);
 					return result;
 				}
 				else
@@ -2622,57 +2635,21 @@ get_matching_range_bounds(PartitionPruneContext *context,
 					maxoff = off + 1;
 				}
 
-				/*
-				 * Skip if minoff/maxoff are actually the upper bound of a
-				 * un-assigned portion of values.
-				 */
-				if (partindices[minoff] < 0 && minoff < boundinfo->ndatums)
-					minoff++;
-				if (partindices[maxoff] < 0 && maxoff >= 1)
-					maxoff--;
-
-				/*
-				 * There may exist a range of values unassigned to any
-				 * non-default partition between the datums at minoff and
-				 * maxoff.  Add the default partition in that case.
-				 */
-				if (partition_bound_has_default(boundinfo))
-				{
-					for (i = minoff; i <= maxoff; i++)
-					{
-						if (partindices[i] < 0)
-						{
-							result->scan_default = true;
-							break;
-						}
-					}
-				}
-
 				Assert(minoff >= 0 && maxoff >= 0);
 				result->bound_offsets = bms_add_range(NULL, minoff, maxoff);
 			}
-			else if (off >= 0)	/* !is_equal */
+			else
 			{
 				/*
 				 * The 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 only
-				 * partition that may contain the lookup value.
+				 * partition that may contain the lookup value.  If 'off' is
+				 * -1 indicating that all bounds are greater, then we simply
+				 * end up adding the first bound's offset, that is, 0.
 				 */
-				if (partindices[off + 1] >= 0)
-					result->bound_offsets = bms_make_singleton(off + 1);
-				else
-					result->scan_default =
-						partition_bound_has_default(boundinfo);
-			}
-			else
-			{
-				/*
-				 * off < 0: the lookup value is smaller than all bounds, so
-				 * only the default partition qualifies, if there is one.
-				 */
-				result->scan_default = partition_bound_has_default(boundinfo);
+				result->bound_offsets = bms_make_singleton(off + 1);
 			}
 
 			return result;
@@ -2770,16 +2747,7 @@ get_matching_range_bounds(PartitionPruneContext *context,
 												boundinfo,
 												nvalues, values,
 												&is_equal);
-			if (off < 0)
-			{
-				/*
-				 * All bounds are greater than the key, so we could only
-				 * expect to find the lookup key in the default partition.
-				 */
-				result->scan_default = partition_bound_has_default(boundinfo);
-				return result;
-			}
-			else
+			if (off >= 0)
 			{
 				/*
 				 * See the comment above.
@@ -2827,6 +2795,12 @@ get_matching_range_bounds(PartitionPruneContext *context,
 				else
 					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:
@@ -2834,58 +2808,43 @@ get_matching_range_bounds(PartitionPruneContext *context,
 			break;
 	}
 
+	Assert(minoff >= 0 && minoff <= boundinfo->ndatums);
+	Assert(maxoff >= 0 && maxoff <= boundinfo->ndatums);
+
 	/*
-	 * Skip a gap and when doing so, check if the bound contains a finite
-	 * value to decide if we need to add the default partition.  If it's an
-	 * infinite bound, we need not add the default partition, as having an
-	 * infinite bound means the partition in question catches any values that
-	 * would otherwise be in the default partition.
+	 * If the smallest partition to return has MINVALUE (negative infinity) as
+	 * its lower bound, increment it to point to the next finite bound
+	 * (supposedly its upper bound), so that we don't advertently end up
+	 * scanning the default partition.
 	 */
-	if (partindices[minoff] < 0)
+	if (minoff < boundinfo->ndatums && partindices[minoff] < 0)
 	{
 		int			lastkey = nvalues - 1;
 
-		if (minoff >= 0 &&
-			minoff < boundinfo->ndatums &&
-			boundinfo->kind[minoff][lastkey] ==
-			PARTITION_RANGE_DATUM_VALUE)
-			result->scan_default = partition_bound_has_default(boundinfo);
-
-		minoff++;
+		if (boundinfo->kind[minoff][lastkey] ==
+			PARTITION_RANGE_DATUM_MINVALUE)
+		{
+			minoff++;
+			Assert(boundinfo->indexes[minoff] >= 0);
+		}
 	}
 
 	/*
-	 * Skip a gap.  See the above comment about how we decide whether or not
-	 * to scan the default partition based whether the datum that will become
-	 * the maximum datum is finite or not.
+	 * If the previous greatest partition has MAXVALUE (positive infinity) as
+	 * its upper bound (something only possible to do with multi-column range
+	 * partitioning), we scan switch to it as the greatest partition to
+	 * return.  Again, so that we don't advertently end up scanning the
+	 * default partition.
 	 */
 	if (maxoff >= 1 && partindices[maxoff] < 0)
 	{
 		int			lastkey = nvalues - 1;
 
-		if (maxoff >= 0 &&
-			maxoff <= boundinfo->ndatums &&
-			boundinfo->kind[maxoff - 1][lastkey] ==
-			PARTITION_RANGE_DATUM_VALUE)
-			result->scan_default = partition_bound_has_default(boundinfo);
-
-		maxoff--;
-	}
-
-	if (partition_bound_has_default(boundinfo))
-	{
-		/*
-		 * There may exist a range of values unassigned to any non-default
-		 * partition between the datums at minoff and maxoff.  Add the default
-		 * partition in that case.
-		 */
-		for (i = minoff; i <= maxoff; i++)
+		if (boundinfo->kind[maxoff - 1][lastkey] ==
+			PARTITION_RANGE_DATUM_MAXVALUE)
 		{
-			if (partindices[i] < 0)
-			{
-				result->scan_default = true;
-				break;
-			}
+			maxoff--;
+			Assert(boundinfo->indexes[maxoff] >= 0);
 		}
 	}
 
@@ -3168,14 +3127,23 @@ 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 partitions.
+	 * any partition pruning, we just return all datum offsets.
 	 */
 	result = (PruneStepResult *) palloc0(sizeof(PruneStepResult));
 	if (list_length(cstep->source_stepids) == 0)
 	{
 		PartitionBoundInfo boundinfo = context->boundinfo;
 
-		result->bound_offsets = bms_add_range(NULL, 0, boundinfo->ndatums - 1);
+		/*
+		 * Add all valid offsets into the boundinfo->indexes array.  For range
+		 * partitioning, boundinfo->indexes contains (boundinfo->ndatums + 1)
+		 * valid entries.
+		 */
+		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);
 		result->scan_default = partition_bound_has_default(boundinfo);
 		result->scan_null = partition_bound_accepts_nulls(boundinfo);
 		return result;
diff --git a/src/test/regress/expected/partition_prune.out b/src/test/regress/expected/partition_prune.out
index 7806ba1..263a1e5 100644
--- a/src/test/regress/expected/partition_prune.out
+++ b/src/test/regress/expected/partition_prune.out
@@ -504,15 +504,13 @@ explain (costs off) select * from rlp where a <= 31;
          Filter: (a <= 31)
    ->  Seq Scan on rlp5_1
          Filter: (a <= 31)
-   ->  Seq Scan on rlp5_default
-         Filter: (a <= 31)
    ->  Seq Scan on rlp_default_10
          Filter: (a <= 31)
    ->  Seq Scan on rlp_default_30
          Filter: (a <= 31)
    ->  Seq Scan on rlp_default_default
          Filter: (a <= 31)
-(29 rows)
+(27 rows)
 
 explain (costs off) select * from rlp where a = 1 or a = 7;
            QUERY PLAN           
@@ -559,11 +557,7 @@ explain (costs off) select * from rlp where a > 20 and a < 27;
          Filter: ((a > 20) AND (a < 27))
    ->  Seq Scan on rlp4_2
          Filter: ((a > 20) AND (a < 27))
-   ->  Seq Scan on rlp4_default
-         Filter: ((a > 20) AND (a < 27))
-   ->  Seq Scan on rlp_default_default
-         Filter: ((a > 20) AND (a < 27))
-(9 rows)
+(5 rows)
 
 explain (costs off) select * from rlp where a = 29;
         QUERY PLAN        
=

view thread (89+ messages)  latest in thread

Message-ID: <008f01d4e911$39aa87a0$acff96e0$@lab.ntt.co.jp>
Permalink:  ../008f01d4e911$39aa87a0$acff96e0$@lab.ntt.co.jp/
Also on:    postgresql.org/message-id/008f01d4e911$39aa87a0$acff96e0$@lab.ntt.co.jp

 ·  · 

reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: pgsql-hackers@postgresql.org
  Cc: hosoya.yuzuko@lab.ntt.co.jp, Langote_Amit_f8@lab.ntt.co.jp, thibaut.madelaine@dalibo.com, imai.yoshikazu@jp.fujitsu.com, pgsql-hackers@lists.postgresql.org
  Subject: RE: Problem with default partition pruning
  In-Reply-To: <008f01d4e911$39aa87a0$acff96e0$@lab.ntt.co.jp>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox