Code review for commit b6e1157e7d

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

diff --git a/src/backend/nodes/makefuncs.c b/src/backend/nodes/makefuncs.c
index c6c310d2531ac37b0cd6cbe498f91410d4f8a465..0e7e6e46d943b79c36825ee0f47fbad6eccd46d1 100644 (file)
--- 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 c41e6bb984cddc52cb629ea105621314e9b088df..503d76aae07ebf19dd1b03a7ff0bb1f03031c080 100644 (file)
--- 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 7a44a374e4918126947f0312de3e7cc608023313..60080e877e8d798c8b2be1a37bf3d6b7d3f06777 100644 (file)
--- 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 5a05caa87445f6a3f970d40854ee1856f4044320..c08c06373a9ad82900c23fba45a189a15ea3b3b2 100644 (file)
--- 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 06d991b7257f7c61b089a74c7aba4afc7eb11e33..3180703005507a4893cf17d32616ee1437898bc8 100644 (file)
--- 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 0d2df069b3b06b064d4815f526035a2c337ed169..e1aadc39cfbe4daf1d6b3c38ef2189efa1196d86 100644 (file)
--- 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
 {