Perform slot validity checks in a separate pass over expression.

This reduces code duplication a bit, but the primary benefit that it
makes JITing expression evaluation easier. When doing so we can't, as
previously done in the interpreted case, really change opcode without
recompiling. Nor dow we just carry around unnecessary branches to
avoid re-checking over and over.

As a minor side-effect this makes ExecEvalStepOp() O(log(N)) rather
than O(N).

Author: Andres Freund
Discussion: https://postgr.es/m/20170901064131.tazjxwus3k2w3ybh@alap3.anarazel.de

src/backend/executor/execExpr.c

@@ -680,20 +680,19 @@ ExecInitExprRec(Expr *node, ExprState *state,
 					/* regular user column */
 					scratch.d.var.attnum = variable->varattno - 1;
 					scratch.d.var.vartype = variable->vartype;
-					/* select EEOP_*_FIRST opcode to force one-time checks */
 					switch (variable->varno)
 					{
 						case INNER_VAR:
-							scratch.opcode = EEOP_INNER_VAR_FIRST;
+							scratch.opcode = EEOP_INNER_VAR;
 							break;
 						case OUTER_VAR:
-							scratch.opcode = EEOP_OUTER_VAR_FIRST;
+							scratch.opcode = EEOP_OUTER_VAR;
 							break;
 
 							/* INDEX_VAR is handled by default case */
 
 						default:
-							scratch.opcode = EEOP_SCAN_VAR_FIRST;
+							scratch.opcode = EEOP_SCAN_VAR;
 							break;
 					}
 				}

src/backend/executor/execExprInterp.c

@@ -95,8 +95,17 @@
  */
 #if defined(EEO_USE_COMPUTED_GOTO)
 
+/* struct for jump target -> opcode lookup table */
+typedef struct ExprEvalOpLookup
+{
+	const void *opcode;
+	ExprEvalOp op;
+} ExprEvalOpLookup;
+
 /* to make dispatch_table accessible outside ExecInterpExpr() */
 static const void **dispatch_table = NULL;
+/* jump target -> opcode lookup table */
+static ExprEvalOpLookup reverse_dispatch_table[EEOP_LAST];
 
 #define EEO_SWITCH()
 #define EEO_CASE(name)		CASE_##name:
@@ -137,11 +146,8 @@ static void ExecEvalRowNullInt(ExprState *state, ExprEvalStep *op,
 				   ExprContext *econtext, bool checkisnull);
 
 /* fast-path evaluation functions */
-static Datum ExecJustInnerVarFirst(ExprState *state, ExprContext *econtext, bool *isnull);
 static Datum ExecJustInnerVar(ExprState *state, ExprContext *econtext, bool *isnull);
-static Datum ExecJustOuterVarFirst(ExprState *state, ExprContext *econtext, bool *isnull);
 static Datum ExecJustOuterVar(ExprState *state, ExprContext *econtext, bool *isnull);
-static Datum ExecJustScanVarFirst(ExprState *state, ExprContext *econtext, bool *isnull);
 static Datum ExecJustScanVar(ExprState *state, ExprContext *econtext, bool *isnull);
 static Datum ExecJustConst(ExprState *state, ExprContext *econtext, bool *isnull);
 static Datum ExecJustAssignInnerVar(ExprState *state, ExprContext *econtext, bool *isnull);
@@ -171,6 +177,14 @@ ExecReadyInterpretedExpr(ExprState *state)
 	if (state->flags & EEO_FLAG_INTERPRETER_INITIALIZED)
 		return;
 
+	/*
+	 * First time through, check whether attribute matches Var.  Might not be
+	 * ok anymore, due to schema changes. We do that by setting up a callback
+	 * that does checking on the first call, which then sets the evalfunc
+	 * callback to the actual method of execution.
+	 */
+	state->evalfunc = ExecInterpExprStillValid;
+
 	/* DIRECT_THREADED should not already be set */
 	Assert((state->flags & EEO_FLAG_DIRECT_THREADED) == 0);
 
@@ -192,53 +206,53 @@ ExecReadyInterpretedExpr(ExprState *state)
 		ExprEvalOp	step1 = state->steps[1].opcode;
 
 		if (step0 == EEOP_INNER_FETCHSOME &&
-			step1 == EEOP_INNER_VAR_FIRST)
+			step1 == EEOP_INNER_VAR)
 		{
-			state->evalfunc = ExecJustInnerVarFirst;
+			state->evalfunc_private = (void *) ExecJustInnerVar;
 			return;
 		}
 		else if (step0 == EEOP_OUTER_FETCHSOME &&
-				 step1 == EEOP_OUTER_VAR_FIRST)
+				 step1 == EEOP_OUTER_VAR)
 		{
-			state->evalfunc = ExecJustOuterVarFirst;
+			state->evalfunc_private = (void *) ExecJustOuterVar;
 			return;
 		}
 		else if (step0 == EEOP_SCAN_FETCHSOME &&
-				 step1 == EEOP_SCAN_VAR_FIRST)
+				 step1 == EEOP_SCAN_VAR)
 		{
-			state->evalfunc = ExecJustScanVarFirst;
+			state->evalfunc_private = (void *) ExecJustScanVar;
 			return;
 		}
 		else if (step0 == EEOP_INNER_FETCHSOME &&
 				 step1 == EEOP_ASSIGN_INNER_VAR)
 		{
-			state->evalfunc = ExecJustAssignInnerVar;
+			state->evalfunc_private = (void *) ExecJustAssignInnerVar;
 			return;
 		}
 		else if (step0 == EEOP_OUTER_FETCHSOME &&
 				 step1 == EEOP_ASSIGN_OUTER_VAR)
 		{
-			state->evalfunc = ExecJustAssignOuterVar;
+			state->evalfunc_private = (void *) ExecJustAssignOuterVar;
 			return;
 		}
 		else if (step0 == EEOP_SCAN_FETCHSOME &&
 				 step1 == EEOP_ASSIGN_SCAN_VAR)
 		{
-			state->evalfunc = ExecJustAssignScanVar;
+			state->evalfunc_private = (void *) ExecJustAssignScanVar;
 			return;
 		}
 		else if (step0 == EEOP_CASE_TESTVAL &&
 				 step1 == EEOP_FUNCEXPR_STRICT &&
 				 state->steps[0].d.casetest.value)
 		{
-			state->evalfunc = ExecJustApplyFuncToCase;
+			state->evalfunc_private = (void *) ExecJustApplyFuncToCase;
 			return;
 		}
 	}
 	else if (state->steps_len == 2 &&
 			 state->steps[0].opcode == EEOP_CONST)
 	{
-		state->evalfunc = ExecJustConst;
+		state->evalfunc_private = (void *) ExecJustConst;
 		return;
 	}
 
@@ -262,7 +276,7 @@ ExecReadyInterpretedExpr(ExprState *state)
 	}
 #endif							/* EEO_USE_COMPUTED_GOTO */
 
-	state->evalfunc = ExecInterpExpr;
+	state->evalfunc_private = (void *) ExecInterpExpr;
 }
 
 
@@ -293,11 +307,8 @@ ExecInterpExpr(ExprState *state, ExprContext *econtext, bool *isnull)
 		&&CASE_EEOP_INNER_FETCHSOME,
 		&&CASE_EEOP_OUTER_FETCHSOME,
 		&&CASE_EEOP_SCAN_FETCHSOME,
-		&&CASE_EEOP_INNER_VAR_FIRST,
 		&&CASE_EEOP_INNER_VAR,
-		&&CASE_EEOP_OUTER_VAR_FIRST,
 		&&CASE_EEOP_OUTER_VAR,
-		&&CASE_EEOP_SCAN_VAR_FIRST,
 		&&CASE_EEOP_SCAN_VAR,
 		&&CASE_EEOP_INNER_SYSVAR,
 		&&CASE_EEOP_OUTER_SYSVAR,
@@ -420,22 +431,6 @@ ExecInterpExpr(ExprState *state, ExprContext *econtext, bool *isnull)
 			EEO_NEXT();
 		}
 
-		EEO_CASE(EEOP_INNER_VAR_FIRST)
-		{
-			int			attnum = op->d.var.attnum;
-
-			/*
-			 * First time through, check whether attribute matches Var.  Might
-			 * not be ok anymore, due to schema changes.
-			 */
-			CheckVarSlotCompatibility(innerslot, attnum + 1, op->d.var.vartype);
-
-			/* Skip that check on subsequent evaluations */
-			op->opcode = EEO_OPCODE(EEOP_INNER_VAR);
-
-			/* FALL THROUGH to EEOP_INNER_VAR */
-		}
-
 		EEO_CASE(EEOP_INNER_VAR)
 		{
 			int			attnum = op->d.var.attnum;
@@ -453,18 +448,6 @@ ExecInterpExpr(ExprState *state, ExprContext *econtext, bool *isnull)
 			EEO_NEXT();
 		}
 
-		EEO_CASE(EEOP_OUTER_VAR_FIRST)
-		{
-			int			attnum = op->d.var.attnum;
-
-			/* See EEOP_INNER_VAR_FIRST comments */
-
-			CheckVarSlotCompatibility(outerslot, attnum + 1, op->d.var.vartype);
-			op->opcode = EEO_OPCODE(EEOP_OUTER_VAR);
-
-			/* FALL THROUGH to EEOP_OUTER_VAR */
-		}
-
 		EEO_CASE(EEOP_OUTER_VAR)
 		{
 			int			attnum = op->d.var.attnum;
@@ -478,18 +461,6 @@ ExecInterpExpr(ExprState *state, ExprContext *econtext, bool *isnull)
 			EEO_NEXT();
 		}
 
-		EEO_CASE(EEOP_SCAN_VAR_FIRST)
-		{
-			int			attnum = op->d.var.attnum;
-
-			/* See EEOP_INNER_VAR_FIRST comments */
-
-			CheckVarSlotCompatibility(scanslot, attnum + 1, op->d.var.vartype);
-			op->opcode = EEO_OPCODE(EEOP_SCAN_VAR);
-
-			/* FALL THROUGH to EEOP_SCAN_VAR */
-		}
-
 		EEO_CASE(EEOP_SCAN_VAR)
 		{
 			int			attnum = op->d.var.attnum;
@@ -1556,6 +1527,78 @@ out:
 	return state->resvalue;
 }
 
+/*
+ * Expression evaluation callback that performs extra checks before executing
+ * the expression. Declared extern so other methods of execution can use it
+ * too.
+ */
+Datum
+ExecInterpExprStillValid(ExprState *state, ExprContext *econtext, bool *isNull)
+{
+	/*
+	 * First time through, check whether attribute matches Var.  Might
+	 * not be ok anymore, due to schema changes.
+	 */
+	CheckExprStillValid(state, econtext);
+
+	/* skip the check during further executions */
+	state->evalfunc = (ExprStateEvalFunc) state->evalfunc_private;
+
+	/* and actually execute */
+	return state->evalfunc(state, econtext, isNull);
+}
+
+/*
+ * Check that an expression is still valid in the face of potential schema
+ * changes since the plan has been created.
+ */
+void
+CheckExprStillValid(ExprState *state, ExprContext *econtext)
+{
+	int i = 0;
+	TupleTableSlot *innerslot;
+	TupleTableSlot *outerslot;
+	TupleTableSlot *scanslot;
+
+	innerslot = econtext->ecxt_innertuple;
+	outerslot = econtext->ecxt_outertuple;
+	scanslot = econtext->ecxt_scantuple;
+
+	for (i = 0; i < state->steps_len;i++)
+	{
+		ExprEvalStep   *op = &state->steps[i];
+
+		switch (ExecEvalStepOp(state, op))
+		{
+			case EEOP_INNER_VAR:
+				{
+					int			attnum = op->d.var.attnum;
+
+					CheckVarSlotCompatibility(innerslot, attnum + 1, op->d.var.vartype);
+					break;
+				}
+
+			case EEOP_OUTER_VAR:
+				{
+					int			attnum = op->d.var.attnum;
+
+					CheckVarSlotCompatibility(outerslot, attnum + 1, op->d.var.vartype);
+					break;
+				}
+
+			case EEOP_SCAN_VAR:
+				{
+					int			attnum = op->d.var.attnum;
+
+					CheckVarSlotCompatibility(scanslot, attnum + 1, op->d.var.vartype);
+					break;
+				}
+			default:
+				break;
+		}
+	}
+}
+
 /*
  * Check whether a user attribute in a slot can be referenced by a Var
  * expression.  This should succeed unless there have been schema changes
@@ -1668,20 +1711,14 @@ ShutdownTupleDescRef(Datum arg)
  * Fast-path functions, for very simple expressions
  */
 
-/* Simple reference to inner Var, first time through */
+/* Simple reference to inner Var */
 static Datum
-ExecJustInnerVarFirst(ExprState *state, ExprContext *econtext, bool *isnull)
+ExecJustInnerVar(ExprState *state, ExprContext *econtext, bool *isnull)
 {
 	ExprEvalStep *op = &state->steps[1];
 	int			attnum = op->d.var.attnum + 1;
 	TupleTableSlot *slot = econtext->ecxt_innertuple;
 
-	/* See ExecInterpExpr()'s comments for EEOP_INNER_VAR_FIRST */
-
-	CheckVarSlotCompatibility(slot, attnum, op->d.var.vartype);
-	op->opcode = EEOP_INNER_VAR;	/* just for cleanliness */
-	state->evalfunc = ExecJustInnerVar;
-
 	/*
 	 * Since we use slot_getattr(), we don't need to implement the FETCHSOME
 	 * step explicitly, and we also needn't Assert that the attnum is in range
@@ -1690,34 +1727,6 @@ ExecJustInnerVarFirst(ExprState *state, ExprContext *econtext, bool *isnull)
 	return slot_getattr(slot, attnum, isnull);
 }
 
-/* Simple reference to inner Var */
-static Datum
-ExecJustInnerVar(ExprState *state, ExprContext *econtext, bool *isnull)
-{
-	ExprEvalStep *op = &state->steps[1];
-	int			attnum = op->d.var.attnum + 1;
-	TupleTableSlot *slot = econtext->ecxt_innertuple;
-
-	/* See comments in ExecJustInnerVarFirst */
-	return slot_getattr(slot, attnum, isnull);
-}
-
-/* Simple reference to outer Var, first time through */
-static Datum
-ExecJustOuterVarFirst(ExprState *state, ExprContext *econtext, bool *isnull)
-{
-	ExprEvalStep *op = &state->steps[1];
-	int			attnum = op->d.var.attnum + 1;
-	TupleTableSlot *slot = econtext->ecxt_outertuple;
-
-	CheckVarSlotCompatibility(slot, attnum, op->d.var.vartype);
-	op->opcode = EEOP_OUTER_VAR;	/* just for cleanliness */
-	state->evalfunc = ExecJustOuterVar;
-
-	/* See comments in ExecJustInnerVarFirst */
-	return slot_getattr(slot, attnum, isnull);
-}
-
 /* Simple reference to outer Var */
 static Datum
 ExecJustOuterVar(ExprState *state, ExprContext *econtext, bool *isnull)
@@ -1726,23 +1735,7 @@ ExecJustOuterVar(ExprState *state, ExprContext *econtext, bool *isnull)
 	int			attnum = op->d.var.attnum + 1;
 	TupleTableSlot *slot = econtext->ecxt_outertuple;
 
-	/* See comments in ExecJustInnerVarFirst */
-	return slot_getattr(slot, attnum, isnull);
-}
-
-/* Simple reference to scan Var, first time through */
-static Datum
-ExecJustScanVarFirst(ExprState *state, ExprContext *econtext, bool *isnull)
-{
-	ExprEvalStep *op = &state->steps[1];
-	int			attnum = op->d.var.attnum + 1;
-	TupleTableSlot *slot = econtext->ecxt_scantuple;
-
-	CheckVarSlotCompatibility(slot, attnum, op->d.var.vartype);
-	op->opcode = EEOP_SCAN_VAR; /* just for cleanliness */
-	state->evalfunc = ExecJustScanVar;
-
-	/* See comments in ExecJustInnerVarFirst */
+	/* See comments in ExecJustInnerVar */
 	return slot_getattr(slot, attnum, isnull);
 }
 
@@ -1754,7 +1747,7 @@ ExecJustScanVar(ExprState *state, ExprContext *econtext, bool *isnull)
 	int			attnum = op->d.var.attnum + 1;
 	TupleTableSlot *slot = econtext->ecxt_scantuple;
 
-	/* See comments in ExecJustInnerVarFirst */
+	/* See comments in ExecJustInnerVar */
 	return slot_getattr(slot, attnum, isnull);
 }
 
@@ -1860,6 +1853,24 @@ ExecJustApplyFuncToCase(ExprState *state, ExprContext *econtext, bool *isnull)
 	return d;
 }
 
+#if defined(EEO_USE_COMPUTED_GOTO)
+/*
+ * Comparator used when building address->opcode lookup table for
+ * ExecEvalStepOp() in the threaded dispatch case.
+ */
+static int
+dispatch_compare_ptr(const void* a, const void *b)
+{
+	const ExprEvalOpLookup *la = (const ExprEvalOpLookup *) a;
+	const ExprEvalOpLookup *lb = (const ExprEvalOpLookup *) b;
+
+	if (la->opcode < lb->opcode)
+		return -1;
+	else if (la->opcode > lb->opcode)
+		return 1;
+	return 0;
+}
+#endif
 
 /*
  * Do one-time initialization of interpretation machinery.
@@ -1870,8 +1881,25 @@ ExecInitInterpreter(void)
 #if defined(EEO_USE_COMPUTED_GOTO)
 	/* Set up externally-visible pointer to dispatch table */
 	if (dispatch_table == NULL)
+	{
+		int			i;
+
 		dispatch_table = (const void **)
 			DatumGetPointer(ExecInterpExpr(NULL, NULL, NULL));
+
+		/* build reverse lookup table */
+		for (i = 0; i < EEOP_LAST; i++)
+		{
+			reverse_dispatch_table[i].opcode = dispatch_table[i];
+			reverse_dispatch_table[i].op = (ExprEvalOp) i;
+		}
+
+		/* make it bsearch()able */
+		qsort(reverse_dispatch_table,
+			  EEOP_LAST /* nmembers */,
+			  sizeof(ExprEvalOpLookup),
+			  dispatch_compare_ptr);
+	}
 #endif
 }
 
@@ -1880,10 +1908,6 @@ ExecInitInterpreter(void)
  *
  * When direct-threading is in use, ExprState->opcode isn't easily
  * decipherable. This function returns the appropriate enum member.
- *
- * This currently is only supposed to be used in paths that aren't critical
- * performance-wise.  If that changes, we could add an inverse dispatch_table
- * that's sorted on the address, so a binary search can be performed.
  */
 ExprEvalOp
 ExecEvalStepOp(ExprState *state, ExprEvalStep *op)
@@ -1891,16 +1915,17 @@ ExecEvalStepOp(ExprState *state, ExprEvalStep *op)
 #if defined(EEO_USE_COMPUTED_GOTO)
 	if (state->flags & EEO_FLAG_DIRECT_THREADED)
 	{
-		int			i;
-
-		for (i = 0; i < EEOP_LAST; i++)
-		{
-			if ((void *) op->opcode == dispatch_table[i])
-			{
-				return (ExprEvalOp) i;
-			}
-		}
-		elog(ERROR, "unknown opcode");
+		ExprEvalOpLookup key;
+		ExprEvalOpLookup *res;
+
+		key.opcode =  (void *) op->opcode;
+		res = bsearch(&key,
+					  reverse_dispatch_table,
+					  EEOP_LAST /* nmembers */,
+					  sizeof(ExprEvalOpLookup),
+					  dispatch_compare_ptr);
+		Assert(res); /* unknown ops shouldn't get looked up */
+		return res->op;
 	}
 #endif
 	return (ExprEvalOp) op->opcode;

src/include/executor/execExpr.h

@@ -51,12 +51,8 @@ typedef enum ExprEvalOp
 	EEOP_SCAN_FETCHSOME,
 
 	/* compute non-system Var value */
-	/* "FIRST" variants are used only the first time through */
-	EEOP_INNER_VAR_FIRST,
 	EEOP_INNER_VAR,
-	EEOP_OUTER_VAR_FIRST,
 	EEOP_OUTER_VAR,
-	EEOP_SCAN_VAR_FIRST,
 	EEOP_SCAN_VAR,
 
 	/* compute system Var value */
@@ -67,8 +63,11 @@ typedef enum ExprEvalOp
 	/* compute wholerow Var */
 	EEOP_WHOLEROW,
 
-	/* compute non-system Var value, assign it into ExprState's resultslot */
-	/* (these are not used if _FIRST checks would be needed) */
+	/*
+	 * Compute non-system Var value, assign it into ExprState's
+	 * resultslot. These are not used if a CheckVarSlotCompatibility() check
+	 * would be needed.
+	 */
 	EEOP_ASSIGN_INNER_VAR,
 	EEOP_ASSIGN_OUTER_VAR,
 	EEOP_ASSIGN_SCAN_VAR,
@@ -621,6 +620,9 @@ extern void ExprEvalPushStep(ExprState *es, const ExprEvalStep *s);
 extern void ExecReadyInterpretedExpr(ExprState *state);
 extern ExprEvalOp ExecEvalStepOp(ExprState *state, ExprEvalStep *op);
 
+extern Datum ExecInterpExprStillValid(ExprState *state, ExprContext *econtext, bool *isNull);
+extern void CheckExprStillValid(ExprState *state, ExprContext *econtext);
+
 /*
  * Non fast-path execution functions. These are externs instead of statics in
  * execExprInterp.c, because that allows them to be used by other methods of

src/include/nodes/execnodes.h

@@ -87,6 +87,9 @@ typedef struct ExprState
 	/* original expression tree, for debugging only */
 	Expr	   *expr;
 
+	/* private state for an evalfunc */
+	void	   *evalfunc_private;
+
 	/*
 	 * XXX: following fields only needed during "compilation" (ExecInitExpr);
 	 * could be thrown away afterwards.