From 71bfd1543f8b68e1013d3e8540b6b5aaf98e02e9 Mon Sep 17 00:00:00 2001 From: Alvaro Herrera Date: Tue, 4 Apr 2023 14:04:30 +0200 Subject: [PATCH] Code review for recent SQL/JSON commits - At the last minute and for no particularly good reason, I changed the WITHOUT token to be marked especially for lookahead, from the one in WITHOUT TIME to the one in WITHOUT UNIQUE. Study of upcoming patches (where a new WITHOUT ARRAY WRAPPER clause is added) showed me that the former was better, so put it back the way the original patch had it. - update exprTypmod() for JsonConstructorExpr to return the typmod of the RETURNING clause, as a comment there suggested. Perhaps it's possible for this to make a difference with datetime types, but I didn't try to build a test case. - The nodeFuncs.c support code for new nodes was calling walker() directly instead of the WALK() macro as introduced by commit 1c27d16e6e5c. Modernize that. Also add exprLocation() support for a couple of nodes that missed it. Lastly, reorder the code more sensibly. The WITHOUT_LA -> WITHOUT change means that stored rules containing either WITHOUT TIME ZONE or WITHOUT UNIQUE KEYS would change representation. Therefore, bump catversion. Discussion: https://postgr.es/m/20230329181708.e64g2tpy7jyufqkr@alvherre.pgsql --- src/backend/nodes/nodeFuncs.c | 420 ++++++++++++++++----------- src/backend/parser/gram.y | 8 +- src/backend/parser/parser.c | 4 +- src/include/catalog/catversion.h | 2 +- src/interfaces/ecpg/preproc/parser.c | 4 +- 5 files changed, 266 insertions(+), 172 deletions(-) diff --git a/src/backend/nodes/nodeFuncs.c b/src/backend/nodes/nodeFuncs.c index fdb0c6b3fe..fe3a113c8f 100644 --- a/src/backend/nodes/nodeFuncs.c +++ b/src/backend/nodes/nodeFuncs.c @@ -218,6 +218,21 @@ exprType(const Node *expr) else type = XMLOID; break; + case T_JsonValueExpr: + { + const JsonValueExpr *jve = (const JsonValueExpr *) expr; + + type = exprType((Node *) + (jve->formatted_expr ? jve->formatted_expr : + jve->raw_expr)); + } + break; + case T_JsonConstructorExpr: + type = ((const JsonConstructorExpr *) expr)->returning->typid; + break; + case T_JsonIsPredicate: + type = BOOLOID; + break; case T_NullTest: type = BOOLOID; break; @@ -249,21 +264,6 @@ exprType(const Node *expr) case T_PlaceHolderVar: type = exprType((Node *) ((const PlaceHolderVar *) expr)->phexpr); break; - case T_JsonValueExpr: - { - const JsonValueExpr *jve = (const JsonValueExpr *) expr; - - type = exprType((Node *) - (jve->formatted_expr ? jve->formatted_expr : - jve->raw_expr)); - } - break; - case T_JsonConstructorExpr: - type = ((const JsonConstructorExpr *) expr)->returning->typid; - break; - case T_JsonIsPredicate: - type = BOOLOID; - break; default: elog(ERROR, "unrecognized node type: %d", (int) nodeTag(expr)); type = InvalidOid; /* keep compiler quiet */ @@ -486,6 +486,10 @@ exprTypmod(const Node *expr) return typmod; } break; + case T_JsonValueExpr: + return exprTypmod((Node *) ((const JsonValueExpr *) expr)->formatted_expr); + case T_JsonConstructorExpr: + return ((const JsonConstructorExpr *) expr)->returning->typmod; case T_CoerceToDomain: return ((const CoerceToDomain *) expr)->resulttypmod; case T_CoerceToDomainValue: @@ -494,10 +498,6 @@ exprTypmod(const Node *expr) return ((const SetToDefault *) expr)->typeMod; case T_PlaceHolderVar: return exprTypmod((Node *) ((const PlaceHolderVar *) expr)->phexpr); - case T_JsonValueExpr: - return exprTypmod((Node *) ((const JsonValueExpr *) expr)->formatted_expr); - case T_JsonConstructorExpr: - return -1; /* XXX maybe expr->returning->typmod? */ default: break; } @@ -942,6 +942,23 @@ exprCollation(const Node *expr) else coll = InvalidOid; break; + case T_JsonValueExpr: + coll = exprCollation((Node *) ((const JsonValueExpr *) expr)->formatted_expr); + break; + case T_JsonConstructorExpr: + { + const JsonConstructorExpr *ctor = (const JsonConstructorExpr *) expr; + + if (ctor->coercion) + coll = exprCollation((Node *) ctor->coercion); + else + coll = InvalidOid; + } + break; + case T_JsonIsPredicate: + /* IS JSON's result is boolean ... */ + coll = InvalidOid; /* ... so it has no collation */ + break; case T_NullTest: /* NullTest's result is boolean ... */ coll = InvalidOid; /* ... so it has no collation */ @@ -973,22 +990,6 @@ exprCollation(const Node *expr) case T_PlaceHolderVar: coll = exprCollation((Node *) ((const PlaceHolderVar *) expr)->phexpr); break; - case T_JsonValueExpr: - coll = exprCollation((Node *) ((const JsonValueExpr *) expr)->formatted_expr); - break; - case T_JsonConstructorExpr: - { - const JsonConstructorExpr *ctor = (const JsonConstructorExpr *) expr; - - if (ctor->coercion) - coll = exprCollation((Node *) ctor->coercion); - else - coll = InvalidOid; - } - break; - case T_JsonIsPredicate: - coll = InvalidOid; /* result is always an boolean type */ - break; default: elog(ERROR, "unrecognized node type: %d", (int) nodeTag(expr)); coll = InvalidOid; /* keep compiler quiet */ @@ -1171,6 +1172,24 @@ exprSetCollation(Node *expr, Oid collation) (collation == DEFAULT_COLLATION_OID) : (collation == InvalidOid)); break; + case T_JsonValueExpr: + exprSetCollation((Node *) ((JsonValueExpr *) expr)->formatted_expr, + collation); + break; + case T_JsonConstructorExpr: + { + JsonConstructorExpr *ctor = (JsonConstructorExpr *) expr; + + if (ctor->coercion) + exprSetCollation((Node *) ctor->coercion, collation); + else + Assert(!OidIsValid(collation)); /* result is always a + * json[b] type */ + } + break; + case T_JsonIsPredicate: + Assert(!OidIsValid(collation)); /* result is always boolean */ + break; case T_NullTest: /* NullTest's result is boolean ... */ Assert(!OidIsValid(collation)); /* ... so never set a collation */ @@ -1196,24 +1215,6 @@ exprSetCollation(Node *expr, Oid collation) /* NextValueExpr's result is an integer type ... */ Assert(!OidIsValid(collation)); /* ... so never set a collation */ break; - case T_JsonValueExpr: - exprSetCollation((Node *) ((JsonValueExpr *) expr)->formatted_expr, - collation); - break; - case T_JsonConstructorExpr: - { - JsonConstructorExpr *ctor = (JsonConstructorExpr *) expr; - - if (ctor->coercion) - exprSetCollation((Node *) ctor->coercion, collation); - else - Assert(!OidIsValid(collation)); /* result is always a - * json[b] type */ - } - break; - case T_JsonIsPredicate: - Assert(!OidIsValid(collation)); /* result is always boolean */ - break; default: elog(ERROR, "unrecognized node type: %d", (int) nodeTag(expr)); break; @@ -1476,6 +1477,18 @@ exprLocation(const Node *expr) exprLocation((Node *) xexpr->args)); } break; + case T_JsonFormat: + loc = ((const JsonFormat *) expr)->location; + break; + case T_JsonValueExpr: + loc = exprLocation((Node *) ((const JsonValueExpr *) expr)->raw_expr); + break; + case T_JsonConstructorExpr: + loc = ((const JsonConstructorExpr *) expr)->location; + break; + case T_JsonIsPredicate: + loc = ((const JsonIsPredicate *) expr)->location; + break; case T_NullTest: { const NullTest *nexpr = (const NullTest *) expr; @@ -1636,6 +1649,28 @@ exprLocation(const Node *expr) case T_CommonTableExpr: loc = ((const CommonTableExpr *) expr)->location; break; + case T_JsonKeyValue: + /* just use the key's location */ + loc = exprLocation((Node *) ((const JsonKeyValue *) expr)->key); + break; + case T_JsonObjectConstructor: + loc = ((const JsonObjectConstructor *) expr)->location; + break; + case T_JsonArrayConstructor: + loc = ((const JsonArrayConstructor *) expr)->location; + break; + case T_JsonArrayQueryConstructor: + loc = ((const JsonArrayQueryConstructor *) expr)->location; + break; + case T_JsonAggConstructor: + loc = ((const JsonAggConstructor *) expr)->location; + break; + case T_JsonObjectAgg: + loc = exprLocation((Node *) ((const JsonObjectAgg *) expr)->constructor); + break; + case T_JsonArrayAgg: + loc = exprLocation((Node *) ((const JsonArrayAgg *) expr)->constructor); + break; case T_PlaceHolderVar: /* just use argument's location */ loc = exprLocation((Node *) ((const PlaceHolderVar *) expr)->phexpr); @@ -1656,15 +1691,6 @@ exprLocation(const Node *expr) case T_PartitionRangeDatum: loc = ((const PartitionRangeDatum *) expr)->location; break; - case T_JsonValueExpr: - loc = exprLocation((Node *) ((const JsonValueExpr *) expr)->raw_expr); - break; - case T_JsonConstructorExpr: - loc = ((const JsonConstructorExpr *) expr)->location; - break; - case T_JsonIsPredicate: - loc = ((const JsonIsPredicate *) expr)->location; - break; default: /* for any other node type it's just unknown... */ loc = -1; @@ -2190,6 +2216,30 @@ expression_tree_walker_impl(Node *node, return true; } break; + case T_JsonValueExpr: + { + JsonValueExpr *jve = (JsonValueExpr *) node; + + if (WALK(jve->raw_expr)) + return true; + if (WALK(jve->formatted_expr)) + return true; + } + break; + case T_JsonConstructorExpr: + { + JsonConstructorExpr *ctor = (JsonConstructorExpr *) node; + + if (WALK(ctor->args)) + return true; + if (WALK(ctor->func)) + return true; + if (WALK(ctor->coercion)) + return true; + } + break; + case T_JsonIsPredicate: + return WALK(((JsonIsPredicate *) node)->expr); case T_NullTest: return WALK(((NullTest *) node)->arg); case T_BooleanTest: @@ -2242,6 +2292,73 @@ expression_tree_walker_impl(Node *node, return true; } break; + case T_JsonKeyValue: + { + JsonKeyValue *kv = (JsonKeyValue *) node; + + if (WALK(kv->key)) + return true; + if (WALK(kv->value)) + return true; + } + break; + case T_JsonObjectConstructor: + { + JsonObjectConstructor *ctor = (JsonObjectConstructor *) node; + + if (LIST_WALK(ctor->exprs)) + return true; + } + break; + case T_JsonArrayConstructor: + { + JsonArrayConstructor *ctor = (JsonArrayConstructor *) node; + + if (LIST_WALK(ctor->exprs)) + return true; + } + break; + case T_JsonArrayQueryConstructor: + { + JsonArrayQueryConstructor *ctor = (JsonArrayQueryConstructor *) node; + + if (WALK(ctor->query)) + return true; + } + break; + case T_JsonAggConstructor: + { + JsonAggConstructor *ctor = (JsonAggConstructor *) node; + + if (WALK(ctor->agg_filter)) + return true; + if (WALK(ctor->agg_order)) + return true; + if (WALK(ctor->over)) + return true; + } + break; + case T_JsonObjectAgg: + { + JsonObjectAgg *ctor = (JsonObjectAgg *) node; + + if (WALK(ctor->constructor)) + return true; + if (WALK(ctor->arg)) + return true; + } + break; + case T_JsonArrayAgg: + { + JsonArrayAgg *ctor = (JsonArrayAgg *) node; + + if (WALK(ctor->constructor)) + return true; + if (WALK(ctor->arg)) + return true; + } + break; + case T_PartitionBoundSpec: { PartitionBoundSpec *pbs = (PartitionBoundSpec *) node; @@ -2396,30 +2513,6 @@ expression_tree_walker_impl(Node *node, return true; } break; - case T_JsonValueExpr: - { - JsonValueExpr *jve = (JsonValueExpr *) node; - - if (WALK(jve->raw_expr)) - return true; - if (WALK(jve->formatted_expr)) - return true; - } - break; - case T_JsonConstructorExpr: - { - JsonConstructorExpr *ctor = (JsonConstructorExpr *) node; - - if (WALK(ctor->args)) - return true; - if (WALK(ctor->func)) - return true; - if (WALK(ctor->coercion)) - return true; - } - break; - case T_JsonIsPredicate: - return walker(((JsonIsPredicate *) node)->expr, context); default: elog(ERROR, "unrecognized node type: %d", (int) nodeTag(node)); @@ -2743,6 +2836,7 @@ expression_tree_mutator_impl(Node *node, break; case T_Param: case T_CaseTestExpr: + case T_JsonFormat: case T_CoerceToDomainValue: case T_SetToDefault: case T_CurrentOfExpr: @@ -2750,7 +2844,6 @@ expression_tree_mutator_impl(Node *node, case T_RangeTblRef: case T_SortGroupClause: case T_CTESearchClause: - case T_JsonFormat: return (Node *) copyObject(node); case T_WithCheckOption: { @@ -3099,6 +3192,52 @@ expression_tree_mutator_impl(Node *node, return (Node *) newnode; } break; + case T_JsonReturning: + { + JsonReturning *jr = (JsonReturning *) node; + JsonReturning *newnode; + + FLATCOPY(newnode, jr, JsonReturning); + MUTATE(newnode->format, jr->format, JsonFormat *); + + return (Node *) newnode; + } + case T_JsonValueExpr: + { + JsonValueExpr *jve = (JsonValueExpr *) node; + JsonValueExpr *newnode; + + FLATCOPY(newnode, jve, JsonValueExpr); + MUTATE(newnode->raw_expr, jve->raw_expr, Expr *); + MUTATE(newnode->formatted_expr, jve->formatted_expr, Expr *); + MUTATE(newnode->format, jve->format, JsonFormat *); + + return (Node *) newnode; + } + case T_JsonConstructorExpr: + { + JsonConstructorExpr *jce = (JsonConstructorExpr *) node; + JsonConstructorExpr *newnode; + + FLATCOPY(newnode, jce, JsonConstructorExpr); + MUTATE(newnode->args, jce->args, List *); + MUTATE(newnode->func, jce->func, Expr *); + MUTATE(newnode->coercion, jce->coercion, Expr *); + MUTATE(newnode->returning, jce->returning, JsonReturning *); + + return (Node *) newnode; + } + case T_JsonIsPredicate: + { + JsonIsPredicate *pred = (JsonIsPredicate *) node; + JsonIsPredicate *newnode; + + FLATCOPY(newnode, pred, JsonIsPredicate); + MUTATE(newnode->expr, pred->expr, Node *); + MUTATE(newnode->format, pred->format, JsonFormat *); + + return (Node *) newnode; + } case T_NullTest: { NullTest *ntest = (NullTest *) node; @@ -3394,51 +3533,6 @@ expression_tree_mutator_impl(Node *node, return (Node *) newnode; } break; - case T_JsonReturning: - { - JsonReturning *jr = (JsonReturning *) node; - JsonReturning *newnode; - - FLATCOPY(newnode, jr, JsonReturning); - MUTATE(newnode->format, jr->format, JsonFormat *); - - return (Node *) newnode; - } - case T_JsonValueExpr: - { - JsonValueExpr *jve = (JsonValueExpr *) node; - JsonValueExpr *newnode; - - FLATCOPY(newnode, jve, JsonValueExpr); - MUTATE(newnode->raw_expr, jve->raw_expr, Expr *); - MUTATE(newnode->formatted_expr, jve->formatted_expr, Expr *); - MUTATE(newnode->format, jve->format, JsonFormat *); - - return (Node *) newnode; - } - case T_JsonConstructorExpr: - { - JsonConstructorExpr *jve = (JsonConstructorExpr *) node; - JsonConstructorExpr *newnode; - - FLATCOPY(newnode, jve, JsonConstructorExpr); - MUTATE(newnode->args, jve->args, List *); - MUTATE(newnode->func, jve->func, Expr *); - MUTATE(newnode->coercion, jve->coercion, Expr *); - MUTATE(newnode->returning, jve->returning, JsonReturning *); - - return (Node *) newnode; - } - case T_JsonIsPredicate: - { - JsonIsPredicate *pred = (JsonIsPredicate *) node; - JsonIsPredicate *newnode; - - FLATCOPY(newnode, pred, JsonIsPredicate); - MUTATE(newnode->expr, pred->expr, Node *); - - return (Node *) newnode; - } default: elog(ERROR, "unrecognized node type: %d", (int) nodeTag(node)); @@ -3700,6 +3794,7 @@ raw_expression_tree_walker_impl(Node *node, switch (nodeTag(node)) { + case T_JsonFormat: case T_SetToDefault: case T_CurrentOfExpr: case T_Integer: @@ -3710,7 +3805,6 @@ raw_expression_tree_walker_impl(Node *node, case T_ParamRef: case T_A_Const: case T_A_Star: - case T_JsonFormat: /* primitive node types with no subnodes */ break; case T_Alias: @@ -3769,6 +3863,36 @@ raw_expression_tree_walker_impl(Node *node, return true; } break; + case T_JsonReturning: + return WALK(((JsonReturning *) node)->format); + case T_JsonValueExpr: + { + JsonValueExpr *jve = (JsonValueExpr *) node; + + if (WALK(jve->raw_expr)) + return true; + if (WALK(jve->formatted_expr)) + return true; + if (WALK(jve->format)) + return true; + } + break; + case T_JsonConstructorExpr: + { + JsonConstructorExpr *ctor = (JsonConstructorExpr *) node; + + if (WALK(ctor->args)) + return true; + if (WALK(ctor->func)) + return true; + if (WALK(ctor->coercion)) + return true; + if (WALK(ctor->returning)) + return true; + } + break; + case T_JsonIsPredicate: + return WALK(((JsonIsPredicate *) node)->expr); case T_NullTest: return WALK(((NullTest *) node)->arg); case T_BooleanTest: @@ -4173,34 +4297,6 @@ raw_expression_tree_walker_impl(Node *node, case T_CommonTableExpr: /* search_clause and cycle_clause are not interesting here */ return WALK(((CommonTableExpr *) node)->ctequery); - case T_JsonReturning: - return WALK(((JsonReturning *) node)->format); - case T_JsonValueExpr: - { - JsonValueExpr *jve = (JsonValueExpr *) node; - - if (WALK(jve->raw_expr)) - return true; - if (WALK(jve->formatted_expr)) - return true; - if (WALK(jve->format)) - return true; - } - break; - case T_JsonConstructorExpr: - { - JsonConstructorExpr *ctor = (JsonConstructorExpr *) node; - - if (WALK(ctor->args)) - return true; - if (WALK(ctor->func)) - return true; - if (WALK(ctor->coercion)) - return true; - if (WALK(ctor->returning)) - return true; - } - break; case T_JsonOutput: { JsonOutput *out = (JsonOutput *) node; @@ -4285,8 +4381,6 @@ raw_expression_tree_walker_impl(Node *node, return true; } break; - case T_JsonIsPredicate: - return walker(((JsonIsPredicate *) node)->expr, context); default: elog(ERROR, "unrecognized node type: %d", (int) nodeTag(node)); diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y index 1b5daf6734..acf6cf4866 100644 --- a/src/backend/parser/gram.y +++ b/src/backend/parser/gram.y @@ -824,7 +824,7 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query); /* SQL/JSON related keywords */ %nonassoc UNIQUE JSON %nonassoc KEYS OBJECT_P SCALAR VALUE_P -%nonassoc WITH WITHOUT_LA +%nonassoc WITH WITHOUT /* * To support target_el without AS, it used to be necessary to assign IDENT an @@ -14313,7 +14313,7 @@ ConstInterval: opt_timezone: WITH_LA TIME ZONE { $$ = true; } - | WITHOUT TIME ZONE { $$ = false; } + | WITHOUT_LA TIME ZONE { $$ = false; } | /*EMPTY*/ { $$ = false; } ; @@ -16464,8 +16464,8 @@ json_predicate_type_constraint: json_key_uniqueness_constraint_opt: WITH UNIQUE KEYS { $$ = true; } | WITH UNIQUE { $$ = true; } - | WITHOUT_LA UNIQUE KEYS { $$ = false; } - | WITHOUT_LA UNIQUE { $$ = false; } + | WITHOUT UNIQUE KEYS { $$ = false; } + | WITHOUT UNIQUE { $$ = false; } | /* EMPTY */ %prec KEYS { $$ = false; } ; diff --git a/src/backend/parser/parser.c b/src/backend/parser/parser.c index 65eb087657..e17c310cc1 100644 --- a/src/backend/parser/parser.c +++ b/src/backend/parser/parser.c @@ -241,10 +241,10 @@ base_yylex(YYSTYPE *lvalp, YYLTYPE *llocp, core_yyscan_t yyscanner) break; case WITHOUT: - /* Replace WITHOUT by WITHOUT_LA if it's followed by UNIQUE */ + /* Replace WITHOUT by WITHOUT_LA if it's followed by TIME */ switch (next_token) { - case UNIQUE: + case TIME: cur_token = WITHOUT_LA; break; } diff --git a/src/include/catalog/catversion.h b/src/include/catalog/catversion.h index 81e78230e4..67f3f632f0 100644 --- a/src/include/catalog/catversion.h +++ b/src/include/catalog/catversion.h @@ -57,6 +57,6 @@ */ /* yyyymmddN */ -#define CATALOG_VERSION_NO 202303311 +#define CATALOG_VERSION_NO 202304041 #endif diff --git a/src/interfaces/ecpg/preproc/parser.c b/src/interfaces/ecpg/preproc/parser.c index a40f4bef09..38e7acb680 100644 --- a/src/interfaces/ecpg/preproc/parser.c +++ b/src/interfaces/ecpg/preproc/parser.c @@ -159,10 +159,10 @@ filtered_base_yylex(void) break; case WITHOUT: - /* Replace WITHOUT by WITHOUT_LA if it's followed by UNIQUE */ + /* Replace WITHOUT by WITHOUT_LA if it's followed by TIME */ switch (next_token) { - case UNIQUE: + case TIME: cur_token = WITHOUT_LA; break; }