Skip to content

Commit 11c8721

Browse files
amitlanamitlan
committed
SQL/JSON: Fix some oversights in commit b6e1157
The decision in b6e1157 to ignore raw_expr when evaluating a JsonValueExpr was incorrect. While its value is not ultimately used (since formatted_expr's value is), failing to initialize it can lead to problems, for instance, when the expression tree in raw_expr contains Aggref nodes, which must be initialized to ensure the parent Agg node works correctly. Also, optimize eval_const_expressions_mutator()'s handling of JsonValueExpr a bit. Currently, when formatted_expr cannot be folded into a constant, we end up processing it twice -- once directly in eval_const_expressions_mutator() and again recursively via ece_generic_processing(). This recursive processing is required to handle raw_expr. To avoid the redundant processing of formatted_expr, we now process raw_expr directly in eval_const_expressions_mutator(). Finally, update the comment of JsonValueExpr to describe the roles of raw_expr and formatted_expr more clearly. Bug: #18657 Reported-by: Alexander Lakhin <exclusion@gmail.com> Diagnosed-by: Fabio R. Sluzala <fabio3rs@gmail.com> Diagnosed-by: Tender Wang <tndrwang@gmail.com> Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us> Discussion: https://postgr.es/m/18657-1b90ccce2b16bdb8@postgresql.org Backpatch-through: 16
1 parent 52475b4 commit 11c8721

File tree

5 files changed

+92
-11
lines changed

5 files changed

+92
-11
lines changed

‎src/backend/executor/execExpr.c‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2307,6 +2307,8 @@ ExecInitExprRec(Expr *node, ExprState *state,
23072307
{
23082308
JsonValueExpr *jve = (JsonValueExpr *) node;
23092309

2310+
Assert(jve->raw_expr != NULL);
2311+
ExecInitExprRec(jve->raw_expr, state, resv, resnull);
23102312
Assert(jve->formatted_expr != NULL);
23112313
ExecInitExprRec(jve->formatted_expr, state, resv, resnull);
23122314
break;

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

Lines changed: 18 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -2915,13 +2915,25 @@ eval_const_expressions_mutator(Node *node,
29152915
case T_JsonValueExpr:
29162916
{
29172917
JsonValueExpr *jve = (JsonValueExpr *) node;
2918-
Node *formatted;
2918+
Node *raw_expr = (Node *) jve->raw_expr;
2919+
Node *formatted_expr = (Node *) jve->formatted_expr;
29192920

2920-
formatted = eval_const_expressions_mutator((Node *) jve->formatted_expr,
2921-
context);
2922-
if (formatted && IsA(formatted, Const))
2923-
return formatted;
2924-
break;
2921+
/*
2922+
* If we can fold formatted_expr to a constant, we can elide
2923+
* the JsonValueExpr altogether. Otherwise we must process
2924+
* raw_expr too. But JsonFormat is a flat node and requires
2925+
* no simplification, only copying.
2926+
*/
2927+
formatted_expr = eval_const_expressions_mutator(formatted_expr,
2928+
context);
2929+
if (formatted_expr && IsA(formatted_expr, Const))
2930+
return formatted_expr;
2931+
2932+
raw_expr = eval_const_expressions_mutator(raw_expr, context);
2933+
2934+
return (Node *) makeJsonValueExpr((Expr *) raw_expr,
2935+
(Expr *) formatted_expr,
2936+
copyObject(jve->format));
29252937
}
29262938

29272939
case T_SubPlan:

‎src/include/nodes/primnodes.h‎

Lines changed: 9 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1669,15 +1669,19 @@ typedef struct JsonReturning
16691669
* JsonValueExpr -
16701670
* representation of JSON value expression (expr [FORMAT JsonFormat])
16711671
*
1672-
* The actual value is obtained by evaluating formatted_expr. raw_expr is
1673-
* only there for displaying the original user-written expression and is not
1674-
* evaluated by ExecInterpExpr() and eval_const_expressions_mutator().
1672+
* raw_expr is the user-specified value, while formatted_expr is the value
1673+
* obtained by coercing raw_expr to the type required by either the FORMAT
1674+
* clause or an enclosing node's RETURNING clause.
1675+
*
1676+
* When deparsing a JsonValueExpr, get_rule_expr() prints raw_expr. However,
1677+
* during the evaluation of a JsonValueExpr, the value of formatted_expr
1678+
* takes precedence over that of raw_expr.
16751679
*/
16761680
typedef struct JsonValueExpr
16771681
{
16781682
NodeTag type;
1679-
Expr *raw_expr; /* raw expression */
1680-
Expr *formatted_expr; /* formatted expression */
1683+
Expr *raw_expr; /* user-specified expression */
1684+
Expr *formatted_expr; /* coerced formatted expression */
16811685
JsonFormat *format; /* FORMAT clause, if specified */
16821686
} JsonValueExpr;
16831687

‎src/test/regress/expected/sqljson.out‎

Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1300,3 +1300,52 @@ SELECT JSON_SERIALIZE('123' RETURNING sqljson_char2);
13001300
ERROR: value too long for type character(2)
13011301
SELECT JSON_SERIALIZE('12' RETURNING sqljson_char2);
13021302
ERROR: value for domain sqljson_char2 violates check constraint "sqljson_char2_check"
1303+
-- Bug #18657: JsonValueExpr.raw_expr was not initialized in ExecInitExprRec()
1304+
-- causing the Aggrefs contained in it to also not be initialized, which led
1305+
-- to a crash in ExecBuildAggTrans() as mentioned in the bug report:
1306+
-- https://postgr.es/m/18657-1b90ccce2b16bdb8@postgresql.org
1307+
CREATE FUNCTION volatile_one() RETURNS int AS $$ BEGIN RETURN 1; END; $$ LANGUAGE plpgsql VOLATILE;
1308+
CREATE FUNCTION stable_one() RETURNS int AS $$ BEGIN RETURN 1; END; $$ LANGUAGE plpgsql STABLE;
1309+
EXPLAIN (VERBOSE, COSTS OFF) SELECT JSON_OBJECT('a': JSON_OBJECTAGG('b': volatile_one() RETURNING text) FORMAT JSON);
1310+
QUERY PLAN
1311+
-------------------------------------------------------------------------------------------------------------
1312+
Aggregate
1313+
Output: JSON_OBJECT('a' : JSON_OBJECTAGG('b' : volatile_one() RETURNING text) FORMAT JSON RETURNING json)
1314+
-> Result
1315+
(3 rows)
1316+
1317+
SELECT JSON_OBJECT('a': JSON_OBJECTAGG('b': volatile_one() RETURNING text) FORMAT JSON);
1318+
json_object
1319+
---------------------
1320+
{"a" : { "b" : 1 }}
1321+
(1 row)
1322+
1323+
EXPLAIN (VERBOSE, COSTS OFF) SELECT JSON_OBJECT('a': JSON_OBJECTAGG('b': stable_one() RETURNING text) FORMAT JSON);
1324+
QUERY PLAN
1325+
-----------------------------------------------------------------------------------------------------------
1326+
Aggregate
1327+
Output: JSON_OBJECT('a' : JSON_OBJECTAGG('b' : stable_one() RETURNING text) FORMAT JSON RETURNING json)
1328+
-> Result
1329+
(3 rows)
1330+
1331+
SELECT JSON_OBJECT('a': JSON_OBJECTAGG('b': stable_one() RETURNING text) FORMAT JSON);
1332+
json_object
1333+
---------------------
1334+
{"a" : { "b" : 1 }}
1335+
(1 row)
1336+
1337+
EXPLAIN (VERBOSE, COSTS OFF) SELECT JSON_OBJECT('a': JSON_OBJECTAGG('b': 1 RETURNING text) FORMAT JSON);
1338+
QUERY PLAN
1339+
------------------------------------------------------------------------------------------------
1340+
Aggregate
1341+
Output: JSON_OBJECT('a' : JSON_OBJECTAGG('b' : 1 RETURNING text) FORMAT JSON RETURNING json)
1342+
-> Result
1343+
(3 rows)
1344+
1345+
SELECT JSON_OBJECT('a': JSON_OBJECTAGG('b': 1 RETURNING text) FORMAT JSON);
1346+
json_object
1347+
---------------------
1348+
{"a" : { "b" : 1 }}
1349+
(1 row)
1350+
1351+
DROP FUNCTION volatile_one, stable_one;

‎src/test/regress/sql/sqljson.sql‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -480,3 +480,17 @@ SELECT JSON_OBJECTAGG(i: ('111' || i)::bytea FORMAT JSON WITH UNIQUE RETURNING v
480480
CREATE DOMAIN sqljson_char2 AS char(2) CHECK (VALUE NOT IN ('12'));
481481
SELECT JSON_SERIALIZE('123' RETURNING sqljson_char2);
482482
SELECT JSON_SERIALIZE('12' RETURNING sqljson_char2);
483+
484+
-- Bug #18657: JsonValueExpr.raw_expr was not initialized in ExecInitExprRec()
485+
-- causing the Aggrefs contained in it to also not be initialized, which led
486+
-- to a crash in ExecBuildAggTrans() as mentioned in the bug report:
487+
-- https://postgr.es/m/18657-1b90ccce2b16bdb8@postgresql.org
488+
CREATE FUNCTION volatile_one() RETURNS int AS $$ BEGIN RETURN 1; END; $$ LANGUAGE plpgsql VOLATILE;
489+
CREATE FUNCTION stable_one() RETURNS int AS $$ BEGIN RETURN 1; END; $$ LANGUAGE plpgsql STABLE;
490+
EXPLAIN (VERBOSE, COSTS OFF) SELECT JSON_OBJECT('a': JSON_OBJECTAGG('b': volatile_one() RETURNING text) FORMAT JSON);
491+
SELECT JSON_OBJECT('a': JSON_OBJECTAGG('b': volatile_one() RETURNING text) FORMAT JSON);
492+
EXPLAIN (VERBOSE, COSTS OFF) SELECT JSON_OBJECT('a': JSON_OBJECTAGG('b': stable_one() RETURNING text) FORMAT JSON);
493+
SELECT JSON_OBJECT('a': JSON_OBJECTAGG('b': stable_one() RETURNING text) FORMAT JSON);
494+
EXPLAIN (VERBOSE, COSTS OFF) SELECT JSON_OBJECT('a': JSON_OBJECTAGG('b': 1 RETURNING text) FORMAT JSON);
495+
SELECT JSON_OBJECT('a': JSON_OBJECTAGG('b': 1 RETURNING text) FORMAT JSON);
496+
DROP FUNCTION volatile_one, stable_one;

0 commit comments

Comments
 (0)