Обсуждение: transformJsonFuncExpr pathspec cache lookup failed
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/
Вложения
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
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.
Вложения
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
Вложения
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/
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
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