Skip to content

Commit 115a365

Browse files
tglsfdchiguoxing
tglsfdc
and
higuoxing
committed
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 <andreas@proxel.se> Co-authored-by: Xing Guo <higuoxing@gmail.com> Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us> Discussion: https://postgr.es/m/CACpMh+AiBYAWn+D1aU7Rsy-V1tox06Cbc0H3qA7rwL5zdJ=anQ@mail.gmail.com
1 parent 6252b1e commit 115a365

File tree

4 files changed

+63
-88
lines changed

4 files changed

+63
-88
lines changed

‎src/backend/executor/execExpr.c‎

Lines changed: 21 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -1901,13 +1901,17 @@ ExecInitExprRec(Expr *node, ExprState *state,
19011901
* actually within a CaseExpr, ArrayCoerceExpr, etc structure.
19021902
* That can happen because some parts of the system abuse
19031903
* CaseTestExpr to cause a read of a value externally supplied
1904-
* in econtext->caseValue_datum. We'll take care of that
1905-
* scenario at runtime.
1904+
* in econtext->caseValue_datum. We'll take care of that by
1905+
* generating a specialized operation.
19061906
*/
1907-
scratch.opcode = EEOP_CASE_TESTVAL;
1908-
scratch.d.casetest.value = state->innermost_caseval;
1909-
scratch.d.casetest.isnull = state->innermost_casenull;
1910-
1907+
if (state->innermost_caseval == NULL)
1908+
scratch.opcode = EEOP_CASE_TESTVAL_EXT;
1909+
else
1910+
{
1911+
scratch.opcode = EEOP_CASE_TESTVAL;
1912+
scratch.d.casetest.value = state->innermost_caseval;
1913+
scratch.d.casetest.isnull = state->innermost_casenull;
1914+
}
19111915
ExprEvalPushStep(state, &scratch);
19121916
break;
19131917
}
@@ -2594,14 +2598,18 @@ ExecInitExprRec(Expr *node, ExprState *state,
25942598
* that innermost_domainval could be NULL, if we're compiling
25952599
* a standalone domain check rather than one embedded in a
25962600
* larger expression. In that case we must read from
2597-
* econtext->domainValue_datum. We'll take care of that
2598-
* scenario at runtime.
2601+
* econtext->domainValue_datum. We'll take care of that by
2602+
* generating a specialized operation.
25992603
*/
2600-
scratch.opcode = EEOP_DOMAIN_TESTVAL;
2601-
/* we share instruction union variant with case testval */
2602-
scratch.d.casetest.value = state->innermost_domainval;
2603-
scratch.d.casetest.isnull = state->innermost_domainnull;
2604-
2604+
if (state->innermost_domainval == NULL)
2605+
scratch.opcode = EEOP_DOMAIN_TESTVAL_EXT;
2606+
else
2607+
{
2608+
scratch.opcode = EEOP_DOMAIN_TESTVAL;
2609+
/* we share instruction union variant with case testval */
2610+
scratch.d.casetest.value = state->innermost_domainval;
2611+
scratch.d.casetest.isnull = state->innermost_domainnull;
2612+
}
26052613
ExprEvalPushStep(state, &scratch);
26062614
break;
26072615
}

‎src/backend/executor/execExprInterp.c‎

Lines changed: 24 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -365,8 +365,7 @@ ExecReadyInterpretedExpr(ExprState *state)
365365
return;
366366
}
367367
else if (step0 == EEOP_CASE_TESTVAL &&
368-
step1 == EEOP_FUNCEXPR_STRICT &&
369-
state->steps[0].d.casetest.value)
368+
step1 == EEOP_FUNCEXPR_STRICT)
370369
{
371370
state->evalfunc_private = ExecJustApplyFuncToCase;
372371
return;
@@ -524,6 +523,7 @@ ExecInterpExpr(ExprState *state, ExprContext *econtext, bool *isnull)
524523
&&CASE_EEOP_PARAM_CALLBACK,
525524
&&CASE_EEOP_PARAM_SET,
526525
&&CASE_EEOP_CASE_TESTVAL,
526+
&&CASE_EEOP_CASE_TESTVAL_EXT,
527527
&&CASE_EEOP_MAKE_READONLY,
528528
&&CASE_EEOP_IOCOERCE,
529529
&&CASE_EEOP_IOCOERCE_SAFE,
@@ -548,6 +548,7 @@ ExecInterpExpr(ExprState *state, ExprContext *econtext, bool *isnull)
548548
&&CASE_EEOP_SBSREF_ASSIGN,
549549
&&CASE_EEOP_SBSREF_FETCH,
550550
&&CASE_EEOP_DOMAIN_TESTVAL,
551+
&&CASE_EEOP_DOMAIN_TESTVAL_EXT,
551552
&&CASE_EEOP_DOMAIN_NOTNULL,
552553
&&CASE_EEOP_DOMAIN_CHECK,
553554
&&CASE_EEOP_HASHDATUM_SET_INITVAL,
@@ -1273,44 +1274,16 @@ ExecInterpExpr(ExprState *state, ExprContext *econtext, bool *isnull)
12731274

12741275
EEO_CASE(EEOP_CASE_TESTVAL)
12751276
{
1276-
/*
1277-
* Normally upper parts of the expression tree have setup the
1278-
* values to be returned here, but some parts of the system
1279-
* currently misuse {caseValue,domainValue}_{datum,isNull} to set
1280-
* run-time data. So if no values have been set-up, use
1281-
* ExprContext's. This isn't pretty, but also not *that* ugly,
1282-
* and this is unlikely to be performance sensitive enough to
1283-
* worry about an extra branch.
1284-
*/
1285-
if (op->d.casetest.value)
1286-
{
1287-
*op->resvalue = *op->d.casetest.value;
1288-
*op->resnull = *op->d.casetest.isnull;
1289-
}
1290-
else
1291-
{
1292-
*op->resvalue = econtext->caseValue_datum;
1293-
*op->resnull = econtext->caseValue_isNull;
1294-
}
1277+
*op->resvalue = *op->d.casetest.value;
1278+
*op->resnull = *op->d.casetest.isnull;
12951279

12961280
EEO_NEXT();
12971281
}
12981282

1299-
EEO_CASE(EEOP_DOMAIN_TESTVAL)
1283+
EEO_CASE(EEOP_CASE_TESTVAL_EXT)
13001284
{
1301-
/*
1302-
* See EEOP_CASE_TESTVAL comment.
1303-
*/
1304-
if (op->d.casetest.value)
1305-
{
1306-
*op->resvalue = *op->d.casetest.value;
1307-
*op->resnull = *op->d.casetest.isnull;
1308-
}
1309-
else
1310-
{
1311-
*op->resvalue = econtext->domainValue_datum;
1312-
*op->resnull = econtext->domainValue_isNull;
1313-
}
1285+
*op->resvalue = econtext->caseValue_datum;
1286+
*op->resnull = econtext->caseValue_isNull;
13141287

13151288
EEO_NEXT();
13161289
}
@@ -1726,6 +1699,22 @@ ExecInterpExpr(ExprState *state, ExprContext *econtext, bool *isnull)
17261699
EEO_NEXT();
17271700
}
17281701

1702+
EEO_CASE(EEOP_DOMAIN_TESTVAL)
1703+
{
1704+
*op->resvalue = *op->d.casetest.value;
1705+
*op->resnull = *op->d.casetest.isnull;
1706+
1707+
EEO_NEXT();
1708+
}
1709+
1710+
EEO_CASE(EEOP_DOMAIN_TESTVAL_EXT)
1711+
{
1712+
*op->resvalue = econtext->domainValue_datum;
1713+
*op->resnull = econtext->domainValue_isNull;
1714+
1715+
EEO_NEXT();
1716+
}
1717+
17291718
EEO_CASE(EEOP_DOMAIN_NOTNULL)
17301719
{
17311720
/* too complex for an inline implementation */

‎src/backend/jit/llvm/llvmjit_expr.c‎

Lines changed: 16 additions & 40 deletions
Original file line numberDiff line numberDiff line change
@@ -1265,41 +1265,30 @@ llvm_compile_expr(ExprState *state)
12651265

12661266
case EEOP_CASE_TESTVAL:
12671267
{
1268-
LLVMBasicBlockRef b_avail,
1269-
b_notavail;
12701268
LLVMValueRef v_casevaluep,
12711269
v_casevalue;
12721270
LLVMValueRef v_casenullp,
12731271
v_casenull;
1274-
LLVMValueRef v_casevaluenull;
1275-
1276-
b_avail = l_bb_before_v(opblocks[opno + 1],
1277-
"op.%d.avail", opno);
1278-
b_notavail = l_bb_before_v(opblocks[opno + 1],
1279-
"op.%d.notavail", opno);
12801272

12811273
v_casevaluep = l_ptr_const(op->d.casetest.value,
12821274
l_ptr(TypeSizeT));
12831275
v_casenullp = l_ptr_const(op->d.casetest.isnull,
12841276
l_ptr(TypeStorageBool));
12851277

1286-
v_casevaluenull =
1287-
LLVMBuildICmp(b, LLVMIntEQ,
1288-
LLVMBuildPtrToInt(b, v_casevaluep,
1289-
TypeSizeT, ""),
1290-
l_sizet_const(0), "");
1291-
LLVMBuildCondBr(b, v_casevaluenull, b_notavail, b_avail);
1292-
1293-
/* if casetest != NULL */
1294-
LLVMPositionBuilderAtEnd(b, b_avail);
12951278
v_casevalue = l_load(b, TypeSizeT, v_casevaluep, "");
12961279
v_casenull = l_load(b, TypeStorageBool, v_casenullp, "");
12971280
LLVMBuildStore(b, v_casevalue, v_resvaluep);
12981281
LLVMBuildStore(b, v_casenull, v_resnullp);
1282+
12991283
LLVMBuildBr(b, opblocks[opno + 1]);
1284+
break;
1285+
}
1286+
1287+
case EEOP_CASE_TESTVAL_EXT:
1288+
{
1289+
LLVMValueRef v_casevalue;
1290+
LLVMValueRef v_casenull;
13001291

1301-
/* if casetest == NULL */
1302-
LLVMPositionBuilderAtEnd(b, b_notavail);
13031292
v_casevalue =
13041293
l_load_struct_gep(b,
13051294
StructExprContext,
@@ -1958,43 +1947,30 @@ llvm_compile_expr(ExprState *state)
19581947

19591948
case EEOP_DOMAIN_TESTVAL:
19601949
{
1961-
LLVMBasicBlockRef b_avail,
1962-
b_notavail;
19631950
LLVMValueRef v_casevaluep,
19641951
v_casevalue;
19651952
LLVMValueRef v_casenullp,
19661953
v_casenull;
1967-
LLVMValueRef v_casevaluenull;
1968-
1969-
b_avail = l_bb_before_v(opblocks[opno + 1],
1970-
"op.%d.avail", opno);
1971-
b_notavail = l_bb_before_v(opblocks[opno + 1],
1972-
"op.%d.notavail", opno);
19731954

19741955
v_casevaluep = l_ptr_const(op->d.casetest.value,
19751956
l_ptr(TypeSizeT));
19761957
v_casenullp = l_ptr_const(op->d.casetest.isnull,
19771958
l_ptr(TypeStorageBool));
19781959

1979-
v_casevaluenull =
1980-
LLVMBuildICmp(b, LLVMIntEQ,
1981-
LLVMBuildPtrToInt(b, v_casevaluep,
1982-
TypeSizeT, ""),
1983-
l_sizet_const(0), "");
1984-
LLVMBuildCondBr(b,
1985-
v_casevaluenull,
1986-
b_notavail, b_avail);
1987-
1988-
/* if casetest != NULL */
1989-
LLVMPositionBuilderAtEnd(b, b_avail);
19901960
v_casevalue = l_load(b, TypeSizeT, v_casevaluep, "");
19911961
v_casenull = l_load(b, TypeStorageBool, v_casenullp, "");
19921962
LLVMBuildStore(b, v_casevalue, v_resvaluep);
19931963
LLVMBuildStore(b, v_casenull, v_resnullp);
1964+
19941965
LLVMBuildBr(b, opblocks[opno + 1]);
1966+
break;
1967+
}
1968+
1969+
case EEOP_DOMAIN_TESTVAL_EXT:
1970+
{
1971+
LLVMValueRef v_casevalue;
1972+
LLVMValueRef v_casenull;
19951973

1996-
/* if casetest == NULL */
1997-
LLVMPositionBuilderAtEnd(b, b_notavail);
19981974
v_casevalue =
19991975
l_load_struct_gep(b,
20001976
StructExprContext,

‎src/include/executor/execExpr.h‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -173,6 +173,7 @@ typedef enum ExprEvalOp
173173

174174
/* return CaseTestExpr value */
175175
EEOP_CASE_TESTVAL,
176+
EEOP_CASE_TESTVAL_EXT,
176177

177178
/* apply MakeExpandedObjectReadOnly() to target value */
178179
EEOP_MAKE_READONLY,
@@ -237,6 +238,7 @@ typedef enum ExprEvalOp
237238

238239
/* evaluate value for CoerceToDomainValue */
239240
EEOP_DOMAIN_TESTVAL,
241+
EEOP_DOMAIN_TESTVAL_EXT,
240242

241243
/* evaluate a domain's NOT NULL constraint */
242244
EEOP_DOMAIN_NOTNULL,

0 commit comments

Comments
 (0)