transformJsonFuncExpr pathspec cache lookup failed

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