Skip to content

Commit d8c3106

Browse files
michaelpqmichaelpq
committed
Add back SQLValueFunction for SQL keywords
This is equivalent to a revert of f193883 and fb32748, with the addition that the declaration of the SQLValueFunction node needs to gain a couple of node_attr for query jumbling. The performance impact of removing the function call inlining is proving to be too huge for some workloads where these are used. A worst-case test case of involving only simple SELECT queries with a SQL keyword is proving to lead to a reduction of 10% in TPS via pgbench and prepared queries on a high-end machine. None of the tests I ran back for this set of changes saw such a huge gap, but Alexander Lakhin and Andres Freund have found that this can be noticeable. Keeping the older performance would mean to do more inlining in the executor when using COERCE_SQL_SYNTAX for a function expression, similarly to what SQLValueFunction does. This requires more redesign work and there is little time until 16beta1 is released, so for now reverting the change is the best way forward, bringing back the previous performance. Bump catalog version. Reported-by: Alexander Lakhin Discussion: https://postgr.es/m/b32bed1b-0746-9b20-1472-4bdc9ca66d52@gmail.com
1 parent 1d369c9 commit d8c3106

File tree

23 files changed

+473
-270
lines changed

23 files changed

+473
-270
lines changed

‎src/backend/catalog/system_functions.sql

Lines changed: 0 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -601,32 +601,6 @@ LANGUAGE internal
601601
STRICT IMMUTABLE PARALLEL SAFE
602602
AS 'unicode_is_normalized';
603603

604-
-- Functions with SQL-mandated special syntax and some defaults.
605-
CREATE OR REPLACE FUNCTION
606-
"current_time"(int4 DEFAULT NULL)
607-
RETURNS timetz
608-
LANGUAGE internal
609-
STABLE PARALLEL SAFE
610-
AS 'current_time';
611-
CREATE OR REPLACE FUNCTION
612-
"current_timestamp"(int4 DEFAULT NULL)
613-
RETURNS timestamptz
614-
LANGUAGE internal
615-
STABLE PARALLEL SAFE
616-
AS 'current_timestamp';
617-
CREATE OR REPLACE FUNCTION
618-
"localtime"(int4 DEFAULT NULL)
619-
RETURNS time
620-
LANGUAGE internal
621-
STABLE PARALLEL SAFE
622-
AS 'sql_localtime';
623-
CREATE OR REPLACE FUNCTION
624-
"localtimestamp"(int4 DEFAULT NULL)
625-
RETURNS timestamp
626-
LANGUAGE internal
627-
STABLE PARALLEL SAFE
628-
AS 'sql_localtimestamp';
629-
630604
--
631605
-- The default permissions for functions mean that anyone can execute them.
632606
-- A number of functions shouldn't be executable by just anyone, but rather

‎src/backend/executor/execExpr.c

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2213,6 +2213,17 @@ ExecInitExprRec(Expr *node, ExprState *state,
22132213
break;
22142214
}
22152215

2216+
case T_SQLValueFunction:
2217+
{
2218+
SQLValueFunction *svf = (SQLValueFunction *) node;
2219+
2220+
scratch.opcode = EEOP_SQLVALUEFUNCTION;
2221+
scratch.d.sqlvaluefunction.svf = svf;
2222+
2223+
ExprEvalPushStep(state, &scratch);
2224+
break;
2225+
}
2226+
22162227
case T_XmlExpr:
22172228
{
22182229
XmlExpr *xexpr = (XmlExpr *) node;

‎src/backend/executor/execExprInterp.c

Lines changed: 73 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -455,6 +455,7 @@ ExecInterpExpr(ExprState *state, ExprContext *econtext, bool *isnull)
455455
&&CASE_EEOP_DISTINCT,
456456
&&CASE_EEOP_NOT_DISTINCT,
457457
&&CASE_EEOP_NULLIF,
458+
&&CASE_EEOP_SQLVALUEFUNCTION,
458459
&&CASE_EEOP_CURRENTOFEXPR,
459460
&&CASE_EEOP_NEXTVALUEEXPR,
460461
&&CASE_EEOP_ARRAYEXPR,
@@ -1305,6 +1306,17 @@ ExecInterpExpr(ExprState *state, ExprContext *econtext, bool *isnull)
13051306
EEO_NEXT();
13061307
}
13071308

1309+
EEO_CASE(EEOP_SQLVALUEFUNCTION)
1310+
{
1311+
/*
1312+
* Doesn't seem worthwhile to have an inline implementation
1313+
* efficiency-wise.
1314+
*/
1315+
ExecEvalSQLValueFunction(state, op);
1316+
1317+
EEO_NEXT();
1318+
}
1319+
13081320
EEO_CASE(EEOP_CURRENTOFEXPR)
13091321
{
13101322
/* error invocation uses space, and shouldn't ever occur */
@@ -2497,6 +2509,67 @@ ExecEvalParamExtern(ExprState *state, ExprEvalStep *op, ExprContext *econtext)
24972509
errmsg("no value found for parameter %d", paramId)));
24982510
}
24992511

2512+
/*
2513+
* Evaluate a SQLValueFunction expression.
2514+
*/
2515+
void
2516+
ExecEvalSQLValueFunction(ExprState *state, ExprEvalStep *op)
2517+
{
2518+
LOCAL_FCINFO(fcinfo, 0);
2519+
SQLValueFunction *svf = op->d.sqlvaluefunction.svf;
2520+
2521+
*op->resnull = false;
2522+
2523+
/*
2524+
* Note: current_schema() can return NULL. current_user() etc currently
2525+
* cannot, but might as well code those cases the same way for safety.
2526+
*/
2527+
switch (svf->op)
2528+
{
2529+
case SVFOP_CURRENT_DATE:
2530+
*op->resvalue = DateADTGetDatum(GetSQLCurrentDate());
2531+
break;
2532+
case SVFOP_CURRENT_TIME:
2533+
case SVFOP_CURRENT_TIME_N:
2534+
*op->resvalue = TimeTzADTPGetDatum(GetSQLCurrentTime(svf->typmod));
2535+
break;
2536+
case SVFOP_CURRENT_TIMESTAMP:
2537+
case SVFOP_CURRENT_TIMESTAMP_N:
2538+
*op->resvalue = TimestampTzGetDatum(GetSQLCurrentTimestamp(svf->typmod));
2539+
break;
2540+
case SVFOP_LOCALTIME:
2541+
case SVFOP_LOCALTIME_N:
2542+
*op->resvalue = TimeADTGetDatum(GetSQLLocalTime(svf->typmod));
2543+
break;
2544+
case SVFOP_LOCALTIMESTAMP:
2545+
case SVFOP_LOCALTIMESTAMP_N:
2546+
*op->resvalue = TimestampGetDatum(GetSQLLocalTimestamp(svf->typmod));
2547+
break;
2548+
case SVFOP_CURRENT_ROLE:
2549+
case SVFOP_CURRENT_USER:
2550+
case SVFOP_USER:
2551+
InitFunctionCallInfoData(*fcinfo, NULL, 0, InvalidOid, NULL, NULL);
2552+
*op->resvalue = current_user(fcinfo);
2553+
*op->resnull = fcinfo->isnull;
2554+
break;
2555+
case SVFOP_SESSION_USER:
2556+
InitFunctionCallInfoData(*fcinfo, NULL, 0, InvalidOid, NULL, NULL);
2557+
*op->resvalue = session_user(fcinfo);
2558+
*op->resnull = fcinfo->isnull;
2559+
break;
2560+
case SVFOP_CURRENT_CATALOG:
2561+
InitFunctionCallInfoData(*fcinfo, NULL, 0, InvalidOid, NULL, NULL);
2562+
*op->resvalue = current_database(fcinfo);
2563+
*op->resnull = fcinfo->isnull;
2564+
break;
2565+
case SVFOP_CURRENT_SCHEMA:
2566+
InitFunctionCallInfoData(*fcinfo, NULL, 0, InvalidOid, NULL, NULL);
2567+
*op->resvalue = current_schema(fcinfo);
2568+
*op->resnull = fcinfo->isnull;
2569+
break;
2570+
}
2571+
}
2572+
25002573
/*
25012574
* Raise error if a CURRENT OF expression is evaluated.
25022575
*

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

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1549,6 +1549,12 @@ llvm_compile_expr(ExprState *state)
15491549
break;
15501550
}
15511551

1552+
case EEOP_SQLVALUEFUNCTION:
1553+
build_EvalXFunc(b, mod, "ExecEvalSQLValueFunction",
1554+
v_state, op);
1555+
LLVMBuildBr(b, opblocks[opno + 1]);
1556+
break;
1557+
15521558
case EEOP_CURRENTOFEXPR:
15531559
build_EvalXFunc(b, mod, "ExecEvalCurrentOfExpr",
15541560
v_state, op);

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -126,6 +126,7 @@ void *referenced_functions[] =
126126
ExecEvalRow,
127127
ExecEvalRowNotNull,
128128
ExecEvalRowNull,
129+
ExecEvalSQLValueFunction,
129130
ExecEvalScalarArrayOp,
130131
ExecEvalHashedScalarArrayOp,
131132
ExecEvalSubPlan,

‎src/backend/nodes/nodeFuncs.c

Lines changed: 28 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -210,6 +210,9 @@ exprType(const Node *expr)
210210
case T_MinMaxExpr:
211211
type = ((const MinMaxExpr *) expr)->minmaxtype;
212212
break;
213+
case T_SQLValueFunction:
214+
type = ((const SQLValueFunction *) expr)->type;
215+
break;
213216
case T_XmlExpr:
214217
if (((const XmlExpr *) expr)->op == IS_DOCUMENT)
215218
type = BOOLOID;
@@ -486,6 +489,8 @@ exprTypmod(const Node *expr)
486489
return typmod;
487490
}
488491
break;
492+
case T_SQLValueFunction:
493+
return ((const SQLValueFunction *) expr)->typmod;
489494
case T_JsonValueExpr:
490495
return exprTypmod((Node *) ((const JsonValueExpr *) expr)->formatted_expr);
491496
case T_JsonConstructorExpr:
@@ -930,6 +935,13 @@ exprCollation(const Node *expr)
930935
case T_MinMaxExpr:
931936
coll = ((const MinMaxExpr *) expr)->minmaxcollid;
932937
break;
938+
case T_SQLValueFunction:
939+
/* Returns either NAME or a non-collatable type */
940+
if (((const SQLValueFunction *) expr)->type == NAMEOID)
941+
coll = C_COLLATION_OID;
942+
else
943+
coll = InvalidOid;
944+
break;
933945
case T_XmlExpr:
934946

935947
/*
@@ -1167,6 +1179,11 @@ exprSetCollation(Node *expr, Oid collation)
11671179
case T_MinMaxExpr:
11681180
((MinMaxExpr *) expr)->minmaxcollid = collation;
11691181
break;
1182+
case T_SQLValueFunction:
1183+
Assert((((SQLValueFunction *) expr)->type == NAMEOID) ?
1184+
(collation == C_COLLATION_OID) :
1185+
(collation == InvalidOid));
1186+
break;
11701187
case T_XmlExpr:
11711188
Assert((((XmlExpr *) expr)->op == IS_XMLSERIALIZE) ?
11721189
(collation == DEFAULT_COLLATION_OID) :
@@ -1468,6 +1485,10 @@ exprLocation(const Node *expr)
14681485
/* GREATEST/LEAST keyword should always be the first thing */
14691486
loc = ((const MinMaxExpr *) expr)->location;
14701487
break;
1488+
case T_SQLValueFunction:
1489+
/* function keyword should always be the first thing */
1490+
loc = ((const SQLValueFunction *) expr)->location;
1491+
break;
14711492
case T_XmlExpr:
14721493
{
14731494
const XmlExpr *xexpr = (const XmlExpr *) expr;
@@ -1789,10 +1810,10 @@ set_sa_opfuncid(ScalarArrayOpExpr *opexpr)
17891810
* for themselves, in case additional checks should be made, or because they
17901811
* have special rules about which parts of the tree need to be visited.
17911812
*
1792-
* Note: we ignore MinMaxExpr, XmlExpr, CoerceToDomain, and NextValueExpr
1793-
* nodes, because they do not contain SQL function OIDs. However, they can
1794-
* invoke SQL-visible functions, so callers should take thought about how
1795-
* to treat them.
1813+
* Note: we ignore MinMaxExpr, SQLValueFunction, XmlExpr, CoerceToDomain,
1814+
* and NextValueExpr nodes, because they do not contain SQL function OIDs.
1815+
* However, they can invoke SQL-visible functions, so callers should take
1816+
* thought about how to treat them.
17961817
*/
17971818
bool
17981819
check_functions_in_node(Node *node, check_function_callback checker,
@@ -2008,6 +2029,7 @@ expression_tree_walker_impl(Node *node,
20082029
case T_Const:
20092030
case T_Param:
20102031
case T_CaseTestExpr:
2032+
case T_SQLValueFunction:
20112033
case T_CoerceToDomainValue:
20122034
case T_SetToDefault:
20132035
case T_CurrentOfExpr:
@@ -2836,6 +2858,7 @@ expression_tree_mutator_impl(Node *node,
28362858
break;
28372859
case T_Param:
28382860
case T_CaseTestExpr:
2861+
case T_SQLValueFunction:
28392862
case T_JsonFormat:
28402863
case T_CoerceToDomainValue:
28412864
case T_SetToDefault:
@@ -3797,6 +3820,7 @@ raw_expression_tree_walker_impl(Node *node,
37973820
case T_JsonFormat:
37983821
case T_SetToDefault:
37993822
case T_CurrentOfExpr:
3823+
case T_SQLValueFunction:
38003824
case T_Integer:
38013825
case T_Float:
38023826
case T_Boolean:

‎src/backend/optimizer/path/costsize.c

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4606,6 +4606,7 @@ cost_qual_eval_walker(Node *node, cost_qual_eval_context *context)
46064606
}
46074607
}
46084608
else if (IsA(node, MinMaxExpr) ||
4609+
IsA(node, SQLValueFunction) ||
46094610
IsA(node, XmlExpr) ||
46104611
IsA(node, CoerceToDomain) ||
46114612
IsA(node, NextValueExpr))

‎src/backend/optimizer/util/clauses.c

Lines changed: 32 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -412,6 +412,12 @@ contain_mutable_functions_walker(Node *node, void *context)
412412
/* Check all subnodes */
413413
}
414414

415+
if (IsA(node, SQLValueFunction))
416+
{
417+
/* all variants of SQLValueFunction are stable */
418+
return true;
419+
}
420+
415421
if (IsA(node, NextValueExpr))
416422
{
417423
/* NextValueExpr is volatile */
@@ -560,8 +566,8 @@ contain_volatile_functions_walker(Node *node, void *context)
560566

561567
/*
562568
* See notes in contain_mutable_functions_walker about why we treat
563-
* MinMaxExpr, XmlExpr, and CoerceToDomain as immutable. Hence, none of
564-
* them are of interest here.
569+
* MinMaxExpr, XmlExpr, and CoerceToDomain as immutable, while
570+
* SQLValueFunction is stable. Hence, none of them are of interest here.
565571
*/
566572

567573
/* Recurse to check arguments */
@@ -606,9 +612,10 @@ contain_volatile_functions_not_nextval_walker(Node *node, void *context)
606612

607613
/*
608614
* See notes in contain_mutable_functions_walker about why we treat
609-
* MinMaxExpr, XmlExpr, and CoerceToDomain as immutable. Hence, none of
610-
* them are of interest here. Also, since we're intentionally ignoring
611-
* nextval(), presumably we should ignore NextValueExpr.
615+
* MinMaxExpr, XmlExpr, and CoerceToDomain as immutable, while
616+
* SQLValueFunction is stable. Hence, none of them are of interest here.
617+
* Also, since we're intentionally ignoring nextval(), presumably we
618+
* should ignore NextValueExpr.
612619
*/
613620

614621
/* Recurse to check arguments */
@@ -754,8 +761,8 @@ max_parallel_hazard_walker(Node *node, max_parallel_hazard_context *context)
754761
* (Note: in principle that's wrong because a domain constraint could
755762
* contain a parallel-unsafe function; but useful constraints probably
756763
* never would have such, and assuming they do would cripple use of
757-
* parallel query in the presence of domain types.) NextValueExpr is
758-
* parallel-unsafe.
764+
* parallel query in the presence of domain types.) SQLValueFunction
765+
* should be safe in all cases. NextValueExpr is parallel-unsafe.
759766
*/
760767
if (IsA(node, CoerceToDomain))
761768
{
@@ -1202,6 +1209,7 @@ contain_leaked_vars_walker(Node *node, void *context)
12021209
case T_CaseExpr:
12031210
case T_CaseTestExpr:
12041211
case T_RowExpr:
1212+
case T_SQLValueFunction:
12051213
case T_NullTest:
12061214
case T_BooleanTest:
12071215
case T_NextValueExpr:
@@ -3243,6 +3251,23 @@ eval_const_expressions_mutator(Node *node,
32433251
newcoalesce->location = coalesceexpr->location;
32443252
return (Node *) newcoalesce;
32453253
}
3254+
case T_SQLValueFunction:
3255+
{
3256+
/*
3257+
* All variants of SQLValueFunction are stable, so if we are
3258+
* estimating the expression's value, we should evaluate the
3259+
* current function value. Otherwise just copy.
3260+
*/
3261+
SQLValueFunction *svf = (SQLValueFunction *) node;
3262+
3263+
if (context->estimate)
3264+
return (Node *) evaluate_expr((Expr *) svf,
3265+
svf->type,
3266+
svf->typmod,
3267+
InvalidOid);
3268+
else
3269+
return copyObject((Node *) svf);
3270+
}
32463271
case T_FieldSelect:
32473272
{
32483273
/*

0 commit comments

Comments
 (0)