Subject: Re: remaining sql/json patches

From: jian he <[email-masked]>
Date: Tue, 6 Feb 2024 12:55:51 +0800

On Thu, Jan 25, 2024 at 10:39 PM jian he <[email-masked]> wrote:
>
> On Thu, Jan 25, 2024 at 7:54 PM Amit Langote <[email-masked]> 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);
>

based on v39.
in ExecEvalJsonCoercion
coercion->targettypmod related function calls:
json_populate_type calls populate_record_field, then populate_scalar,
later will eventually call InputFunctionCallSafe.

so I make the following change:
--- a/src/backend/executor/execExprInterp.c
+++ b/src/backend/executor/execExprInterp.c
@@ -4533,7 +4533,7 @@ ExecEvalJsonCoercion(ExprState *state, ExprEvalStep *op,
         * deed ourselves by calling the input function, that is, after removing
         * the quotes.
         */
-       if (jb && JB_ROOT_IS_SCALAR(jb) && coercion->omit_quotes)
+       if ((jb && JB_ROOT_IS_SCALAR(jb) && coercion->omit_quotes) ||
coercion->targettypmod != -1)

now the following two return the same result:  `[1,`
SELECT JSON_QUERY(jsonb '[1,2]', '$' RETURNING char(3) omit quotes);
SELECT JSON_QUERY(jsonb '[1,2]', '$' RETURNING char(3) keep quotes);

---

From: jian he <[email-masked]>
Date: Wed, 14 Feb 2024 08:00:00 +0800

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.
I kind of want to know, using `pnstrdup`, when the malloc related
memory will be freed?

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.

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

---

From: Amit Langote <[email-masked]>
Date: Mon, 4 Mar 2024 18:40:10 +0900

Hi Jian,

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

> 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 <[email-masked]> 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 <[email-masked]> 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.

> 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

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

> </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

---

From: Erik Rijkers <[email-masked]>
Date: Mon, 4 Mar 2024 15:51:53 +0100

> [v40-0001-Add-SQL-JSON-query-functions.patch]
 > [v40-0002-Show-function-name-in-TableFuncScan.patch]
 > [v40-0003-JSON_TABLE.patch]

In my hands (applying with patch), the patches, esp. 0001, do not apply. 
  But I see the cfbot builds without problem so maybe just ignore these 
FAILED lines.  Better get them merged - so I can test there...

Erik

checking file doc/src/sgml/func.sgml
checking file src/backend/catalog/sql_features.txt
checking file src/backend/executor/execExpr.c
Hunk #1 succeeded at 48 with fuzz 2 (offset -1 lines).
Hunk #2 succeeded at 88 (offset -1 lines).
Hunk #3 succeeded at 2419 (offset -1 lines).
Hunk #4 succeeded at 4195 (offset -1 lines).
checking file src/backend/executor/execExprInterp.c
Hunk #1 succeeded at 72 (offset -1 lines).
Hunk #2 succeeded at 180 (offset -1 lines).
Hunk #3 succeeded at 485 (offset -1 lines).
Hunk #4 succeeded at 1560 (offset -1 lines).
Hunk #5 succeeded at 4242 (offset -1 lines).
checking file src/backend/jit/llvm/llvmjit_expr.c
checking file src/backend/jit/llvm/llvmjit_types.c
checking file src/backend/nodes/makefuncs.c
Hunk #1 succeeded at 856 (offset -1 lines).
checking file src/backend/nodes/nodeFuncs.c
Hunk #1 succeeded at 233 (offset -1 lines).
Hunk #2 succeeded at 517 (offset -1 lines).
Hunk #3 succeeded at 1019 (offset -1 lines).
Hunk #4 succeeded at 1276 (offset -1 lines).
Hunk #5 succeeded at 1617 (offset -1 lines).
Hunk #6 succeeded at 2381 (offset -1 lines).
Hunk #7 succeeded at 3429 (offset -1 lines).
Hunk #8 succeeded at 4164 (offset -1 lines).
checking file src/backend/optimizer/path/costsize.c
Hunk #1 succeeded at 4878 (offset -1 lines).
checking file src/backend/optimizer/util/clauses.c
Hunk #1 succeeded at 50 (offset -3 lines).
Hunk #2 succeeded at 415 (offset -3 lines).
checking file src/backend/parser/gram.y
checking file src/backend/parser/parse_expr.c
checking file src/backend/parser/parse_target.c
Hunk #1 succeeded at 1988 (offset -1 lines).
checking file src/backend/utils/adt/formatting.c
Hunk #1 succeeded at 4465 (offset -1 lines).
checking file src/backend/utils/adt/jsonb.c
Hunk #1 succeeded at 2159 (offset -4 lines).
checking file src/backend/utils/adt/jsonfuncs.c
checking file src/backend/utils/adt/jsonpath.c
Hunk #1 FAILED at 68.
Hunk #2 succeeded at 1239 (offset -1 lines).
1 out of 2 hunks FAILED
checking file src/backend/utils/adt/jsonpath_exec.c
Hunk #1 succeeded at 229 (offset -5 lines).
Hunk #2 succeeded at 2866 (offset -5 lines).
Hunk #3 succeeded at 3751 (offset -5 lines).
checking file src/backend/utils/adt/ruleutils.c
Hunk #1 succeeded at 474 (offset -1 lines).
Hunk #2 succeeded at 518 (offset -1 lines).
Hunk #3 succeeded at 8303 (offset -1 lines).
Hunk #4 succeeded at 8475 (offset -1 lines).
Hunk #5 succeeded at 8591 (offset -1 lines).
Hunk #6 succeeded at 9808 (offset -1 lines).
Hunk #7 succeeded at 9858 (offset -1 lines).
Hunk #8 succeeded at 10039 (offset -1 lines).
Hunk #9 succeeded at 10909 (offset -1 lines).
checking file src/include/executor/execExpr.h
checking file src/include/nodes/execnodes.h
checking file src/include/nodes/makefuncs.h
checking file src/include/nodes/parsenodes.h
checking file src/include/nodes/primnodes.h
checking file src/include/parser/kwlist.h
checking file src/include/utils/formatting.h
checking file src/include/utils/jsonb.h
checking file src/include/utils/jsonfuncs.h
checking file src/include/utils/jsonpath.h
checking file src/interfaces/ecpg/preproc/ecpg.trailer
checking file src/test/regress/expected/sqljson_queryfuncs.out
checking file src/test/regress/parallel_schedule
checking file src/test/regress/sql/sqljson_queryfuncs.sql
checking file src/tools/pgindent/typedefs.list