From 11c87216d134a606938531e52edab6189bce6c2d Mon Sep 17 00:00:00 2001 From: Amit Langote Date: Sun, 20 Oct 2024 12:20:55 +0900 Subject: [PATCH] SQL/JSON: Fix some oversights in commit b6e1157e7 The decision in b6e1157e7 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 Diagnosed-by: Fabio R. Sluzala Diagnosed-by: Tender Wang Reviewed-by: Tom Lane Discussion: https://postgr.es/m/18657-1b90ccce2b16bdb8@postgresql.org Backpatch-through: 16 --- src/backend/executor/execExpr.c | 2 ++ src/backend/optimizer/util/clauses.c | 24 +++++++++---- src/include/nodes/primnodes.h | 14 +++++--- src/test/regress/expected/sqljson.out | 49 +++++++++++++++++++++++++++ src/test/regress/sql/sqljson.sql | 14 ++++++++ 5 files changed, 92 insertions(+), 11 deletions(-) diff --git a/src/backend/executor/execExpr.c b/src/backend/executor/execExpr.c index a343d0bc6a2..45954d979f6 100644 --- a/src/backend/executor/execExpr.c +++ b/src/backend/executor/execExpr.c @@ -2307,6 +2307,8 @@ ExecInitExprRec(Expr *node, ExprState *state, { JsonValueExpr *jve = (JsonValueExpr *) node; + Assert(jve->raw_expr != NULL); + ExecInitExprRec(jve->raw_expr, state, resv, resnull); Assert(jve->formatted_expr != NULL); ExecInitExprRec(jve->formatted_expr, state, resv, resnull); break; diff --git a/src/backend/optimizer/util/clauses.c b/src/backend/optimizer/util/clauses.c index b4e085e9d4b..8e39795e245 100644 --- a/src/backend/optimizer/util/clauses.c +++ b/src/backend/optimizer/util/clauses.c @@ -2915,13 +2915,25 @@ eval_const_expressions_mutator(Node *node, case T_JsonValueExpr: { JsonValueExpr *jve = (JsonValueExpr *) node; - Node *formatted; + Node *raw_expr = (Node *) jve->raw_expr; + Node *formatted_expr = (Node *) jve->formatted_expr; - formatted = eval_const_expressions_mutator((Node *) jve->formatted_expr, - context); - if (formatted && IsA(formatted, Const)) - return formatted; - break; + /* + * If we can fold formatted_expr to a constant, we can elide + * the JsonValueExpr altogether. Otherwise we must process + * raw_expr too. But JsonFormat is a flat node and requires + * no simplification, only copying. + */ + formatted_expr = eval_const_expressions_mutator(formatted_expr, + context); + if (formatted_expr && IsA(formatted_expr, Const)) + return formatted_expr; + + raw_expr = eval_const_expressions_mutator(raw_expr, context); + + return (Node *) makeJsonValueExpr((Expr *) raw_expr, + (Expr *) formatted_expr, + copyObject(jve->format)); } case T_SubPlan: diff --git a/src/include/nodes/primnodes.h b/src/include/nodes/primnodes.h index 5b7b87dea54..b0ef1952e8f 100644 --- a/src/include/nodes/primnodes.h +++ b/src/include/nodes/primnodes.h @@ -1669,15 +1669,19 @@ typedef struct JsonReturning * JsonValueExpr - * representation of JSON value expression (expr [FORMAT JsonFormat]) * - * The actual value is obtained by evaluating formatted_expr. raw_expr is - * only there for displaying the original user-written expression and is not - * evaluated by ExecInterpExpr() and eval_const_expressions_mutator(). + * raw_expr is the user-specified value, while formatted_expr is the value + * obtained by coercing raw_expr to the type required by either the FORMAT + * clause or an enclosing node's RETURNING clause. + * + * When deparsing a JsonValueExpr, get_rule_expr() prints raw_expr. However, + * during the evaluation of a JsonValueExpr, the value of formatted_expr + * takes precedence over that of raw_expr. */ typedef struct JsonValueExpr { NodeTag type; - Expr *raw_expr; /* raw expression */ - Expr *formatted_expr; /* formatted expression */ + Expr *raw_expr; /* user-specified expression */ + Expr *formatted_expr; /* coerced formatted expression */ JsonFormat *format; /* FORMAT clause, if specified */ } JsonValueExpr; diff --git a/src/test/regress/expected/sqljson.out b/src/test/regress/expected/sqljson.out index 9adfbe15f65..70721c9a5ad 100644 --- a/src/test/regress/expected/sqljson.out +++ b/src/test/regress/expected/sqljson.out @@ -1300,3 +1300,52 @@ SELECT JSON_SERIALIZE('123' RETURNING sqljson_char2); ERROR: value too long for type character(2) SELECT JSON_SERIALIZE('12' RETURNING sqljson_char2); ERROR: value for domain sqljson_char2 violates check constraint "sqljson_char2_check" +-- Bug #18657: JsonValueExpr.raw_expr was not initialized in ExecInitExprRec() +-- causing the Aggrefs contained in it to also not be initialized, which led +-- to a crash in ExecBuildAggTrans() as mentioned in the bug report: +-- https://postgr.es/m/18657-1b90ccce2b16bdb8@postgresql.org +CREATE FUNCTION volatile_one() RETURNS int AS $$ BEGIN RETURN 1; END; $$ LANGUAGE plpgsql VOLATILE; +CREATE FUNCTION stable_one() RETURNS int AS $$ BEGIN RETURN 1; END; $$ LANGUAGE plpgsql STABLE; +EXPLAIN (VERBOSE, COSTS OFF) SELECT JSON_OBJECT('a': JSON_OBJECTAGG('b': volatile_one() RETURNING text) FORMAT JSON); + QUERY PLAN +------------------------------------------------------------------------------------------------------------- + Aggregate + Output: JSON_OBJECT('a' : JSON_OBJECTAGG('b' : volatile_one() RETURNING text) FORMAT JSON RETURNING json) + -> Result +(3 rows) + +SELECT JSON_OBJECT('a': JSON_OBJECTAGG('b': volatile_one() RETURNING text) FORMAT JSON); + json_object +--------------------- + {"a" : { "b" : 1 }} +(1 row) + +EXPLAIN (VERBOSE, COSTS OFF) SELECT JSON_OBJECT('a': JSON_OBJECTAGG('b': stable_one() RETURNING text) FORMAT JSON); + QUERY PLAN +----------------------------------------------------------------------------------------------------------- + Aggregate + Output: JSON_OBJECT('a' : JSON_OBJECTAGG('b' : stable_one() RETURNING text) FORMAT JSON RETURNING json) + -> Result +(3 rows) + +SELECT JSON_OBJECT('a': JSON_OBJECTAGG('b': stable_one() RETURNING text) FORMAT JSON); + json_object +--------------------- + {"a" : { "b" : 1 }} +(1 row) + +EXPLAIN (VERBOSE, COSTS OFF) SELECT JSON_OBJECT('a': JSON_OBJECTAGG('b': 1 RETURNING text) FORMAT JSON); + QUERY PLAN +------------------------------------------------------------------------------------------------ + Aggregate + Output: JSON_OBJECT('a' : JSON_OBJECTAGG('b' : 1 RETURNING text) FORMAT JSON RETURNING json) + -> Result +(3 rows) + +SELECT JSON_OBJECT('a': JSON_OBJECTAGG('b': 1 RETURNING text) FORMAT JSON); + json_object +--------------------- + {"a" : { "b" : 1 }} +(1 row) + +DROP FUNCTION volatile_one, stable_one; diff --git a/src/test/regress/sql/sqljson.sql b/src/test/regress/sql/sqljson.sql index 42dbec26d6f..9b5d82602d0 100644 --- a/src/test/regress/sql/sqljson.sql +++ b/src/test/regress/sql/sqljson.sql @@ -480,3 +480,17 @@ SELECT JSON_OBJECTAGG(i: ('111' || i)::bytea FORMAT JSON WITH UNIQUE RETURNING v CREATE DOMAIN sqljson_char2 AS char(2) CHECK (VALUE NOT IN ('12')); SELECT JSON_SERIALIZE('123' RETURNING sqljson_char2); SELECT JSON_SERIALIZE('12' RETURNING sqljson_char2); + +-- Bug #18657: JsonValueExpr.raw_expr was not initialized in ExecInitExprRec() +-- causing the Aggrefs contained in it to also not be initialized, which led +-- to a crash in ExecBuildAggTrans() as mentioned in the bug report: +-- https://postgr.es/m/18657-1b90ccce2b16bdb8@postgresql.org +CREATE FUNCTION volatile_one() RETURNS int AS $$ BEGIN RETURN 1; END; $$ LANGUAGE plpgsql VOLATILE; +CREATE FUNCTION stable_one() RETURNS int AS $$ BEGIN RETURN 1; END; $$ LANGUAGE plpgsql STABLE; +EXPLAIN (VERBOSE, COSTS OFF) SELECT JSON_OBJECT('a': JSON_OBJECTAGG('b': volatile_one() RETURNING text) FORMAT JSON); +SELECT JSON_OBJECT('a': JSON_OBJECTAGG('b': volatile_one() RETURNING text) FORMAT JSON); +EXPLAIN (VERBOSE, COSTS OFF) SELECT JSON_OBJECT('a': JSON_OBJECTAGG('b': stable_one() RETURNING text) FORMAT JSON); +SELECT JSON_OBJECT('a': JSON_OBJECTAGG('b': stable_one() RETURNING text) FORMAT JSON); +EXPLAIN (VERBOSE, COSTS OFF) SELECT JSON_OBJECT('a': JSON_OBJECTAGG('b': 1 RETURNING text) FORMAT JSON); +SELECT JSON_OBJECT('a': JSON_OBJECTAGG('b': 1 RETURNING text) FORMAT JSON); +DROP FUNCTION volatile_one, stable_one;