From: Tom Lane Date: Thu, 30 Jan 2025 18:21:42 +0000 (-0500) Subject: Simplify executor's handling of CaseTestExpr & CoerceToDomainValue. X-Git-Tag: REL_18_BETA1~983 X-Git-Url: http://git.postgresql.org/gitweb/?a=commitdiff_plain;h=115a365519bfd53a65bf17d253b26902eff0c337;p=postgresql.git Simplify executor's handling of CaseTestExpr & CoerceToDomainValue. Instead of deciding at runtime whether to read from casetest.value or caseValue_datum, split EEOP_CASE_TESTVAL into two opcodes and make the decision during expression compilation. Similarly for EEOP_DOMAIN_TESTVAL. This actually results in net less code, mainly because llvmjit_expr.c's code for handling these opcodes gets shorter. The performance gain is doubtless negligible, but this seems worth changing anyway on grounds of simplicity and understandability. Author: Andreas Karlsson Co-authored-by: Xing Guo Reviewed-by: Tom Lane Discussion: https://postgr.es/m/CACpMh+AiBYAWn+D1aU7Rsy-V1tox06Cbc0H3qA7rwL5zdJ=anQ@mail.gmail.com --- diff --git a/src/backend/executor/execExpr.c b/src/backend/executor/execExpr.c index 8f28da4bf94..03566c4d181 100644 --- a/src/backend/executor/execExpr.c +++ b/src/backend/executor/execExpr.c @@ -1901,13 +1901,17 @@ ExecInitExprRec(Expr *node, ExprState *state, * actually within a CaseExpr, ArrayCoerceExpr, etc structure. * That can happen because some parts of the system abuse * CaseTestExpr to cause a read of a value externally supplied - * in econtext->caseValue_datum. We'll take care of that - * scenario at runtime. + * in econtext->caseValue_datum. We'll take care of that by + * generating a specialized operation. */ - scratch.opcode = EEOP_CASE_TESTVAL; - scratch.d.casetest.value = state->innermost_caseval; - scratch.d.casetest.isnull = state->innermost_casenull; - + if (state->innermost_caseval == NULL) + scratch.opcode = EEOP_CASE_TESTVAL_EXT; + else + { + scratch.opcode = EEOP_CASE_TESTVAL; + scratch.d.casetest.value = state->innermost_caseval; + scratch.d.casetest.isnull = state->innermost_casenull; + } ExprEvalPushStep(state, &scratch); break; } @@ -2594,14 +2598,18 @@ ExecInitExprRec(Expr *node, ExprState *state, * that innermost_domainval could be NULL, if we're compiling * a standalone domain check rather than one embedded in a * larger expression. In that case we must read from - * econtext->domainValue_datum. We'll take care of that - * scenario at runtime. + * econtext->domainValue_datum. We'll take care of that by + * generating a specialized operation. */ - scratch.opcode = EEOP_DOMAIN_TESTVAL; - /* we share instruction union variant with case testval */ - scratch.d.casetest.value = state->innermost_domainval; - scratch.d.casetest.isnull = state->innermost_domainnull; - + if (state->innermost_domainval == NULL) + scratch.opcode = EEOP_DOMAIN_TESTVAL_EXT; + else + { + scratch.opcode = EEOP_DOMAIN_TESTVAL; + /* we share instruction union variant with case testval */ + scratch.d.casetest.value = state->innermost_domainval; + scratch.d.casetest.isnull = state->innermost_domainnull; + } ExprEvalPushStep(state, &scratch); break; } diff --git a/src/backend/executor/execExprInterp.c b/src/backend/executor/execExprInterp.c index 1127e6f11eb..09f6a5f14c1 100644 --- a/src/backend/executor/execExprInterp.c +++ b/src/backend/executor/execExprInterp.c @@ -365,8 +365,7 @@ ExecReadyInterpretedExpr(ExprState *state) return; } else if (step0 == EEOP_CASE_TESTVAL && - step1 == EEOP_FUNCEXPR_STRICT && - state->steps[0].d.casetest.value) + step1 == EEOP_FUNCEXPR_STRICT) { state->evalfunc_private = ExecJustApplyFuncToCase; return; @@ -524,6 +523,7 @@ ExecInterpExpr(ExprState *state, ExprContext *econtext, bool *isnull) &&CASE_EEOP_PARAM_CALLBACK, &&CASE_EEOP_PARAM_SET, &&CASE_EEOP_CASE_TESTVAL, + &&CASE_EEOP_CASE_TESTVAL_EXT, &&CASE_EEOP_MAKE_READONLY, &&CASE_EEOP_IOCOERCE, &&CASE_EEOP_IOCOERCE_SAFE, @@ -548,6 +548,7 @@ ExecInterpExpr(ExprState *state, ExprContext *econtext, bool *isnull) &&CASE_EEOP_SBSREF_ASSIGN, &&CASE_EEOP_SBSREF_FETCH, &&CASE_EEOP_DOMAIN_TESTVAL, + &&CASE_EEOP_DOMAIN_TESTVAL_EXT, &&CASE_EEOP_DOMAIN_NOTNULL, &&CASE_EEOP_DOMAIN_CHECK, &&CASE_EEOP_HASHDATUM_SET_INITVAL, @@ -1273,44 +1274,16 @@ ExecInterpExpr(ExprState *state, ExprContext *econtext, bool *isnull) EEO_CASE(EEOP_CASE_TESTVAL) { - /* - * Normally upper parts of the expression tree have setup the - * values to be returned here, but some parts of the system - * currently misuse {caseValue,domainValue}_{datum,isNull} to set - * run-time data. So if no values have been set-up, use - * ExprContext's. This isn't pretty, but also not *that* ugly, - * and this is unlikely to be performance sensitive enough to - * worry about an extra branch. - */ - if (op->d.casetest.value) - { - *op->resvalue = *op->d.casetest.value; - *op->resnull = *op->d.casetest.isnull; - } - else - { - *op->resvalue = econtext->caseValue_datum; - *op->resnull = econtext->caseValue_isNull; - } + *op->resvalue = *op->d.casetest.value; + *op->resnull = *op->d.casetest.isnull; EEO_NEXT(); } - EEO_CASE(EEOP_DOMAIN_TESTVAL) + EEO_CASE(EEOP_CASE_TESTVAL_EXT) { - /* - * See EEOP_CASE_TESTVAL comment. - */ - if (op->d.casetest.value) - { - *op->resvalue = *op->d.casetest.value; - *op->resnull = *op->d.casetest.isnull; - } - else - { - *op->resvalue = econtext->domainValue_datum; - *op->resnull = econtext->domainValue_isNull; - } + *op->resvalue = econtext->caseValue_datum; + *op->resnull = econtext->caseValue_isNull; EEO_NEXT(); } @@ -1726,6 +1699,22 @@ ExecInterpExpr(ExprState *state, ExprContext *econtext, bool *isnull) EEO_NEXT(); } + EEO_CASE(EEOP_DOMAIN_TESTVAL) + { + *op->resvalue = *op->d.casetest.value; + *op->resnull = *op->d.casetest.isnull; + + EEO_NEXT(); + } + + EEO_CASE(EEOP_DOMAIN_TESTVAL_EXT) + { + *op->resvalue = econtext->domainValue_datum; + *op->resnull = econtext->domainValue_isNull; + + EEO_NEXT(); + } + EEO_CASE(EEOP_DOMAIN_NOTNULL) { /* too complex for an inline implementation */ diff --git a/src/backend/jit/llvm/llvmjit_expr.c b/src/backend/jit/llvm/llvmjit_expr.c index c1cf34f1034..f0f5c3bd49f 100644 --- a/src/backend/jit/llvm/llvmjit_expr.c +++ b/src/backend/jit/llvm/llvmjit_expr.c @@ -1265,41 +1265,30 @@ llvm_compile_expr(ExprState *state) case EEOP_CASE_TESTVAL: { - LLVMBasicBlockRef b_avail, - b_notavail; LLVMValueRef v_casevaluep, v_casevalue; LLVMValueRef v_casenullp, v_casenull; - LLVMValueRef v_casevaluenull; - - b_avail = l_bb_before_v(opblocks[opno + 1], - "op.%d.avail", opno); - b_notavail = l_bb_before_v(opblocks[opno + 1], - "op.%d.notavail", opno); v_casevaluep = l_ptr_const(op->d.casetest.value, l_ptr(TypeSizeT)); v_casenullp = l_ptr_const(op->d.casetest.isnull, l_ptr(TypeStorageBool)); - v_casevaluenull = - LLVMBuildICmp(b, LLVMIntEQ, - LLVMBuildPtrToInt(b, v_casevaluep, - TypeSizeT, ""), - l_sizet_const(0), ""); - LLVMBuildCondBr(b, v_casevaluenull, b_notavail, b_avail); - - /* if casetest != NULL */ - LLVMPositionBuilderAtEnd(b, b_avail); v_casevalue = l_load(b, TypeSizeT, v_casevaluep, ""); v_casenull = l_load(b, TypeStorageBool, v_casenullp, ""); LLVMBuildStore(b, v_casevalue, v_resvaluep); LLVMBuildStore(b, v_casenull, v_resnullp); + LLVMBuildBr(b, opblocks[opno + 1]); + break; + } + + case EEOP_CASE_TESTVAL_EXT: + { + LLVMValueRef v_casevalue; + LLVMValueRef v_casenull; - /* if casetest == NULL */ - LLVMPositionBuilderAtEnd(b, b_notavail); v_casevalue = l_load_struct_gep(b, StructExprContext, @@ -1958,43 +1947,30 @@ llvm_compile_expr(ExprState *state) case EEOP_DOMAIN_TESTVAL: { - LLVMBasicBlockRef b_avail, - b_notavail; LLVMValueRef v_casevaluep, v_casevalue; LLVMValueRef v_casenullp, v_casenull; - LLVMValueRef v_casevaluenull; - - b_avail = l_bb_before_v(opblocks[opno + 1], - "op.%d.avail", opno); - b_notavail = l_bb_before_v(opblocks[opno + 1], - "op.%d.notavail", opno); v_casevaluep = l_ptr_const(op->d.casetest.value, l_ptr(TypeSizeT)); v_casenullp = l_ptr_const(op->d.casetest.isnull, l_ptr(TypeStorageBool)); - v_casevaluenull = - LLVMBuildICmp(b, LLVMIntEQ, - LLVMBuildPtrToInt(b, v_casevaluep, - TypeSizeT, ""), - l_sizet_const(0), ""); - LLVMBuildCondBr(b, - v_casevaluenull, - b_notavail, b_avail); - - /* if casetest != NULL */ - LLVMPositionBuilderAtEnd(b, b_avail); v_casevalue = l_load(b, TypeSizeT, v_casevaluep, ""); v_casenull = l_load(b, TypeStorageBool, v_casenullp, ""); LLVMBuildStore(b, v_casevalue, v_resvaluep); LLVMBuildStore(b, v_casenull, v_resnullp); + LLVMBuildBr(b, opblocks[opno + 1]); + break; + } + + case EEOP_DOMAIN_TESTVAL_EXT: + { + LLVMValueRef v_casevalue; + LLVMValueRef v_casenull; - /* if casetest == NULL */ - LLVMPositionBuilderAtEnd(b, b_notavail); v_casevalue = l_load_struct_gep(b, StructExprContext, diff --git a/src/include/executor/execExpr.h b/src/include/executor/execExpr.h index 5371e344ecd..51bd35dcb07 100644 --- a/src/include/executor/execExpr.h +++ b/src/include/executor/execExpr.h @@ -173,6 +173,7 @@ typedef enum ExprEvalOp /* return CaseTestExpr value */ EEOP_CASE_TESTVAL, + EEOP_CASE_TESTVAL_EXT, /* apply MakeExpandedObjectReadOnly() to target value */ EEOP_MAKE_READONLY, @@ -237,6 +238,7 @@ typedef enum ExprEvalOp /* evaluate value for CoerceToDomainValue */ EEOP_DOMAIN_TESTVAL, + EEOP_DOMAIN_TESTVAL_EXT, /* evaluate a domain's NOT NULL constraint */ EEOP_DOMAIN_NOTNULL,