From 92deabcaac100256baa25009dd656b1015490327 Mon Sep 17 00:00:00 2001
From: Amit Langote <amitlan@postgresql.org>
Date: Thu, 17 Oct 2024 16:03:18 +0900
Subject: [PATCH v3] Revert "Don't include CaseTestExpr in
 JsonValueExpr.formatted_expr"

This reverts commit b6e1157e7d339c4e20d68448125a4cef42b1ac9d.
---
 src/backend/executor/execExpr.c       | 17 +++++++--
 src/backend/optimizer/util/clauses.c  | 23 +++++++++---
 src/backend/parser/parse_expr.c       | 32 +++++++++++------
 src/include/nodes/primnodes.h         | 10 +++---
 src/test/regress/expected/sqljson.out | 50 +++++++++++++++++++++++++++
 src/test/regress/sql/sqljson.sql      | 15 ++++++++
 6 files changed, 124 insertions(+), 23 deletions(-)

diff --git a/src/backend/executor/execExpr.c b/src/backend/executor/execExpr.c
index c8077aa57b..daa4cb9d69 100644
--- a/src/backend/executor/execExpr.c
+++ b/src/backend/executor/execExpr.c
@@ -2307,8 +2307,21 @@ ExecInitExprRec(Expr *node, ExprState *state,
 			{
 				JsonValueExpr *jve = (JsonValueExpr *) node;
 
-				Assert(jve->formatted_expr != NULL);
-				ExecInitExprRec(jve->formatted_expr, state, resv, resnull);
+				ExecInitExprRec(jve->raw_expr, state, resv, resnull);
+
+				if (jve->formatted_expr)
+				{
+					Datum	   *innermost_caseval = state->innermost_caseval;
+					bool	   *innermost_isnull = state->innermost_casenull;
+
+					state->innermost_caseval = resv;
+					state->innermost_casenull = resnull;
+
+					ExecInitExprRec(jve->formatted_expr, state, resv, resnull);
+
+					state->innermost_caseval = innermost_caseval;
+					state->innermost_casenull = innermost_isnull;
+				}
 				break;
 			}
 
diff --git a/src/backend/optimizer/util/clauses.c b/src/backend/optimizer/util/clauses.c
index b4e085e9d4..6872dad5c2 100644
--- a/src/backend/optimizer/util/clauses.c
+++ b/src/backend/optimizer/util/clauses.c
@@ -2915,12 +2915,25 @@ eval_const_expressions_mutator(Node *node,
 		case T_JsonValueExpr:
 			{
 				JsonValueExpr *jve = (JsonValueExpr *) node;
-				Node	   *formatted;
+				Node	   *raw;
 
-				formatted = eval_const_expressions_mutator((Node *) jve->formatted_expr,
-														   context);
-				if (formatted && IsA(formatted, Const))
-					return formatted;
+				raw = eval_const_expressions_mutator((Node *) jve->raw_expr,
+													 context);
+				if (raw && IsA(raw, Const))
+				{
+					Node	   *formatted;
+					Node	   *save_case_val = context->case_val;
+
+					context->case_val = raw;
+
+					formatted = eval_const_expressions_mutator((Node *) jve->formatted_expr,
+															   context);
+
+					context->case_val = save_case_val;
+
+					if (formatted && IsA(formatted, Const))
+						return formatted;
+				}
 				break;
 			}
 
diff --git a/src/backend/parser/parse_expr.c b/src/backend/parser/parse_expr.c
index ef0b560f5e..40736730cc 100644
--- a/src/backend/parser/parse_expr.c
+++ b/src/backend/parser/parse_expr.c
@@ -3268,6 +3268,21 @@ makeJsonByteaToTextConversion(Node *expr, JsonFormat *format, int location)
 	return (Node *) fexpr;
 }
 
+/*
+ * Make a CaseTestExpr node.
+ */
+static Node *
+makeCaseTestExpr(Node *expr)
+{
+	CaseTestExpr *placeholder = makeNode(CaseTestExpr);
+
+	placeholder->typeId = exprType(expr);
+	placeholder->typeMod = exprTypmod(expr);
+	placeholder->collation = exprCollation(expr);
+
+	return (Node *) placeholder;
+}
+
 /*
  * Transform JSON value expression using specified input JSON format or
  * default format otherwise, coercing to the targettype if needed.
@@ -3354,9 +3369,12 @@ transformJsonValueExpr(ParseState *pstate, const char *constructName,
 	if (format != JS_FORMAT_DEFAULT ||
 		(OidIsValid(targettype) && exprtype != targettype))
 	{
+		Node	   *orig = makeCaseTestExpr(expr);
 		Node	   *coerced;
 		bool		only_allow_cast = OidIsValid(targettype);
 
+		expr = orig;
+
 		/*
 		 * PASSING args are handled appropriately by GetJsonPathVar() /
 		 * JsonItemFromDatum().
@@ -3416,7 +3434,7 @@ transformJsonValueExpr(ParseState *pstate, const char *constructName,
 			coerced = (Node *) fexpr;
 		}
 
-		if (coerced == expr)
+		if (coerced == orig)
 			expr = rawexpr;
 		else
 		{
@@ -3666,15 +3684,7 @@ makeJsonConstructorExpr(ParseState *pstate, JsonConstructorType type,
 	 * that is of type JSON or JSONB to the coercion function.
 	 */
 	if (fexpr)
-	{
-		CaseTestExpr *cte = makeNode(CaseTestExpr);
-
-		cte->typeId = exprType((Node *) fexpr);
-		cte->typeMod = exprTypmod((Node *) fexpr);
-		cte->collation = exprCollation((Node *) fexpr);
-
-		placeholder = (Node *) cte;
-	}
+		placeholder = makeCaseTestExpr((Node *) fexpr);
 	else
 	{
 		CaseTestExpr *cte = makeNode(CaseTestExpr);
@@ -4042,7 +4052,7 @@ transformJsonParseArg(ParseState *pstate, Node *jsexpr, JsonFormat *format,
 	{
 		JsonValueExpr *jve;
 
-		expr = raw_expr;
+		expr = makeCaseTestExpr(raw_expr);
 		expr = makeJsonByteaToTextConversion(expr, format, exprLocation(expr));
 		*exprtype = TEXTOID;
 
diff --git a/src/include/nodes/primnodes.h b/src/include/nodes/primnodes.h
index ea47652adb..fa3cbc858f 100644
--- a/src/include/nodes/primnodes.h
+++ b/src/include/nodes/primnodes.h
@@ -1338,7 +1338,7 @@ typedef struct CaseWhen
  *	* Nested FieldStore/SubscriptingRef assignment expressions in INSERT/UPDATE;
  *	  see transformAssignmentIndirection().
  *	* Placeholder for intermediate results in some SQL/JSON expression nodes,
- *	  such as JsonConstructorExpr.
+ *	  such as JsonConstructorExpr.coercion, JsonValueExpr.formatted_expr.
  *
  * The uses in CaseExpr and ArrayCoerceExpr are safe only to the extent that
  * there is not any other CaseExpr or ArrayCoerceExpr between the value source
@@ -1669,15 +1669,15 @@ 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().
+ * formatted_expr, if needed, contains a formatting expression such as
+ * CoerceViaIO, where the argument is a CaseTestExpr. At runtime, this
+ * CaseTestExpr points to the value obtained by evaluating raw_expr.
  */
 typedef struct JsonValueExpr
 {
 	NodeTag		type;
 	Expr	   *raw_expr;		/* raw expression */
-	Expr	   *formatted_expr; /* formatted expression */
+	Expr	   *formatted_expr; /* formatted expression or NULL */
 	JsonFormat *format;			/* FORMAT clause, if specified */
 } JsonValueExpr;
 
diff --git a/src/test/regress/expected/sqljson.out b/src/test/regress/expected/sqljson.out
index 9adfbe15f6..5fe5968b42 100644
--- a/src/test/regress/expected/sqljson.out
+++ b/src/test/regress/expected/sqljson.out
@@ -1300,3 +1300,53 @@ 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 accidentally processed by the
+-- planner (preprocess_aggref()) causing the plan to carry Aggrefs containing
+-- incorrect / out-of-sync value of aggtransno.  One symptom of that is the
+-- 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 42dbec26d6..031cab7365 100644
--- a/src/test/regress/sql/sqljson.sql
+++ b/src/test/regress/sql/sqljson.sql
@@ -480,3 +480,18 @@ 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 accidentally processed by the
+-- planner (preprocess_aggref()) causing the plan to carry Aggrefs containing
+-- incorrect / out-of-sync value of aggtransno.  One symptom of that is the
+-- 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;
-- 
2.43.0

