Thread
Commits
-
Fix error reporting for SQL/JSON path type mismatches
- 519fa0433b37 19 (unreleased) landed
- 15ba0702c1ae 18.2 landed
- b5511fed500e 17.8 landed
-
transformJsonFuncExpr pathspec cache lookup failed
jian he <jian.universality@gmail.com> — 2025-11-07T02:50:09Z
hi. in transformJsonFuncExpr: path_spec = transformExprRecurse(pstate, func->pathspec); path_spec = coerce_to_target_type(pstate, path_spec, exprType(path_spec), JSONPATHOID, -1, COERCION_EXPLICIT, COERCE_IMPLICIT_CAST, exprLocation(path_spec)); if (path_spec == NULL) ereport(ERROR, (errcode(ERRCODE_DATATYPE_MISMATCH), errmsg("JSON path expression must be of type %s, not of type %s", "jsonpath", format_type_be(exprType(path_spec))), parser_errposition(pstate, exprLocation(path_spec)))); There is no test for this, if you try it, you can easily reach "cache lookup failed". SELECT JSON_VALUE(jsonb 'null', NULL::date); ERROR: cache lookup failed for type 0 because we first call ``format_type_be(exprType(path_spec))),`` then ereport. format_type_be can not code with InvalidOid. A patch is attached. ----------- Also, note that we allow: SELECT * FROM JSON_TABLE(jsonb'"1.23"', '$' COLUMNS (js2 int PATH '$')); but don't allow SELECT * FROM JSON_TABLE(jsonb'"1.23"', '$'::jsonpath COLUMNS (js2 int PATH '$')); Maybe we should support this. since every A_Const should have a type for it. Allowing something like: JSON_TABLE(jsonb '"1.23"', '$'::some_jsonpath_type ... ) seems consistent. I guess that's a separate issue, so I didn't touch it. -- jian https://www.enterprisedb.com/ -
Re: transformJsonFuncExpr pathspec cache lookup failed
Kirill Reshke <reshkekirill@gmail.com> — 2025-11-07T06:25:59Z
On Fri, 7 Nov 2025 at 07:50, jian he <jian.universality@gmail.com> wrote: > > hi. > > in transformJsonFuncExpr: > > path_spec = transformExprRecurse(pstate, func->pathspec); > path_spec = coerce_to_target_type(pstate, path_spec, exprType(path_spec), > JSONPATHOID, -1, > COERCION_EXPLICIT, COERCE_IMPLICIT_CAST, > exprLocation(path_spec)); > if (path_spec == NULL) > ereport(ERROR, > (errcode(ERRCODE_DATATYPE_MISMATCH), > errmsg("JSON path expression must be of type %s, not > of type %s", > "jsonpath", format_type_be(exprType(path_spec))), > parser_errposition(pstate, exprLocation(path_spec)))); > > There is no test for this, if you try it, you can easily reach "cache > lookup failed". > SELECT JSON_VALUE(jsonb 'null', NULL::date); > ERROR: cache lookup failed for type 0 > > because we first call ``format_type_be(exprType(path_spec))),`` then ereport. > format_type_be can not code with InvalidOid. > > A patch is attached. > ----------- > Also, note that we allow: > SELECT * FROM JSON_TABLE(jsonb'"1.23"', '$' COLUMNS (js2 int PATH '$')); > but don't allow > SELECT * FROM JSON_TABLE(jsonb'"1.23"', '$'::jsonpath COLUMNS (js2 int > PATH '$')); > > Maybe we should support this. > since every A_Const should have a type for it. Allowing something like: > JSON_TABLE(jsonb '"1.23"', '$'::some_jsonpath_type ... ) > seems consistent. > I guess that's a separate issue, so I didn't touch it. > > -- > jian > https://www.enterprisedb.com/ Hi! I tried your fix and this indeed fixes an issue. Two minor comments: First, in the `src/backend/parser/parse_expr.c` fil there are multiple examples of working with `coerce_to_target_type`, they all share different coding practice: ``` coerced_expr = coerce_to_target_type(.., expr, ..) if (coerced_expr == NULL) ereport(); expr = coerced_expr; ``` Instead of ``` expr = coerce_to_target_type(.., expr, ..) if (expr == NULL) ereport(); ``` Let's be consistent? Second, about allowing JSON_TABLE(jsonb '"1.23"', '$'::some_jsonpath_type ... ). The way we check input for JSON_TABLE is we simply check for node type in the parser. So, I think it may be too difficult to change that, because we do not work with catalog in parser, so we cannot resolve if the type is indeed some_json_type? What we can do is we can change parser rules, to allow more user-friendly error here: ``` reshke=# SELECT * FROM JSON_TABLE(jsonb '{"d1": "H"}', '$' COLUMNS (js1 text PATH '$.d1'::jsonpath )); ERROR: syntax error at or near "::" LINE 2: COLUMNS (js1 text PATH '$.d1'::jsonpath )); ^ ``` The way we can achieve it is to change the second token in json_table_column_path_clause_opt to a_expr and give a more clear error. Thoughts? -- Best regards, Kirill Reshke -
Re: transformJsonFuncExpr pathspec cache lookup failed
jian he <jian.universality@gmail.com> — 2025-11-19T02:55:07Z
On Fri, Nov 7, 2025 at 2:26 PM Kirill Reshke <reshkekirill@gmail.com> wrote: > > Hi! > I tried your fix and this indeed fixes an issue. Two minor comments: > > First, > in the `src/backend/parser/parse_expr.c` fil there are multiple > examples of working with `coerce_to_target_type`, they all share > different coding practice: > > ``` > coerced_expr = coerce_to_target_type(.., expr, ..) > > if (coerced_expr == NULL) > ereport(); > > > expr = coerced_expr; > ``` > > Instead of > ``` > expr = coerce_to_target_type(.., expr, ..) > > if (expr == NULL) > ereport(); > ``` > > Let's be consistent? > IMHO, > coerced_expr = coerce_to_target_type(.., expr, ..) is better than > expr = coerce_to_target_type(.., expr, ..) I changed accordingly.
-
Re: transformJsonFuncExpr pathspec cache lookup failed
Amit Langote <amitlangote09@gmail.com> — 2025-11-26T07:32:20Z
Hi, On Wed, Nov 19, 2025 at 11:55 AM jian he <jian.universality@gmail.com> wrote: > On Fri, Nov 7, 2025 at 2:26 PM Kirill Reshke <reshkekirill@gmail.com> wrote: > > > > Hi! > > I tried your fix and this indeed fixes an issue. Two minor comments: > > > > First, > > in the `src/backend/parser/parse_expr.c` fil there are multiple > > examples of working with `coerce_to_target_type`, they all share > > different coding practice: > > > > ``` > > coerced_expr = coerce_to_target_type(.., expr, ..) > > > > if (coerced_expr == NULL) > > ereport(); > > > > > > expr = coerced_expr; > > ``` > > > > Instead of > > ``` > > expr = coerce_to_target_type(.., expr, ..) > > > > if (expr == NULL) > > ereport(); > > ``` > > > > Let's be consistent? > > > > IMHO, > > coerced_expr = coerce_to_target_type(.., expr, ..) > is better than > > expr = coerce_to_target_type(.., expr, ..) > > I changed accordingly. I agree, though I prefer a different name for that coerced_* variable. Also, let’s define the _type and _loc variables inside the error block. Updated patch attached. Please feel free to post a patch for the 2nd issue. -- Thanks, Amit Langote
-
Re: transformJsonFuncExpr pathspec cache lookup failed
jian he <jian.universality@gmail.com> — 2025-11-26T11:55:03Z
On Wed, Nov 26, 2025 at 3:32 PM Amit Langote <amitlangote09@gmail.com> wrote: > > > > First, > > > in the `src/backend/parser/parse_expr.c` fil there are multiple > > > examples of working with `coerce_to_target_type`, they all share > > > different coding practice: > > > > > > ``` > > > coerced_expr = coerce_to_target_type(.., expr, ..) > > > > > > if (coerced_expr == NULL) > > > ereport(); > > > > > > > > > expr = coerced_expr; > > > ``` > > > > > > Instead of > > > ``` > > > expr = coerce_to_target_type(.., expr, ..) > > > > > > if (expr == NULL) > > > ereport(); > > > ``` > > > > > > Let's be consistent? > > > > > > > IMHO, > > > coerced_expr = coerce_to_target_type(.., expr, ..) > > is better than > > > expr = coerce_to_target_type(.., expr, ..) > > > > I changed accordingly. > > I agree, though I prefer a different name for that coerced_* variable. > Also, let’s define the _type and _loc variables inside the error > block. Updated patch attached. > v2-0001 looks good to me. -- jian https://www.enterprisedb.com/
-
Re: transformJsonFuncExpr pathspec cache lookup failed
Kirill Reshke <reshkekirill@gmail.com> — 2025-11-26T11:56:52Z
On Wed, 26 Nov 2025 at 10:32, Amit Langote <amitlangote09@gmail.com> wrote: > > Hi, > > On Wed, Nov 19, 2025 at 11:55 AM jian he <jian.universality@gmail.com> wrote: > > On Fri, Nov 7, 2025 at 2:26 PM Kirill Reshke <reshkekirill@gmail.com> wrote: > > > > > > Hi! > > > I tried your fix and this indeed fixes an issue. Two minor comments: > > > > > > First, > > > in the `src/backend/parser/parse_expr.c` fil there are multiple > > > examples of working with `coerce_to_target_type`, they all share > > > different coding practice: > > > > > > ``` > > > coerced_expr = coerce_to_target_type(.., expr, ..) > > > > > > if (coerced_expr == NULL) > > > ereport(); > > > > > > > > > expr = coerced_expr; > > > ``` > > > > > > Instead of > > > ``` > > > expr = coerce_to_target_type(.., expr, ..) > > > > > > if (expr == NULL) > > > ereport(); > > > ``` > > > > > > Let's be consistent? > > > > > > > IMHO, > > > coerced_expr = coerce_to_target_type(.., expr, ..) > > is better than > > > expr = coerce_to_target_type(.., expr, ..) > > > > I changed accordingly. > > I agree, though I prefer a different name for that coerced_* variable. > Also, let’s define the _type and _loc variables inside the error > block. Updated patch attached. > > Please feel free to post a patch for the 2nd issue. > > -- > Thanks, Amit Langote v2 lgtm -- Best regards, Kirill Reshke
-
Re: transformJsonFuncExpr pathspec cache lookup failed
Amit Langote <amitlangote09@gmail.com> — 2025-11-27T03:16:22Z
On Wed, Nov 26, 2025 at 8:57 PM Kirill Reshke <reshkekirill@gmail.com> wrote: > On Wed, 26 Nov 2025 at 10:32, Amit Langote <amitlangote09@gmail.com> wrote: > > On Wed, Nov 19, 2025 at 11:55 AM jian he <jian.universality@gmail.com> wrote: > > > On Fri, Nov 7, 2025 at 2:26 PM Kirill Reshke <reshkekirill@gmail.com> wrote: > > > > > > > > Hi! > > > > I tried your fix and this indeed fixes an issue. Two minor comments: > > > > > > > > First, > > > > in the `src/backend/parser/parse_expr.c` fil there are multiple > > > > examples of working with `coerce_to_target_type`, they all share > > > > different coding practice: > > > > > > > > ``` > > > > coerced_expr = coerce_to_target_type(.., expr, ..) > > > > > > > > if (coerced_expr == NULL) > > > > ereport(); > > > > > > > > > > > > expr = coerced_expr; > > > > ``` > > > > > > > > Instead of > > > > ``` > > > > expr = coerce_to_target_type(.., expr, ..) > > > > > > > > if (expr == NULL) > > > > ereport(); > > > > ``` > > > > > > > > Let's be consistent? > > > > > > > > > > IMHO, > > > > coerced_expr = coerce_to_target_type(.., expr, ..) > > > is better than > > > > expr = coerce_to_target_type(.., expr, ..) > > > > > > I changed accordingly. > > > > I agree, though I prefer a different name for that coerced_* variable. > > Also, let’s define the _type and _loc variables inside the error > > block. Updated patch attached. > > > > Please feel free to post a patch for the 2nd issue. > > v2 lgtm Pushed after moving the _type, _loc variable declarations outside after all, because I had missed that they are passed to coerce_to_target_type() as well in Jian's patch. -- Thanks, Amit Langote