From 1aed0dc46f63b1332dfaad9b40a8c08250227b87 Mon Sep 17 00:00:00 2001 From: Andres Freund Date: Thu, 23 Feb 2023 17:48:12 -0800 Subject: [PATCH v2] WIP: generate explicit expression step to evaluate correlated MULTIEXPR_SUBLINK --- src/include/executor/execExpr.h | 6 ++- src/include/executor/nodeSubplan.h | 2 + src/backend/executor/execExpr.c | 78 +++++++++++++++++++++++++++ src/backend/executor/execExprInterp.c | 21 ++++++++ src/backend/executor/nodeSubplan.c | 25 ++++++++- src/backend/jit/llvm/llvmjit_expr.c | 6 +++ src/backend/jit/llvm/llvmjit_types.c | 1 + 7 files changed, 137 insertions(+), 2 deletions(-) diff --git a/src/include/executor/execExpr.h b/src/include/executor/execExpr.h index 06c3adc0a19..faa806e1a16 100644 --- a/src/include/executor/execExpr.h +++ b/src/include/executor/execExpr.h @@ -256,6 +256,8 @@ typedef enum ExprEvalOp EEOP_AGG_ORDERED_TRANS_DATUM, EEOP_AGG_ORDERED_TRANS_TUPLE, + EEOP_SUBPLAN_PARAMS, + /* non-existent operation, used e.g. to check array lengths */ EEOP_LAST } ExprEvalOp; @@ -607,7 +609,7 @@ typedef struct ExprEvalStep WindowFuncExprState *wfstate; } window_func; - /* for EEOP_SUBPLAN */ + /* for EEOP_SUBPLAN and EEOP_SUBPLAN_PARAMS */ struct { /* out-of-line state, created by nodeSubplan.c */ @@ -766,6 +768,8 @@ extern void ExecEvalXmlExpr(ExprState *state, ExprEvalStep *op); extern void ExecEvalGroupingFunc(ExprState *state, ExprEvalStep *op); extern void ExecEvalSubPlan(ExprState *state, ExprEvalStep *op, ExprContext *econtext); +extern void ExecEvalSubPlanParams(ExprState *state, ExprEvalStep *op, + ExprContext *econtext); extern void ExecEvalWholeRowVar(ExprState *state, ExprEvalStep *op, ExprContext *econtext); extern void ExecEvalSysVar(ExprState *state, ExprEvalStep *op, diff --git a/src/include/executor/nodeSubplan.h b/src/include/executor/nodeSubplan.h index 3f0e2bba2b7..cf369c239a6 100644 --- a/src/include/executor/nodeSubplan.h +++ b/src/include/executor/nodeSubplan.h @@ -26,4 +26,6 @@ extern void ExecSetParamPlan(SubPlanState *node, ExprContext *econtext); extern void ExecSetParamPlanMulti(const Bitmapset *params, ExprContext *econtext); +extern bool IsCorrelatedMultiSubPlan(SubPlan *subplan); + #endif /* NODESUBPLAN_H */ diff --git a/src/backend/executor/execExpr.c b/src/backend/executor/execExpr.c index 812ead95bc6..b176001bea8 100644 --- a/src/backend/executor/execExpr.c +++ b/src/backend/executor/execExpr.c @@ -52,11 +52,13 @@ #include "utils/typcache.h" +/* FIXME: rename */ typedef struct LastAttnumInfo { AttrNumber last_inner; AttrNumber last_outer; AttrNumber last_scan; + List *multiexpr_subplans; } LastAttnumInfo; static void ExecReadyExpr(ExprState *state); @@ -444,6 +446,12 @@ ExecBuildProjectionInfo(List *targetList, scratch.d.assign_var.resultnum = tle->resno - 1; ExprEvalPushStep(state, &scratch); } + else if (IsA(tle->expr, SubPlan) && + IsCorrelatedMultiSubPlan((SubPlan *) tle->expr)) + { + /* correlated multiexprs already have been handled */ + continue; + } else { /* @@ -679,6 +687,9 @@ ExecBuildUpdateProjection(List *targetList, /* * If we're evaluating the tlist, must evaluate any resjunk columns too. * (This matters for things like MULTIEXPR_SUBLINK SubPlans.) + * + * FIXME: Are there any cases where we still need to do this, given the + * multiexpr case is already handled? */ if (evalTargetList) { @@ -687,6 +698,12 @@ ExecBuildUpdateProjection(List *targetList, TargetEntry *tle = lfirst_node(TargetEntry, lc); Assert(tle->resjunk); + + /* correlated multiexprs already have been handled */ + if (tle->resjunk && IsA(tle->expr, SubPlan) && + IsCorrelatedMultiSubPlan((SubPlan *) tle->expr)) + continue; + ExecInitExprRec(tle->expr, state, &state->resvalue, &state->resnull); } @@ -1405,6 +1422,9 @@ ExecInitExprRec(Expr *node, ExprState *state, if (!state->parent) elog(ERROR, "SubPlan found with no parent plan"); + /* needs to already have been evaluated */ + Assert(!IsCorrelatedMultiSubPlan(subplan)); + sstate = ExecInitSubPlan(subplan, state->parent); /* add SubPlanState nodes to state->parent->subPlan */ @@ -2544,6 +2564,8 @@ ExecInitFunc(ExprEvalStep *scratch, Expr *node, List *args, Oid funcid, /* * Add expression steps deforming the ExprState's inner/outer/scan slots * as much as required by the expression. + * + * FIXME: rename */ static void ExecInitExprSlots(ExprState *state, Node *node) @@ -2562,11 +2584,14 @@ ExecInitExprSlots(ExprState *state, Node *node) * Add steps deforming the ExprState's inner/out/scan slots as much as * indicated by info. This is useful when building an ExprState covering more * than one expression. + * + * FIXME: rename */ static void ExecPushExprSlots(ExprState *state, LastAttnumInfo *info) { ExprEvalStep scratch = {0}; + ListCell *lc; scratch.resvalue = NULL; scratch.resnull = NULL; @@ -2602,10 +2627,37 @@ ExecPushExprSlots(ExprState *state, LastAttnumInfo *info) if (ExecComputeSlotInfo(state, &scratch)) ExprEvalPushStep(state, &scratch); } + + /* + * FIXME: This obviously isn't the right place, but for some reason + * ExecBuildUpdateProjection() duplicates the get_last_attnums_walker + * invocation. + */ + foreach(lc, info->multiexpr_subplans) + { + SubPlan *sp = (SubPlan *) lfirst(lc); + SubPlanState *sstate; + + if (!state->parent) + elog(ERROR, "SubPlan found with no parent plan"); + + sstate = ExecInitSubPlan(sp, state->parent); + + /* add SubPlanState nodes to state->parent->subPlan */ + state->parent->subPlan = lappend(state->parent->subPlan, + sstate); + + scratch.opcode = EEOP_SUBPLAN_PARAMS; + scratch.d.subplan.sstate = sstate; + ExprEvalPushStep(state, &scratch); + } + } /* * get_last_attnums_walker: expression walker for ExecInitExprSlots + * + * FIXME: rename */ static bool get_last_attnums_walker(Node *node, LastAttnumInfo *info) @@ -2636,6 +2688,32 @@ get_last_attnums_walker(Node *node, LastAttnumInfo *info) return false; } + /* + * Collect all MULTIEXPR subplans. We want generate SUBPLAN_PARAMS steps + * before we could encounter a Param for the subplan. That can + * conveniently be done here, since we already do a full expression tree + * traversal. + * + * FIXME: Is it worth building bitmasks of used paramids to avoid + * unnecessary evaluation of a SubPlan? I don't think they should be + * emitted into the targetlist if not referenced? + * + * FIXME: I think this may make sense for initplans as well, not just for + * the MULTIEXPR case. + * + * XXX: This is probably worth doing for other kinds of subplans as well. + */ + if (IsA(node, SubPlan)) + { + SubPlan *subplan = (SubPlan *) node; + + if (IsCorrelatedMultiSubPlan(subplan)) + { + info->multiexpr_subplans = lappend(info->multiexpr_subplans, + subplan); + } + } + /* * Don't examine the arguments or filters of Aggrefs or WindowFuncs, * because those do not represent expressions to be evaluated within the diff --git a/src/backend/executor/execExprInterp.c b/src/backend/executor/execExprInterp.c index 19351fe34bf..ebae1ef423d 100644 --- a/src/backend/executor/execExprInterp.c +++ b/src/backend/executor/execExprInterp.c @@ -493,6 +493,7 @@ ExecInterpExpr(ExprState *state, ExprContext *econtext, bool *isnull) &&CASE_EEOP_AGG_PRESORTED_DISTINCT_MULTI, &&CASE_EEOP_AGG_ORDERED_TRANS_DATUM, &&CASE_EEOP_AGG_ORDERED_TRANS_TUPLE, + &&CASE_EEOP_SUBPLAN_PARAMS, &&CASE_EEOP_LAST }; @@ -1804,6 +1805,14 @@ ExecInterpExpr(ExprState *state, ExprContext *econtext, bool *isnull) EEO_NEXT(); } + EEO_CASE(EEOP_SUBPLAN_PARAMS) + { + /* too complex for an inline implementation */ + ExecEvalSubPlanParams(state, op, econtext); + + EEO_NEXT(); + } + EEO_CASE(EEOP_LAST) { /* unreachable */ @@ -3912,6 +3921,18 @@ ExecEvalSubPlan(ExprState *state, ExprEvalStep *op, ExprContext *econtext) *op->resvalue = ExecSubPlan(sstate, econtext, op->resnull); } +void +ExecEvalSubPlanParams(ExprState *state, ExprEvalStep *op, + ExprContext *econtext) +{ + SubPlanState *sstate = op->d.subplan.sstate; + + /* could potentially be nested, so make sure there's enough stack */ + check_stack_depth(); + + ExecSetParamPlan(sstate, econtext); +} + /* * Evaluate a wholerow Var expression. * diff --git a/src/backend/executor/nodeSubplan.c b/src/backend/executor/nodeSubplan.c index 64af36a1873..8d0c9a79a3b 100644 --- a/src/backend/executor/nodeSubplan.c +++ b/src/backend/executor/nodeSubplan.c @@ -265,6 +265,13 @@ ExecScanSubPlan(SubPlanState *node, { EState *estate = node->parent->state; + /* + * ExecScanSubPlan() shouldn't be called for correlated subqueries, + * execExpr should have handled the case internally. Note we may get + * called for non-correlated MULTIEXPR. + */ + Assert(!IsCorrelatedMultiSubPlan(subplan)); + foreach(l, subplan->setParam) { int paramid = lfirst_int(l); @@ -868,7 +875,9 @@ ExecInitSubPlan(SubPlan *subplan, PlanState *parent) int paramid = lfirst_int(lst); ParamExecData *prm = &(estate->es_param_exec_vals[paramid]); - prm->execPlan = sstate; + /* MULTIEXPR gets automatically invoked when necessary via execExpr.c */ + if (!IsCorrelatedMultiSubPlan(subplan)) + prm->execPlan = sstate; } } @@ -1326,3 +1335,17 @@ ExecReScanSetParamPlan(SubPlanState *node, PlanState *parent) parent->chgParam = bms_add_member(parent->chgParam, paramid); } } + +/* + * XXX: Where should this be? Perhaps SubPlan should just indicate this more + * conveniently? + */ +bool +IsCorrelatedMultiSubPlan(SubPlan *subplan) +{ + if (subplan->subLinkType != MULTIEXPR_SUBLINK) + return false; + + /* can't be correlated if there are no input parameters */ + return subplan->parParam != NULL; +} diff --git a/src/backend/jit/llvm/llvmjit_expr.c b/src/backend/jit/llvm/llvmjit_expr.c index 1c722c79552..70d8c328255 100644 --- a/src/backend/jit/llvm/llvmjit_expr.c +++ b/src/backend/jit/llvm/llvmjit_expr.c @@ -1904,6 +1904,12 @@ llvm_compile_expr(ExprState *state) LLVMBuildBr(b, opblocks[opno + 1]); break; + case EEOP_SUBPLAN_PARAMS: + build_EvalXFunc(b, mod, "ExecEvalSubPlanParams", + v_state, op, v_econtext); + LLVMBuildBr(b, opblocks[opno + 1]); + break; + case EEOP_AGG_STRICT_DESERIALIZE: case EEOP_AGG_DESERIALIZE: { diff --git a/src/backend/jit/llvm/llvmjit_types.c b/src/backend/jit/llvm/llvmjit_types.c index 876fb640294..048029c5d0b 100644 --- a/src/backend/jit/llvm/llvmjit_types.c +++ b/src/backend/jit/llvm/llvmjit_types.c @@ -129,6 +129,7 @@ void *referenced_functions[] = ExecEvalScalarArrayOp, ExecEvalHashedScalarArrayOp, ExecEvalSubPlan, + ExecEvalSubPlanParams, ExecEvalSysVar, ExecEvalWholeRowVar, ExecEvalXmlExpr, -- 2.38.0