sql/json remaining issue

Hi,

On Tue, Apr 9, 2024 at 4:47 PM jian he <jian.universality@gmail.com> wrote:

hi.
`
| NESTED [ PATH ] json_path_specification [ AS json_path_name ]
COLUMNS ( json_table_column [, ...] )
NESTED [ PATH ] json_path_specification [ AS json_path_name ] COLUMNS
( json_table_column [, ...] )
`
"json_path_specification" should be "path_expression"?
--------------------------------------------------------------------------------------------------------------------------------------------------------------------------
drop table s1;
create or replace function random_text_1000() returns text
as $$select string_agg(md5(random()::text),'') from
generate_Series(1,1000) s $$ LANGUAGE SQL;

create unlogged table s1(a int GENERATED BY DEFAULT AS IDENTITY, js jsonb);
insert into s1(js)
select jsonb ('{"a":{"za":[{"z1": [11,2222]},{"z21": [22, 234,' || g
|| ']},{"z22": [32, 204,145]}]},"c": ' || g
|| ',"id": "' || random_text_1000() || '"}')
from generate_series(1_000_000, 1_000_000) g;
insert into s1(js)
select jsonb ('{"a":{"za":[{"z1": [11,2222]},{"z21": [22, 234,' || g
|| ']},{"z22": [32, 204,145]}]},"c": ' || g
|| ',"id": "' || random_text_1000() || '"}')
from generate_series(235, 235 + 200000,1) g;

select count(*), pg_size_pretty(pg_total_relation_size('s1')) from s1;
count | pg_size_pretty
--------+----------------
200002 | 6398 MB
--------------------------------------------------------------------------------------------------------------------------------------------------------------------------

explain(analyze, costs off,buffers, timing)
SELECT sub.*, s.a as s_a FROM s,
(values(23)) x(x),
generate_series(13, 13) y,
JSON_TABLE(js, '$' AS c1 PASSING x AS x, y AS y
COLUMNS (
xx1 int PATH '$.c',
NESTED PATH '$.a.za[2]' COLUMNS (NESTED PATH '$.z22[*]' as z22 COLUMNS
(c int PATH '$')),
NESTED PATH '$.a.za[1]' COLUMNS (d json PATH '$ ? (@.z21[*] == ($"x" -1))'),
NESTED PATH '$.a.za[0]' COLUMNS (NESTED PATH '$.z1[*] ? (@ >= ($"x"
-2))' as z1 COLUMNS (a int PATH '$')),
NESTED PATH '$.a.za[1]' COLUMNS
(NESTED PATH '$.z21[*] ? (@ >= ($"y" +121))' as z21 COLUMNS (b int
PATH '$ ? (@ <= ($"x" + 999976))' default -1000 ON EMPTY))
)) sub;

for one jsonb, it can expand to 7 rows, the above query will return
around 1.4 million rows.
i use the above query, and pg_log_backend_memory_contexts in another
session to check the memory usage.
didn't find memory over consumed issue.

but I am not sure this is the right way to check the memory consumption.
----------------------------------------------------------------------------------------------------------------------
begin;
SELECT sub.*, s.a as s_a FROM s,
(values(23)) x(x),
generate_series(13, 13) y,
JSON_TABLE(js, '$' AS c1 PASSING x AS x, y AS y
COLUMNS (
xx1 int PATH '$.c',
NESTED PATH '$.a.za[2]' COLUMNS (NESTED PATH '$.z22[*]' as z22 COLUMNS
(c int PATH '$')),
NESTED PATH '$.a.za[1]' COLUMNS (d json PATH '$ ? (@.z21[*] == ($"x" -1))'),
NESTED PATH '$.a.za[0]' COLUMNS (NESTED PATH '$.z1[*] ? (@ >= ($"x"
-2))' as z1 COLUMNS (a int PATH '$')),
NESTED PATH '$.a.za[1]' COLUMNS
(NESTED PATH '$.z21[*] ? (@ >= ($"y" +121))' as z21 COLUMNS (b int
PATH '$ ? (@ <= ($"x" + 999976))' error ON EMPTY))
)) sub;
rollback;

only the last row will fail, because of "error ON EMPTY", (1_000_000
<= 23 + 999976) is false.
I remember the very previous patch, because of error cleanup, it took
a lot of resources.
does our current implementation, only the very last row fail, will it
be easy to clean up the transaction?

I am not sure I understand your concern. Could you please rephrase
it? Which previous patch are you referring to and what problem did it
cause with respect to error cleanup?

Per-row memory allocated for each successful output row JSON_TABLE()
doesn't pile up, because it's allocated in a context that is reset
after evaluating each row; see tfuncLoadRows(). But again I may be
misunderstanding your concern.

the last query error message is:
`
ERROR: no SQL/JSON item
`

we are in ExecEvalJsonExprPath, can we output it to be:
`
ERROR: after applying json_path "5s", no SQL/JSON item found
`
in a json_table query, we can have multiple path_expressions, like the
above query.
it's not easy to know applying which path_expression failed.

Hmm, I'm not so sure about mentioning the details of the path because
path names are optional and printing path expression itself is not a
good idea. Perhaps, we could mention the column name which would
always be there, but we'd then need to add a new field column_name
that's optionally set to JsonFuncExpr and JsonExpr, that is, when they
are being set up for JSON_TABLE() columns. As shown in the attached.
With the patch you'll get:

ERROR: no SQL/JSON item found for column "b"

--
Thanks, Amit Langote

On Tue, Apr 9, 2024 at 8:37 PM Amit Langote <amitlangote09@gmail.com> wrote:

On Tue, Apr 9, 2024 at 4:47 PM jian he <jian.universality@gmail.com> wrote:

the last query error message is:
`
ERROR: no SQL/JSON item
`

we are in ExecEvalJsonExprPath, can we output it to be:
`
ERROR: after applying json_path "5s", no SQL/JSON item found
`
in a json_table query, we can have multiple path_expressions, like the
above query.
it's not easy to know applying which path_expression failed.

Hmm, I'm not so sure about mentioning the details of the path because
path names are optional and printing path expression itself is not a
good idea. Perhaps, we could mention the column name which would
always be there, but we'd then need to add a new field column_name
that's optionally set to JsonFuncExpr and JsonExpr, that is, when they
are being set up for JSON_TABLE() columns. As shown in the attached.
With the patch you'll get:

ERROR: no SQL/JSON item found for column "b"

Attached is a bit more polished version of that, which also addresses
the error messages in JsonPathQuery() and JsonPathValue(). I noticed
that there was comment I had written at one point during JSON_TABLE()
hacking that said that we should be doing this.

I've also added an open item for this.

--
Thanks, Amit Langote

On Wed, Apr 10, 2024 at 4:39 PM Amit Langote <amitlangote09@gmail.com> wrote:

Attached is a bit more polished version of that, which also addresses
the error messages in JsonPathQuery() and JsonPathValue(). I noticed
that there was comment I had written at one point during JSON_TABLE()
hacking that said that we should be doing this.

I've also added an open item for this.

`
| NESTED [ PATH ] json_path_specification [ AS json_path_name ]
COLUMNS ( json_table_column [, ...] )
NESTED [ PATH ] json_path_specification [ AS json_path_name ] COLUMNS
( json_table_column [, ...] )
`
"json_path_specification" should be "path_expression"?

your explanation about memory usage is clear to me!

The following are minor cosmetic issues while applying v2.
+errmsg("JSON path expression in JSON_VALUE should return singleton
scalar item")));
"singleton" is not intuitive to me.
Then I looked around.
https://www.postgresql.org/search/?u=%2Fdocs%2F16%2F&amp;q=singleton
There is only one appearance of "singleton" in the manual.
then I wonder what's the difference between
22038 ERRCODE_SINGLETON_SQL_JSON_ITEM_REQUIRED
2203F ERRCODE_SQL_JSON_SCALAR_REQUIRED

i assume '{"hello":"world"}' is a singleton, but not a scalar item?
if so, then I think the error message within the "if (count > 1)"
branch in JsonPathValue
should use ERRCODE_SINGLETON_SQL_JSON_ITEM_REQUIRED
within the "if (!IsAJsonbScalar(res))" branch should use
ERRCODE_SQL_JSON_SCALAR_REQUIRED
?

errhint("Use WITH WRAPPER clause to wrap SQL/JSON item sequence into array.")));
maybe
errhint("Use WITH WRAPPER clause to wrap SQL/JSON items into array.")));
or
errhint("Use WITH WRAPPER clause to wrap SQL/JSON item sequences into
array.")));

+ if (column_name)
+ ereport(ERROR,
+ (errcode(ERRCODE_MORE_THAN_ONE_SQL_JSON_ITEM),
+ errmsg("JSON path expression for column \"%s\" should return
singleton scalar item",
+ column_name)));
+ else
+ ereport(ERROR,
+ (errcode(ERRCODE_SQL_JSON_SCALAR_REQUIRED),
+ errmsg("JSON path expression in JSON_VALUE should return singleton
scalar item")));
the error message seems similar, but the error code is different?
both within "if (count > 1)" and "if (!IsAJsonbScalar(res))" branch.

in src/include/utils/jsonpath.h, comments
/* SQL/JSON item */
should be
/* SQL/JSON query functions */

elog(ERROR, "unrecognized json wrapper %d", wrapper);
should be
elog(ERROR, "unrecognized json wrapper %d", (int) wrapper);

On Thu, Apr 11, 2024 at 12:02 PM jian he <jian.universality@gmail.com> wrote:

On Wed, Apr 10, 2024 at 4:39 PM Amit Langote <amitlangote09@gmail.com> wrote:

Attached is a bit more polished version of that, which also addresses
the error messages in JsonPathQuery() and JsonPathValue(). I noticed
that there was comment I had written at one point during JSON_TABLE()
hacking that said that we should be doing this.

I've also added an open item for this.

`
| NESTED [ PATH ] json_path_specification [ AS json_path_name ]
COLUMNS ( json_table_column [, ...] )
NESTED [ PATH ] json_path_specification [ AS json_path_name ] COLUMNS
( json_table_column [, ...] )
`
"json_path_specification" should be "path_expression"?

Fixed in 0002.

your explanation about memory usage is clear to me!

The following are minor cosmetic issues while applying v2.
+errmsg("JSON path expression in JSON_VALUE should return singleton
scalar item")));
"singleton" is not intuitive to me.
Then I looked around.
https://www.postgresql.org/search/?u=%2Fdocs%2F16%2F&amp;q=singleton
There is only one appearance of "singleton" in the manual.

Yes, singleton is a term used a lot in the source code but let's keep
it out of error messages and docs. So fixed.

errhint("Use WITH WRAPPER clause to wrap SQL/JSON item sequence into array.")));
maybe
errhint("Use WITH WRAPPER clause to wrap SQL/JSON items into array.")));
or
errhint("Use WITH WRAPPER clause to wrap SQL/JSON item sequences into
array.")));

Changed to use "SQL/JSON items into array.".

then I wonder what's the difference between
22038 ERRCODE_SINGLETON_SQL_JSON_ITEM_REQUIRED
2203F ERRCODE_SQL_JSON_SCALAR_REQUIRED

i assume '{"hello":"world"}'  is a singleton, but not a scalar item?
if so, then I think the error message within the "if (count > 1)"
branch in JsonPathValue
should use ERRCODE_SINGLETON_SQL_JSON_ITEM_REQUIRED
within the "if (!IsAJsonbScalar(res))" branch should use
ERRCODE_SQL_JSON_SCALAR_REQUIRED
?
+ if (column_name)
+ ereport(ERROR,
+ (errcode(ERRCODE_MORE_THAN_ONE_SQL_JSON_ITEM),
+ errmsg("JSON path expression for column \"%s\" should return
singleton scalar item",
+ column_name)));
+ else
+ ereport(ERROR,
+ (errcode(ERRCODE_SQL_JSON_SCALAR_REQUIRED),
+ errmsg("JSON path expression in JSON_VALUE should return singleton
scalar item")));
the error message seems similar, but the error code is different?
both within "if (count > 1)" and "if (!IsAJsonbScalar(res))" branch.

Using different error codes for the same error is a copy-paste mistake
on my part. Fixed.

in src/include/utils/jsonpath.h, comments
/* SQL/JSON item */
should be
/* SQL/JSON query functions */

elog(ERROR, "unrecognized json wrapper %d", wrapper);
should be
elog(ERROR, "unrecognized json wrapper %d", (int) wrapper);

Fixed in 0003.

--
Thanks, Amit Langote

On Fri, Apr 12, 2024 at 5:44 PM Amit Langote <amitlangote09@gmail.com> wrote:

elog(ERROR, "unrecognized json wrapper %d", wrapper);
should be
elog(ERROR, "unrecognized json wrapper %d", (int) wrapper);

Fixed in 0003.

the fix seems not in 0003?
other than that, everything looks fine.

<programlisting>
SELECT * FROM JSON_TABLE (
'{"favorites":
{"movies":
[{"name": "One", "director": "John Doe"},
{"name": "Two", "director": "Don Joe"}],
"books":
[{"name": "Mystery", "authors": [{"name": "Brown Dan"}]},
{"name": "Wonder", "authors": [{"name": "Jun Murakami"},
{"name":"Craig Doe"}]}]
}}'::json, '$.favs[*]'
COLUMNS (user_id FOR ORDINALITY,
NESTED '$.movies[*]'
COLUMNS (
movie_id FOR ORDINALITY,
mname text PATH '$.name',
director text),
NESTED '$.books[*]'
COLUMNS (
book_id FOR ORDINALITY,
bname text PATH '$.name',
NESTED '$.authors[*]'
COLUMNS (
author_id FOR ORDINALITY,
author_name text PATH '$.name'))));
</programlisting>

I actually did run the query, it returns null.
'$.favs[*]'
should be
'$.favorites[*]'

one more minor thing, I previously mentioned in getJsonPathVariable
ereport(ERROR,
(errcode(ERRCODE_UNDEFINED_OBJECT),
errmsg("could not find jsonpath variable \"%s\"",
pnstrdup(varName, varNameLength))));

do we need to remove pnstrdup?

hi.
https://wiki.postgresql.org/wiki/PostgreSQL_17_Open_Items#Open_Issues
issue: Problems with deparsed SQL/JSON query function

original the bug report link:
/messages/by-id/CACJufxEqhqsfrg_p7EMyo5zak3d767iFDL8vz_4=ZBHpOtrghw@mail.gmail.com

forgive me for putting it in the new email thread.
I made the following change, added several tests on it.

--- a/src/backend/parser/parse_expr.c
+++ b/src/backend/parser/parse_expr.c
@@ -4636,10 +4636,10 @@ transformJsonBehavior(ParseState *pstate,
JsonBehavior *behavior,
  {
  expr = transformExprRecurse(pstate, behavior->expr);
  if (!IsA(expr, Const) && !IsA(expr, FuncExpr) &&
- !IsA(expr, OpExpr))
+ !IsA(expr, OpExpr) && !IsA(expr, CoerceViaIO) && !IsA(expr, CoerceToDomain))
  ereport(ERROR,
  (errcode(ERRCODE_DATATYPE_MISMATCH),
- errmsg("can only specify a constant, non-aggregate function, or
operator expression for DEFAULT"),
+ errmsg("can only specify a constant, non-aggregate function, or
operator expression or cast expression for DEFAULT"),
  parser_errposition(pstate, exprLocation(expr))));
  if (contain_var_clause(expr))
  ereport(ERROR,

these two expression node also looks like Const:
CoerceViaIO: "foo1"'::jsonb::text
CoerceToDomain: 'foo'::jsonb_test_domain

we need to deal with these two, otherwise we cannot use domain type in
DEFAULT expression.
also the following should not fail:
SELECT JSON_VALUE(jsonb '{"d1": "foo"}', '$.a2' returning text DEFAULT
'"foo1"'::text::json::text ON ERROR);

we have `if (contain_var_clause(expr))` further check it,
so it should be fine?

Hi,

On Sat, Apr 13, 2024 at 11:12 PM jian he <jian.universality@gmail.com> wrote:

On Fri, Apr 12, 2024 at 5:44 PM Amit Langote <amitlangote09@gmail.com> wrote:

elog(ERROR, "unrecognized json wrapper %d", wrapper);
should be
elog(ERROR, "unrecognized json wrapper %d", (int) wrapper);

Fixed in 0003.

the fix seems not in 0003?
other than that, everything looks fine.

<programlisting>
SELECT * FROM JSON_TABLE (
'{"favorites":
{"movies":
[{"name": "One", "director": "John Doe"},
{"name": "Two", "director": "Don Joe"}],
"books":
[{"name": "Mystery", "authors": [{"name": "Brown Dan"}]},
{"name": "Wonder", "authors": [{"name": "Jun Murakami"},
{"name":"Craig Doe"}]}]
}}'::json, '$.favs[*]'
COLUMNS (user_id FOR ORDINALITY,
NESTED '$.movies[*]'
COLUMNS (
movie_id FOR ORDINALITY,
mname text PATH '$.name',
director text),
NESTED '$.books[*]'
COLUMNS (
book_id FOR ORDINALITY,
bname text PATH '$.name',
NESTED '$.authors[*]'
COLUMNS (
author_id FOR ORDINALITY,
author_name text PATH '$.name'))));
</programlisting>

I actually did run the query, it returns null.
'$.favs[*]'
should be
'$.favorites[*]'

Oops, fixed.

I've combined these patches into one -- attached 0001. Will push tomorrow.

one more minor thing, I previously mentioned in getJsonPathVariable
ereport(ERROR,
(errcode(ERRCODE_UNDEFINED_OBJECT),
errmsg("could not find jsonpath variable \"%s\"",
pnstrdup(varName, varNameLength))));

do we need to remove pnstrdup?

Looking at this again, it seems like that's necessary because varName,
being a string extracted from JsonPathItem, is not necessarily
null-terminated. There are many pndstrdup()s in jsonpath_exec.c
because of that aspect.

Now studying the JsonBehavior DEFAULT expression issue and your patch.

--
Thanks, Amit Langote

On Mon, Apr 15, 2024 at 9:46 PM Amit Langote <amitlangote09@gmail.com> wrote:

On Sat, Apr 13, 2024 at 11:12 PM jian he <jian.universality@gmail.com> wrote:

On Fri, Apr 12, 2024 at 5:44 PM Amit Langote <amitlangote09@gmail.com> wrote:

elog(ERROR, "unrecognized json wrapper %d", wrapper);
should be
elog(ERROR, "unrecognized json wrapper %d", (int) wrapper);

Fixed in 0003.

the fix seems not in 0003?
other than that, everything looks fine.

Oops, really fixed now in 0002.

I've combined these patches into one -- attached 0001. Will push tomorrow.

Decided to break the error message improvement patch into its own
after all -- attached 0001.

Now studying the JsonBehavior DEFAULT expression issue and your patch.

I found some more coercion-related expression nodes that must also be
checked along with CoerceViaIO and CoerceToDomain. Also, after fixing
the code to allow them, I found that we'd need to also check
recursively whether their argument expression is also one of the
supported expression nodes. Also, I decided that it's not necessary
to include "cast" in the error message as one of the supported
expressions.

Will push all today.

--
Thanks, Amit Langote

On Thu, Apr 18, 2024 at 9:33 AM Amit Langote <amitlangote09@gmail.com> wrote:

On Mon, Apr 15, 2024 at 9:46 PM Amit Langote <amitlangote09@gmail.com> wrote:

On Sat, Apr 13, 2024 at 11:12 PM jian he <jian.universality@gmail.com> wrote:

On Fri, Apr 12, 2024 at 5:44 PM Amit Langote <amitlangote09@gmail.com> wrote:

elog(ERROR, "unrecognized json wrapper %d", wrapper);
should be
elog(ERROR, "unrecognized json wrapper %d", (int) wrapper);

Fixed in 0003.

the fix seems not in 0003?
other than that, everything looks fine.

Oops, really fixed now in 0002.

I've combined these patches into one -- attached 0001. Will push tomorrow.

Decided to break the error message improvement patch into its own
after all -- attached 0001.

Now studying the JsonBehavior DEFAULT expression issue and your patch.

I found some more coercion-related expression nodes that must also be
checked along with CoerceViaIO and CoerceToDomain. Also, after fixing
the code to allow them, I found that we'd need to also check
recursively whether their argument expression is also one of the
supported expression nodes. Also, I decided that it's not necessary
to include "cast" in the error message as one of the supported
expressions.

Will push all today.

Totally forgot to drop a note here that I pushed those and marked the
2 open items as resolved.

--
Thanks, Amit Langote