Date: Mon, 12 Aug 2019 13:45:09 -0400
From: Alvaro Herrera <alvherre@2ndquadrant.com>
To: Amit Langote <amitlangote09@gmail.com>
Cc: Kyotaro Horiguchi <horikyota.ntt@gmail.com>,
	Simon Riggs <simon@2ndquadrant.com>,
	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: <20190812174509.GA11692@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="bp/iNruPH9dso1Pn"
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: 
 <CA+HiwqGm18B8UQ5Sip_nsNYmDiHtoaVORvCPumo_bbXTXHPRBw@mail.gmail.com>
User-Agent: Mutt/1.9.4 (2018-02-28)
Precedence: bulk


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

v3-0001 still seems to leave things a bit duplicative.  I think we can
make it better if we move the logic to set RelOptInfo->partition_qual to
a separate routine (set_baserel_partition_constraint mirroring the
existing set_baserel_partition_key_exprs), and then call that from both
places that need access to partition_qual.

So I propose that the attached v4 patch should be the final form of this
(also rebased across today's list_concat API change).  I verified that
constraint exclusion is not being called by partprune unless a default
partition exists (thanks errbacktrace()); I think that should appease
Simon's performance concern for the most common case of default
partition not existing.

I think I was not really understanding the comments being added by
Amit's v3, so I reworded them.  I hope I understood the intent of the
code correctly.

I'm not comfortable with RelOptInfo->partition_qual.  But I'd rather
leave that for another time.

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

--bp/iNruPH9dso1Pn
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment; filename="v4-0001-Rejigger-code.patch"

From eccc45f4e62be8ded446e8e5afd81aa248eed937 Mon Sep 17 00:00:00 2001
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
Date: Fri, 9 Aug 2019 13:23:56 -0400
Subject: [PATCH v4] Rejigger code

---
 src/backend/optimizer/util/plancat.c | 59 +++++++++++++++--------
 src/backend/partitioning/partprune.c | 70 ++++++++++------------------
 2 files changed, 64 insertions(+), 65 deletions(-)

diff --git a/src/backend/optimizer/util/plancat.c b/src/backend/optimizer/util/plancat.c
index 98e99481c6..cf1761401d 100644
--- a/src/backend/optimizer/util/plancat.c
+++ b/src/backend/optimizer/util/plancat.c
@@ -78,6 +78,9 @@ static void set_relation_partition_info(PlannerInfo *root, RelOptInfo *rel,
 static PartitionScheme find_partition_scheme(PlannerInfo *root, Relation rel);
 static void set_baserel_partition_key_exprs(Relation relation,
 											RelOptInfo *rel);
+static void set_baserel_partition_constraint(Relation relation,
+											 RelOptInfo *rel);
+
 
 /*
  * get_relation_info -
@@ -1267,25 +1270,9 @@ get_relation_constraints(PlannerInfo *root,
 	 */
 	if (include_partition && relation->rd_rel->relispartition)
 	{
-		List	   *pcqual = RelationGetPartitionQual(relation);
-
-		if (pcqual)
-		{
-			/*
-			 * Run the partition quals through const-simplification similar to
-			 * check constraints.  We skip canonicalize_qual, though, because
-			 * partition quals should be in canonical form already; also,
-			 * since the qual is in implicit-AND format, we'd have to
-			 * explicitly convert it to explicit-AND format and back again.
-			 */
-			pcqual = (List *) eval_const_expressions(root, (Node *) pcqual);
-
-			/* Fix Vars to have the desired varno */
-			if (varno != 1)
-				ChangeVarNodes((Node *) pcqual, 1, varno, 0);
-
-			result = list_concat(result, pcqual);
-		}
+		/* make sure rel->partition_qual is set */
+		set_baserel_partition_constraint(relation, rel);
+		result = list_concat(result, rel->partition_qual);
 	}
 
 	table_close(relation, NoLock);
@@ -2149,7 +2136,7 @@ set_relation_partition_info(PlannerInfo *root, RelOptInfo *rel,
 	rel->boundinfo = partdesc->boundinfo;
 	rel->nparts = partdesc->nparts;
 	set_baserel_partition_key_exprs(relation, rel);
-	rel->partition_qual = RelationGetPartitionQual(relation);
+	set_baserel_partition_constraint(relation, rel);
 }
 
 /*
@@ -2324,3 +2311,35 @@ set_baserel_partition_key_exprs(Relation relation,
 	 */
 	rel->nullable_partexprs = (List **) palloc0(sizeof(List *) * partnatts);
 }
+
+/*
+ * set_baserel_partition_constraint
+ *
+ * Builds the partition constraint for the given base relation and sets it
+ * in the given RelOptInfo.  All Var nodes are restamped with the relid of the
+ * given relation.
+ */
+static void
+set_baserel_partition_constraint(Relation relation, RelOptInfo *rel)
+{
+	List	   *partconstr;
+
+	if (rel->partition_qual)	/* already done */
+		return;
+
+	/*
+	 * Run the partition quals through const-simplification similar to check
+	 * constraints.  We skip canonicalize_qual, though, because partition
+	 * quals should be in canonical form already; also, since the qual is in
+	 * implicit-AND format, we'd have to explicitly convert it to explicit-AND
+	 * format and back again.
+	 */
+	partconstr = RelationGetPartitionQual(relation);
+	if (partconstr)
+	{
+		partconstr = (List *) expression_planner((Expr *) partconstr);
+		if (rel->relid != 1)
+			ChangeVarNodes((Node *) partconstr, 1, rel->relid, 0);
+		rel->partition_qual = partconstr;
+	}
+}
diff --git a/src/backend/partitioning/partprune.c b/src/backend/partitioning/partprune.c
index a1bd4efd0b..f619171ace 100644
--- a/src/backend/partitioning/partprune.c
+++ b/src/backend/partitioning/partprune.c
@@ -619,31 +619,16 @@ gen_partprune_steps(RelOptInfo *rel, List *clauses, PartClauseTarget target,
 	context->target = target;
 
 	/*
-	 * For sub-partitioned tables there's a corner case where if the
-	 * sub-partitioned table shares any partition keys with its parent, then
-	 * it's possible that the partitioning hierarchy allows the parent
-	 * partition to only contain a narrower range of values than the
-	 * sub-partitioned table does.  In this case it is possible that we'd
-	 * include partitions that could not possibly have any tuples matching
-	 * 'clauses'.  The possibility of such a partition arrangement is perhaps
-	 * unlikely for non-default partitions, but it may be more likely in the
-	 * case of default partitions, so we'll add the parent partition table's
-	 * partition qual to the clause list in this case only.  This may result
-	 * in the default partition being eliminated.
+	 * If this partitioned table is in turn a partition, and it shares any
+	 * partition keys with its parent, then it's possible that the hierarchy
+	 * allows the parent a narrower range of values than some of its
+	 * partitions (particularly the default one).  This is normally not
+	 * useful, but it can be to prune the default partition.
 	 */
-	if (partition_bound_has_default(rel->boundinfo) &&
-		rel->partition_qual != NIL)
+	if (partition_bound_has_default(rel->boundinfo) && rel->partition_qual)
 	{
-		List	   *partqual = rel->partition_qual;
-
-		partqual = (List *) expression_planner((Expr *) partqual);
-
-		/* Fix Vars to have the desired varno */
-		if (rel->relid != 1)
-			ChangeVarNodes((Node *) partqual, 1, rel->relid, 0);
-
 		/* Make a copy to avoid modifying the passed-in List */
-		clauses = list_concat_copy(clauses, partqual);
+		clauses = list_concat_copy(clauses, rel->partition_qual);
 	}
 
 	/* Down into the rabbit-hole. */
@@ -867,6 +852,24 @@ gen_partprune_steps_internal(GeneratePruningStepsContext *context,
 	List	   *result = NIL;
 	ListCell   *lc;
 
+	/*
+	 * If this partitioned relation has a default partition and is itself
+	 * a partition (as evidenced by partition_qual being not NIL), we first
+	 * check if the clauses contradict the partition constraint.  If they do,
+	 * there's no need to generate any steps as it'd already be proven that no
+	 * partitions need to be scanned.
+	 *
+	 * This is a measure of last resort only to be used because the default
+	 * partition cannot be pruned using the steps; regular pruning, which is
+	 * cheaper, is sufficient when no default partition exists.
+	 */
+	if (partition_bound_has_default(context->rel->boundinfo) &&
+		predicate_refuted_by(context->rel->partition_qual, clauses, false))
+	{
+		context->contradictory = true;
+		return NIL;
+	}
+
 	memset(keyclauses, 0, sizeof(keyclauses));
 	foreach(lc, clauses)
 	{
@@ -1019,29 +1022,6 @@ 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.
 		 */
-- 
2.17.1


--bp/iNruPH9dso1Pn--