Re: remaining sql/json patches - Mailing list pgsql-hackers

Hi Jian,

Thanks for the reviews and sorry for the late reply. Replying to all
emails in one.

On Thu, Jan 25, 2024 at 11:39 PM jian he <jian.universality@gmail.com> wrote:
> On Thu, Jan 25, 2024 at 7:54 PM Amit Langote <amitlangote09@gmail.com> wrote:
> > > The problem with returning comp_domain_with_typmod from json_value()
> > > seems to be that it's using a text-to-record CoerceViaIO expression
> > > picked from JsonExpr.item_coercions, which behaves differently than
> > > the expression tree that the following uses:
> > >
> > > select ('abcd', 42)::comp_domain_with_typmod;
> > >    row
> > > ----------
> > >  (abc,42)
> > > (1 row)
> >
> > Oh, it hadn't occurred to me to check what trying to coerce a "string"
> > containing the record literal would do:
> >
> > select '(''abcd'', 42)'::comp_domain_with_typmod;
> > ERROR:  value too long for type character(3)
> > LINE 1: select '(''abcd'', 42)'::comp_domain_with_typmod;
> >
> > which is the same thing as what the JSON_QUERY() and JSON_VALUE() are
> > running into.  So, it might be fair to think that the error is not a
> > limitation of the SQL/JSON patch but an underlying behavior that it
> > has to accept as is.
>
> Hi, I reconciled with these cases.
> What bugs me now is the first query of the following 4 cases (for comparison).
> SELECT JSON_QUERY(jsonb '[1,2]', '$' RETURNING char(3) omit quotes);
> SELECT JSON_QUERY(jsonb '[1,2]', '$' RETURNING char(3) keep quotes);
> SELECT JSON_QUERY(jsonb '[1,2]', '$' RETURNING text omit quotes);
> SELECT JSON_QUERY(jsonb '[1,2]', '$' RETURNING text keep quotes);

Fixed:

SELECT JSON_QUERY(jsonb '"[1,2]"', '$' RETURNING char(3) omit quotes);
 json_query
------------
 [1,
(1 row)

SELECT JSON_QUERY(jsonb '"[1,2]"', '$' RETURNING char(3) keep quotes);
 json_query
------------
 "[1
(1 row)

SELECT JSON_QUERY(jsonb '"[1,2]"', '$' RETURNING text omit quotes);
 json_query
------------
 [1,2]
(1 row)

SELECT JSON_QUERY(jsonb '"[1,2]"', '$' RETURNING text keep quotes);
 json_query
------------
 "[1,2]"
(1 row)

I didn't go with your proposed solution to check targettypmod in
ExecEvalJsonCoercion() though.

> I did some minor refactoring on the function coerceJsonFuncExprOutput.
> it will make the following queries return null instead of error. NULL
> is the return of json_value.
>
>     SELECT JSON_QUERY(jsonb '"123"', '$' RETURNING int2);
>     SELECT JSON_QUERY(jsonb '"123"', '$' RETURNING int4);
>     SELECT JSON_QUERY(jsonb '"123"', '$' RETURNING int8);
>     SELECT JSON_QUERY(jsonb '"123"', '$' RETURNING bool);
>     SELECT JSON_QUERY(jsonb '"123"', '$' RETURNING numeric);
>     SELECT JSON_QUERY(jsonb '"123"', '$' RETURNING real);
>     SELECT JSON_QUERY(jsonb '"123"', '$' RETURNING float8);

I didn't really want to add an exception in the parser for these
specific types, but I agree that it's not great that the current code
doesn't respect the default NULL ON ERROR behavior, so I've adopted
your fix.  I'm not sure if we'll do so in the future but the code can
be removed if we someday make the non-IO cast functions handle errors
softly too.

On Wed, Jan 31, 2024 at 11:52 PM jian he <jian.universality@gmail.com> wrote:
>
> Hi.
> minor issues.
> I am wondering do we need add `pg_node_attr(query_jumble_ignore)`
> to some of our created structs in src/include/nodes/parsenodes.h in
> v39-0001-Add-SQL-JSON-query-functions.patch

We haven't added those to the node structs of other SQL/JSON
functions, so I'm inclined to skip adding them in this patch.

> diff --git a/src/backend/parser/parse_jsontable.c
> b/src/backend/parser/parse_jsontable.c
> new file mode 100644
> index 0000000000..25b8204dc6
> --- /dev/null
> +++ b/src/backend/parser/parse_jsontable.c
> @@ -0,0 +1,718 @@
> +/*-------------------------------------------------------------------------
> + *
> + * parse_jsontable.c
> + *  parsing of JSON_TABLE
> + *
> + * Portions Copyright (c) 1996-2022, PostgreSQL Global Development Group
> + * Portions Copyright (c) 1994, Regents of the University of California
> + *
> + *
> + * IDENTIFICATION
> + *  src/backend/parser/parse_jsontable.c
> + *
> + *-------------------------------------------------------------------------
> + */
> 2022 should change to 2024.

Oops, fixed.

On Mon, Feb 5, 2024 at 9:28 PM jian he <jian.universality@gmail.com> wrote:
>
> based on this query:
> begin;
> SET LOCAL TIME ZONE 10.5;
> with cte(s) as (select jsonb '"2023-08-15 12:34:56 +05:30"')
> select JSON_QUERY(s, '$.timestamp_tz()')::text,'+10.5'::text,
> 'timestamp_tz'::text from cte
> union all
> select JSON_QUERY(s, '$.time()')::text,'+10.5'::text, 'time'::text from cte
> union all
> select JSON_QUERY(s, '$.timestamp()')::text,'+10.5'::text,
> 'timestamp'::text from cte
> union all
> select JSON_QUERY(s, '$.date()')::text,'+10.5'::text, 'date'::text from cte
> union all
> select JSON_QUERY(s, '$.time_tz()')::text,'+10.5'::text,
> 'time_tz'::text from cte;
>
> SET LOCAL TIME ZONE -8;
> with cte(s) as (select jsonb '"2023-08-15 12:34:56 +05:30"')
> select JSON_QUERY(s, '$.timestamp_tz()')::text,'+10.5'::text,
> 'timestamp_tz'::text from cte
> union all
> select JSON_QUERY(s, '$.time()')::text,'+10.5'::text, 'time'::text from cte
> union all
> select JSON_QUERY(s, '$.timestamp()')::text,'+10.5'::text,
> 'timestamp'::text from cte
> union all
> select JSON_QUERY(s, '$.date()')::text,'+10.5'::text, 'date'::text from cte
> union all
> select JSON_QUERY(s, '$.time_tz()')::text,'+10.5'::text,
> 'time_tz'::text from cte;
> commit;
>
> I made some changes on jspIsMutableWalker.
> various new jsonpath methods added:
> https://git.postgresql.org/cgit/postgresql.git/commit/?id=66ea94e8e606529bb334515f388c62314956739e
> so we need to change jspIsMutableWalker accordingly.

Thanks for the heads up about that, merged.

On Wed, Feb 14, 2024 at 9:00 AM jian he <jian.universality@gmail.com> wrote:
>
> This part is already committed.
> ereport(ERROR,
> (errcode(ERRCODE_UNDEFINED_OBJECT),
> errmsg("could not find jsonpath variable \"%s\"",
> pnstrdup(varName, varNameLength))));
>
> but, you can simply use:
> ereport(ERROR,
> (errcode(ERRCODE_UNDEFINED_OBJECT),
> errmsg("could not find jsonpath variable \"%s\"",varName)));
>
> maybe not worth the trouble.

Yeah, maybe the pnstrdup is unnecessary.  I'm inclined to leave that
alone for now and fix it later, not as part of this patch.

> I kind of want to know, using `pnstrdup`, when the malloc related
> memory will be freed?

That particular pnstrdup() will allocate somewhere in the
ExecutorState memory context, which gets reset during the transaction
abort processing, releasing that memory.

> json_query and json_query doc explanation is kind of crammed together.
> Do you think it's a good idea to use </listitem> and </itemizedlist>?
> it will look like bullet points. but the distance between the bullet
> point and the first text in the same line is a little bit long, so it
> may not look elegant.
> I've attached the picture, json_query is using `</listitem> and
> </itemizedlist>`, json_value is as of the v39.

Yeah, the bullet point list layout looks kind of neat, and is not
unprecedented because we have a list in the description of
json_poulate_record() for one.  Though I wasn't able to come up with a
good breakdown of the points into sentences of appropriate length.
I'm inclined to leave that beautification project to another day.

> other than this and previous points, v39, 0001 looks good to go.

I've attached the updated patches.  I would like to get 0001 committed
after I spent a couple more days staring at it.

Alvaro, do you still think that 0002 is a good idea and would you like
to push it yourself?

--
Thanks, Amit Langote