Date: Tue, 6 Aug 2019 09:30:53 -0400
From: Alvaro Herrera <alvherre@2ndquadrant.com>
To: yuzuko <yuzukohosoya@gmail.com>
Cc: 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: <20190806133053.GA23706@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="+QahgC5+KEYLbs62"
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20190805143920.GA6232@alvherre.pgsql>
User-Agent: Mutt/1.9.4 (2018-02-28)
Precedence: bulk


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

On 2019-Aug-05, Alvaro Herrera wrote:

> So we have three locations for that test; one is where it currently is,
> which handles a small subset of the cases.  The other is where Amit
> first proposed putting it, which handles some additional cases; and the
> third one is where your latest patch puts it, which seems to handle all
> cases.  Isn't that what Amit is saying?  If that's correct (and that's
> what I want to imply with the comment changes I proposed), then we
> should just accept that version of the patch.

... actually, there's a fourth possible location, which is outside the
per-partitioning-attribute loop.  Nothing in the moved block is to be
done per attribute, so it'd be wasted work AFAICS.  I propose the
attached.

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

--+QahgC5+KEYLbs62
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
 filename="v7_ignore_contradictory_where_clauses_at_partprune_step.patch"

diff --git a/src/backend/partitioning/partprune.c b/src/backend/partitioning/partprune.c
index 09c4ed83b6..2ed1e44c18 100644
--- a/src/backend/partitioning/partprune.c
+++ b/src/backend/partitioning/partprune.c
@@ -849,10 +849,11 @@ get_matching_partitions(PartitionPruneContext *context, List *pruning_steps)
  * the context's steps list.  Each step is assigned a step identifier, unique
  * even across recursive calls.
  *
- * If we find clauses that are mutually contradictory, or a pseudoconstant
- * clause that contains false, we set context->contradictory to true and
- * return NIL (that is, no pruning steps).  Caller should consider all
- * partitions as pruned in that case.
+ * If we find clauses that are mutually contradictory, or contradictory with
+ * the partitioning constraint, or a pseudoconstant clause that contains
+ * false, we set context->contradictory to true and return NIL (that is, no
+ * pruning steps).  Caller should consider all partitions as pruned in that
+ * case.
  */
 static List *
 gen_partprune_steps_internal(GeneratePruningStepsContext *context,
@@ -942,35 +943,15 @@ gen_partprune_steps_internal(GeneratePruningStepsContext *context,
 					}
 					else
 					{
+						PartitionPruneStep *orstep;
+
 						/*
 						 * The arg didn't contain a clause matching this
 						 * partition key.  We cannot prune using such 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 = context->rel->partition_qual;
-						PartitionPruneStep *orstep;
-
-						if (partconstr)
-						{
-							partconstr = (List *)
-								expression_planner((Expr *) partconstr);
-							if (context->rel->relid != 1)
-								ChangeVarNodes((Node *) partconstr, 1,
-											   context->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);
@@ -1038,6 +1019,29 @@ gen_partprune_steps_internal(GeneratePruningStepsContext *context,
 			 */
 		}
 
+		/*
+		 * If the clause contradicts the partition constraint, mark the clause
+		 * as contradictory and we're done.  This is particularly helpful to
+		 * prune the default partition.
+		 */
+		if (context->rel->partition_qual)
+		{
+			List	   *partconstr;
+
+			partconstr = (List *)
+				expression_planner((Expr *) context->rel->partition_qual);
+			if (context->rel->relid != 1)
+				ChangeVarNodes((Node *) partconstr, 1,
+							   context->rel->relid, 0);
+			if (predicate_refuted_by(partconstr,
+									 list_make1(clause),
+									 false))
+			{
+				context->contradictory = true;
+				return NIL;
+			}
+		}
+
 		/*
 		 * See if we can match this clause to any of the partition keys.
 		 */
diff --git a/src/test/regress/expected/partition_prune.out b/src/test/regress/expected/partition_prune.out
index 2d3229fd73..6ccc91d390 100644
--- a/src/test/regress/expected/partition_prune.out
+++ b/src/test/regress/expected/partition_prune.out
@@ -592,6 +592,24 @@ explain (costs off) select * from rlp where a < 1 or (a > 20 and a < 25);
          Filter: ((a < 1) OR ((a > 20) AND (a < 25)))
 (5 rows)
 
+-- where clause contradicts sub-partition's constraint
+explain (costs off) select * from rlp where a = 20 or a = 40;
+               QUERY PLAN               
+----------------------------------------
+ Append
+   ->  Seq Scan on rlp4_1
+         Filter: ((a = 20) OR (a = 40))
+   ->  Seq Scan on rlp5_default
+         Filter: ((a = 20) OR (a = 40))
+(5 rows)
+
+explain (costs off) select * from rlp3 where a = 20;   /* empty */
+        QUERY PLAN        
+--------------------------
+ Result
+   One-Time Filter: false
+(2 rows)
+
 -- redundant clauses are eliminated
 explain (costs off) select * from rlp where a > 1 and a = 10;	/* only default */
             QUERY PLAN            
diff --git a/src/test/regress/sql/partition_prune.sql b/src/test/regress/sql/partition_prune.sql
index efdedaaeb8..a26c2f0e72 100644
--- a/src/test/regress/sql/partition_prune.sql
+++ b/src/test/regress/sql/partition_prune.sql
@@ -85,6 +85,10 @@ explain (costs off) select * from rlp where a = 29;
 explain (costs off) select * from rlp where a >= 29;
 explain (costs off) select * from rlp where a < 1 or (a > 20 and a < 25);
 
+-- where clause contradicts sub-partition's constraint
+explain (costs off) select * from rlp where a = 20 or a = 40;
+explain (costs off) select * from rlp3 where a = 20;   /* empty */
+
 -- redundant clauses are eliminated
 explain (costs off) select * from rlp where a > 1 and a = 10;	/* only default */
 explain (costs off) select * from rlp where a > 1 and a >=15;	/* rlp3 onwards, including default */

--+QahgC5+KEYLbs62--