Date: Tue, 24 Apr 2018 11:19:04 -0300
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
To: Amit Langote <Langote_Amit_f8@lab.ntt.co.jp>
Cc: David Rowley <david.rowley@2ndquadrant.com>,
	Robert Haas <robertmhaas@gmail.com>,
	Amit Langote <amitlangote09@gmail.com>,
	Jesper Pedersen <jesper.pedersen@redhat.com>,
	Rajkumar Raghuwanshi <rajkumar.raghuwanshi@enterprisedb.com>,
	Beena Emerson <memissemerson@gmail.com>,
	amul sul <sulamul@gmail.com>,
	"pgsql-hackers@postgresql.org" <pgsql-hackers@postgresql.org>,
	Dilip Kumar <dilipbalaut@gmail.com>
Subject: Re: [HACKERS] Runtime Partition Pruning
Message-ID: <20180424141904.73tydqgb35lhjcsb@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="w6ct55433733gdzt"
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <be61173a-b5a1-3993-27f0-4116e40d9621@lab.ntt.co.jp>
User-Agent: NeoMutt/20170306-137-4415bd-dirty (1.8.0)
Precedence: bulk


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

Amit Langote wrote:

> That's neat!  Definitely agree that we should call ExecInitExpr just once
> here.  The patch looks good too, except the long line.

How about this as a small tweak?  Determine the array index using a
macro, which serves as documentation.

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

--w6ct55433733gdzt
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: attachment;
 filename="v3-0001-Initialize-ExprStates-once-in-run-time-partition-.patch"
Content-Transfer-Encoding: 8bit

From 135740cd17e8c05f36a3b91b3af29515df6a2d3b Mon Sep 17 00:00:00 2001
From: "dgrowley@gmail.com" <dgrowley@gmail.com>
Date: Thu, 19 Apr 2018 11:51:16 +1200
Subject: [PATCH v3] Initialize ExprStates once in run-time partition pruning
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Instead of doing ExecInitExpr every time a Param needs to be evaluated
in run-time partition pruning, do it once during run-time pruning
set-up and cache the exprstate in PartitionPruneContext, saving a lot of
work.

Author: David Rowley
Reviewed-by: Amit Langote, Álvaro Herrera
Discussion: https://postgr.es/m/CAKJS1f8-x+q-90QAPDu_okhQBV4DPEtPz8CJ=m0940GyT4DA4w@mail.gmail.com
---
 src/backend/executor/execPartition.c | 35 +++++++++++++++++++++++++++++++++++
 src/backend/partitioning/partprune.c | 27 +++++++++++++++++----------
 src/include/partitioning/partprune.h |  9 +++++++++
 3 files changed, 61 insertions(+), 10 deletions(-)

diff --git a/src/backend/executor/execPartition.c b/src/backend/executor/execPartition.c
index f7bbb804aa..f7418f64b1 100644
--- a/src/backend/executor/execPartition.c
+++ b/src/backend/executor/execPartition.c
@@ -1442,7 +1442,9 @@ ExecSetupPartitionPruneState(PlanState *planstate, List *partitionpruneinfo)
 		PartitionDesc partdesc;
 		Relation	rel;
 		PartitionKey partkey;
+		ListCell   *lc2;
 		int			partnatts;
+		int			n_steps;
 
 		pprune->present_parts = bms_copy(pinfo->present_parts);
 		pprune->subnode_map = palloc(sizeof(int) * pinfo->nparts);
@@ -1465,6 +1467,7 @@ ExecSetupPartitionPruneState(PlanState *planstate, List *partitionpruneinfo)
 
 		partkey = RelationGetPartitionKey(rel);
 		partdesc = RelationGetPartitionDesc(rel);
+		n_steps = list_length(pinfo->pruning_steps);
 
 		context->strategy = partkey->strategy;
 		context->partnatts = partnatts = partkey->partnatts;
@@ -1476,6 +1479,38 @@ ExecSetupPartitionPruneState(PlanState *planstate, List *partitionpruneinfo)
 		context->boundinfo = partition_bounds_copy(partdesc->boundinfo, partkey);
 		context->planstate = planstate;
 		context->safeparams = NULL; /* empty for now */
+		context->exprstates = palloc0(sizeof(ExprState *) * n_steps * partnatts);
+
+		/* Initialize expression states for each expression */
+		foreach(lc2, pinfo->pruning_steps)
+		{
+			PartitionPruneStepOp *step = (PartitionPruneStepOp *) lfirst(lc2);
+			ListCell   *lc3;
+			int			keyno;
+
+			/* not needed for other step kinds */
+			if (!IsA(step, PartitionPruneStepOp))
+				continue;
+
+			Assert(list_length(step->exprs) <= partnatts);
+
+			keyno = 0;
+			foreach(lc3, step->exprs)
+			{
+				Expr	   *expr = (Expr *) lfirst(lc3);
+				int			stateidx;
+
+				/* not needed for Consts */
+				if (!IsA(expr, Const))
+				{
+					stateidx = PruneCxtStateIdx(partnatts,
+												step->step.step_id, keyno);
+					context->exprstates[stateidx] =
+						ExecInitExpr(expr, context->planstate);
+				}
+				keyno++;
+			}
+		}
 
 		pprune->pruning_steps = pinfo->pruning_steps;
 		pprune->extparams = bms_copy(pinfo->extparams);
diff --git a/src/backend/partitioning/partprune.c b/src/backend/partitioning/partprune.c
index f8844ef2eb..62159477c1 100644
--- a/src/backend/partitioning/partprune.c
+++ b/src/backend/partitioning/partprune.c
@@ -169,7 +169,7 @@ static PruneStepResult *perform_pruning_combine_step(PartitionPruneContext *cont
 static bool match_boolean_partition_clause(Oid partopfamily, Expr *clause,
 							   Expr *partkey, Expr **outconst);
 static bool partkey_datum_from_expr(PartitionPruneContext *context,
-						Expr *expr, Datum *value);
+						Expr *expr, int stateidx, Datum *value);
 
 /*
  * make_partition_pruneinfo
@@ -444,6 +444,7 @@ prune_append_rel_partitions(RelOptInfo *rel)
 	/* Not valid when being called from the planner */
 	context.planstate = NULL;
 	context.safeparams = NULL;
+	context.exprstates = NULL;
 
 	/* Actual pruning happens here. */
 	partindexes = get_matching_partitions(&context, pruning_steps);
@@ -2788,10 +2789,13 @@ perform_pruning_base_step(PartitionPruneContext *context,
 		if (lc1 != NULL)
 		{
 			Expr	   *expr;
+			int			stateidx;
 			Datum		datum;
 
 			expr = lfirst(lc1);
-			if (partkey_datum_from_expr(context, expr, &datum))
+			stateidx = PruneCxtStateIdx(context->partnatts,
+										opstep->step.step_id, keyno);
+			if (partkey_datum_from_expr(context, expr, stateidx, &datum))
 			{
 				Oid			cmpfn;
 
@@ -3025,12 +3029,15 @@ match_boolean_partition_clause(Oid partopfamily, Expr *clause, Expr *partkey,
 
 /*
  * partkey_datum_from_expr
- *		Evaluate 'expr', set *value to the resulting Datum. Return true if
- *		evaluation was possible, otherwise false.
+ *		Evaluate expression for potential partition pruning
+ *
+ * Evaluate 'expr', whose ExprState is stateidx of the context exprstate
+ * array; set *value to the resulting Datum.  Return true if evaluation was
+ * possible, otherwise false.
  */
 static bool
 partkey_datum_from_expr(PartitionPruneContext *context,
-						Expr *expr, Datum *value)
+						Expr *expr, int stateidx, Datum *value)
 {
 	switch (nodeTag(expr))
 	{
@@ -3048,18 +3055,18 @@ partkey_datum_from_expr(PartitionPruneContext *context,
 				bms_is_member(((Param *) expr)->paramid, context->safeparams))
 			{
 				ExprState  *exprstate;
+				ExprContext *ectx;
 				bool		isNull;
 
-				exprstate = ExecInitExpr(expr, context->planstate);
-
-				*value = ExecEvalExprSwitchContext(exprstate,
-												   context->planstate->ps_ExprContext,
-												   &isNull);
+				exprstate = context->exprstates[stateidx];
+				ectx = context->planstate->ps_ExprContext;
+				*value = ExecEvalExprSwitchContext(exprstate, ectx, &isNull);
 				if (isNull)
 					return false;
 
 				return true;
 			}
+			break;
 
 		default:
 			break;
diff --git a/src/include/partitioning/partprune.h b/src/include/partitioning/partprune.h
index a5568abce6..c9fe95dc30 100644
--- a/src/include/partitioning/partprune.h
+++ b/src/include/partitioning/partprune.h
@@ -50,8 +50,17 @@ typedef struct PartitionPruneContext
 	 * are not safe to use until the executor is running.
 	 */
 	Bitmapset  *safeparams;
+
+	/*
+	 * Array of ExprStates, indexed as per PruneCtxStateIdx; one for each
+	 * partkey in each pruning step.  Allocated if planstate is non-NULL,
+	 * otherwise NULL.
+	 */
+	ExprState **exprstates;
 } PartitionPruneContext;
 
+#define PruneCxtStateIdx(partnatts, step_id, keyno) \
+	((partnatts) * (step_id) + (keyno))
 
 extern List *make_partition_pruneinfo(PlannerInfo *root, List *partition_rels,
 						 List *subpaths, List *prunequal);
-- 
2.11.0


--w6ct55433733gdzt--