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 <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

diff --git a/src/backend/executor/execExpr.c b/src/backend/executor/execExpr.c
index bf3a08c5f08d935c90d8e9fe7f5819f396259051..60f9eb28cd40befea6697a22ab8f8d235f328dff 100644 (file)
--- a/src/backend/executor/execExpr.c
+++ b/src/backend/executor/execExpr.c
@@ -2294,6 +2294,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 62650995cb65a924eec754b0b94694ae1d2aad0d..9b36e04048943d899d95b225a14f5c3e915c5910 100644 (file)
--- a/src/backend/optimizer/util/clauses.c
+++ b/src/backend/optimizer/util/clauses.c
@@ -2897,13 +2897,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 e1aadc39cfbe4daf1d6b3c38ef2189efa1196d86..633e9d6f3e5217f77d4ea0e4c797cb667f6d2fc6 100644 (file)
--- a/src/include/nodes/primnodes.h
+++ b/src/include/nodes/primnodes.h
@@ -1596,15 +1596,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_exprs_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 ff75ea78ee643f6476337614bcd0bf01e1ff75c8..8554dc38a4e2c8572d84cb3504327a07f6885c3e 100644 (file)
--- a/src/test/regress/expected/sqljson.out
+++ b/src/test/regress/expected/sqljson.out
@@ -951,3 +951,52 @@ CREATE OR REPLACE VIEW public.is_json_view AS
     '{}'::text IS JSON OBJECT WITH UNIQUE KEYS AS object
    FROM generate_series(1, 3) i(i)
 DROP VIEW is_json_view;
+-- 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 6fbcdeed61c7a316f39283092213488df9878e8c..085606bf20b6879c9aa02ad9abb0bc38fcfdfcb7 100644 (file)
--- a/src/test/regress/sql/sqljson.sql
+++ b/src/test/regress/sql/sqljson.sql
@@ -383,3 +383,17 @@ SELECT '1' IS JSON AS "any", ('1' || i) IS JSON SCALAR AS "scalar", '[]' IS NOT
 \sv is_json_view
 
 DROP VIEW is_json_view;
+
+-- 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;