From 7c7412cae3ea8f8accdec1022969a9360b74f253 Mon Sep 17 00:00:00 2001
From: Amit Langote <amitlan@postgresql.org>
Date: Fri, 21 Jul 2023 19:15:34 +0900
Subject: [PATCH] Code review for commit b6e1157e7d
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
b6e1157e7d made some changes to enforce that
JsonValueExpr.formatted_expr is always set and is the expression that
gives a JsonValueExpr its runtime value, but that's not really
apparent from the comments about and the code manipulating
formatted_expr. This commit fixes that.
Per suggestion from Álvaro Herrera.
Discussion: https://postgr.es/m/20230718155313.3wqg6encgt32adqb%40alvherre.pgsql
---
src/backend/nodes/makefuncs.c | 11 ++++-------
src/backend/nodes/nodeFuncs.c | 4 +---
src/backend/parser/gram.y | 4 +++-
src/backend/parser/parse_expr.c | 17 +++++++++++------
src/include/nodes/makefuncs.h | 3 ++-
src/include/nodes/primnodes.h | 5 +++--
6 files changed, 24 insertions(+), 20 deletions(-)
diff --git a/src/backend/nodes/makefuncs.c b/src/backend/nodes/makefuncs.c
index c6c310d253..0e7e6e46d9 100644
--- a/src/backend/nodes/makefuncs.c
+++ b/src/backend/nodes/makefuncs.c
@@ -848,16 +848,13 @@ makeJsonFormat(JsonFormatType type, JsonEncoding encoding, int location)
* creates a JsonValueExpr node
*/
JsonValueExpr *
-makeJsonValueExpr(Expr *expr, JsonFormat *format)
+makeJsonValueExpr(Expr *raw_expr, Expr *formatted_expr,
+ JsonFormat *format)
{
JsonValueExpr *jve = makeNode(JsonValueExpr);
- jve->raw_expr = expr;
-
- /*
- * Set after checking the format, if needed, in transformJsonValueExpr().
- */
- jve->formatted_expr = NULL;
+ jve->raw_expr = raw_expr;
+ jve->formatted_expr = formatted_expr;
jve->format = format;
return jve;
diff --git a/src/backend/nodes/nodeFuncs.c b/src/backend/nodes/nodeFuncs.c
index c41e6bb984..503d76aae0 100644
--- a/src/backend/nodes/nodeFuncs.c
+++ b/src/backend/nodes/nodeFuncs.c
@@ -225,9 +225,7 @@ exprType(const Node *expr)
{
const JsonValueExpr *jve = (const JsonValueExpr *) expr;
- type = exprType((Node *)
- (jve->formatted_expr ? jve->formatted_expr :
- jve->raw_expr));
+ type = exprType((Node *) jve->formatted_expr);
}
break;
case T_JsonConstructorExpr:
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index 7a44a374e4..60080e877e 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -16362,7 +16362,9 @@ opt_asymmetric: ASYMMETRIC
json_value_expr:
a_expr json_format_clause_opt
{
- $$ = (Node *) makeJsonValueExpr((Expr *) $1, castNode(JsonFormat, $2));
+ /* formatted_expr will be set during parse-analysis. */
+ $$ = (Node *) makeJsonValueExpr((Expr *) $1, NULL,
+ castNode(JsonFormat, $2));
}
;
diff --git a/src/backend/parser/parse_expr.c b/src/backend/parser/parse_expr.c
index 5a05caa874..c08c06373a 100644
--- a/src/backend/parser/parse_expr.c
+++ b/src/backend/parser/parse_expr.c
@@ -3205,6 +3205,10 @@ makeJsonByteaToTextConversion(Node *expr, JsonFormat *format, int location)
/*
* Transform JSON value expression using specified input JSON format or
* default format otherwise.
+ *
+ * Returned expression is either ve->raw_expr coerced to text (if needed) or
+ * a JsonValueExpr with formatted_expr set to the coerced copy of raw_expr
+ * if the specified format requires it.
*/
static Node *
transformJsonValueExpr(ParseState *pstate, const char *constructName,
@@ -3304,6 +3308,10 @@ transformJsonValueExpr(ParseState *pstate, const char *constructName,
}
}
+ /* If returning a JsonValueExpr, formatted_expr must have been set. */
+ Assert(!IsA(expr, JsonValueExpr) ||
+ ((JsonValueExpr *) expr)->formatted_expr != NULL);
+
return expr;
}
@@ -3631,13 +3639,12 @@ transformJsonArrayQueryConstructor(ParseState *pstate,
makeString(pstrdup("a")));
colref->location = ctor->location;
- agg->arg = makeJsonValueExpr((Expr *) colref, ctor->format);
-
/*
* No formatting necessary, so set formatted_expr to be the same as
* raw_expr.
*/
- agg->arg->formatted_expr = agg->arg->raw_expr;
+ agg->arg = makeJsonValueExpr((Expr *) colref, (Expr *) colref,
+ ctor->format);
agg->absent_on_null = ctor->absent_on_null;
agg->constructor = makeNode(JsonAggConstructor);
agg->constructor->agg_order = NIL;
@@ -3906,9 +3913,7 @@ transformJsonParseArg(ParseState *pstate, Node *jsexpr, JsonFormat *format,
expr = makeJsonByteaToTextConversion(expr, format, exprLocation(expr));
*exprtype = TEXTOID;
- jve = makeJsonValueExpr((Expr *) raw_expr, format);
-
- jve->formatted_expr = (Expr *) expr;
+ jve = makeJsonValueExpr((Expr *) raw_expr, (Expr *) expr, format);
expr = (Node *) jve;
}
else
diff --git a/src/include/nodes/makefuncs.h b/src/include/nodes/makefuncs.h
index 06d991b725..3180703005 100644
--- a/src/include/nodes/makefuncs.h
+++ b/src/include/nodes/makefuncs.h
@@ -110,7 +110,8 @@ extern VacuumRelation *makeVacuumRelation(RangeVar *relation, Oid oid, List *va_
extern JsonFormat *makeJsonFormat(JsonFormatType type, JsonEncoding encoding,
int location);
-extern JsonValueExpr *makeJsonValueExpr(Expr *expr, JsonFormat *format);
+extern JsonValueExpr *makeJsonValueExpr(Expr *raw_expr, Expr *formatted_expr,
+ JsonFormat *format);
extern Node *makeJsonKeyValue(Node *key, Node *value);
extern Node *makeJsonIsPredicate(Node *expr, JsonFormat *format,
JsonValueType item_type, bool unique_keys,
diff --git a/src/include/nodes/primnodes.h b/src/include/nodes/primnodes.h
index 0d2df069b3..e1aadc39cf 100644
--- a/src/include/nodes/primnodes.h
+++ b/src/include/nodes/primnodes.h
@@ -1596,8 +1596,9 @@ typedef struct JsonReturning
* JsonValueExpr -
* representation of JSON value expression (expr [FORMAT JsonFormat])
*
- * Note that raw_expr is only there for displaying and is not evaluated by
- * ExecInterpExpr() and eval_const_exprs_mutator().
+ * 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().
*/
typedef struct JsonValueExpr
{