Re: transformJsonFuncExpr pathspec cache lookup failed

From: Kirill Reshke <reshkekirill(at)gmail(dot)com>
To: jian he <jian(dot)universality(at)gmail(dot)com>
Cc: PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: transformJsonFuncExpr pathspec cache lookup failed
Date: 2025-11-07 06:25:59
Message-ID: CALdSSPiHtj4R3vuOuP7qMpO8XV4uYZ_=4j_Upcn0VR97GepboA@mail.gmail.com
Views: Whole Thread | Raw Message | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

On Fri, 7 Nov 2025 at 07:50, jian he <jian(dot)universality(at)gmail(dot)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

In response to

Browse pgsql-hackers by date

  From Date Subject
Next Message Fujii Masao 2025-11-07 07:44:49 Re: doc: Improve description of io_combine_limit and io_max_combine_limit GUCs
Previous Message Bertrand Drouvot 2025-11-07 06:17:27 Re: Consistently use the XLogRecPtrIsInvalid() macro