Subject: Re: remaining sql/json patches

From: Alvaro Herrera <[email-masked]>
Date: Mon, 4 Mar 2024 16:03:34 +0100

On 2024-Mar-04, Erik Rijkers wrote:

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

It's because of dbbca2cf299b.  It should apply cleanly if you do "git
checkout dbbca2cf299b^" first ...  That commit is so recent that
evidently the cfbot hasn't had a chance to try this patch again since it
went in, which is why it's still green.

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
"If you have nothing to say, maybe you need just the right tool to help you
not say it."                   (New York Times, about Microsoft PowerPoint)

---

From: Amit Langote <[email-masked]>
Date: Tue, 5 Mar 2024 10:21:54 +0900

> went in, which is why it's still green.

Thanks for the heads up.  Attaching rebased patches.

-- 
Thanks, Amit Langote

---

From: Andy Fan <[email-masked]>
Date: Tue, 05 Mar 2024 12:28:32 +0800

Hi,

> Thanks for the heads up.  Attaching rebased patches.

In the commit message of 0001, we have:

"""
Both JSON_VALUE() and JSON_QUERY() functions have options for
handling EMPTY and ERROR conditions, which can be used to specify
the behavior when no values are matched and when an error occurs
during evaluation, respectively.

All of these functions only operate on jsonb values. The workaround
for now is to cast the argument to jsonb.
"""

which is not clear for me why we introduce JSON_VALUE() function, is it
for handling EMPTY or ERROR conditions? I think the existing cast
workaround have a similar capacity?

Then I think if it is introduced as a performance improvement like [1],
then the test at [1] might be interesting. If this is the case, the
method in [1] can avoid the user to modify these queries for the using
the new function. 

[1] https://www.postgresql.org/message-id/[email-masked]

-- 
Best Regards
Andy Fan

---

From: jian he <[email-masked]>
Date: Tue, 5 Mar 2024 12:42:17 +0800

> Thanks for the heads up.  Attaching rebased patches.

Walking through the v41-0001-Add-SQL-JSON-query-functions.patch documentation.
I found some minor cosmetic issues.

+       <para>
+        <literal>select json_query(jsonb '{"a": "[1, 2]"}', 'lax $.a'
RETURNING int[] OMIT QUOTES);</literal>
+        <returnvalue></returnvalue>
+       </para>
this example is not so good, it returns NULL, makes it harder to
render the result.

+   <replaceable>context_item</replaceable> (the document); seen
+   <xref linkend="functions-sqljson-path"/> for more details on what
+   <replaceable>path_expression</replaceable> can contain.
"seen" should be "see"?

+       <para>
+        This function must return a JSON string, so if the path expression
+        returns multiple SQL/JSON items, you must wrap the result using the
+        <literal>WITH WRAPPER</literal> clause.  If the wrapper is
"must" may be not correct?
since we have a RETURNING clause.
"generally" may be more accurate, I think.
maybe we can rephrase the sentence:
+        This function generally return a JSON string, so if the path expression
+        yield multiple SQL/JSON items, you must wrap the result using the
+        <literal>WITH WRAPPER</literal> clause

+        is spcified, the returned value will be of type <type>text</type>.
+        If no <literal>RETURNING</literal> is spcified, the returned value will
two typos, and should be "specified".

+        Note that if the <replaceable>path_expression</replaceable>
+        is <literal>strict</literal> and <literal>ON ERROR</literal> behavior
+        is <literal>ON ERROR</literal>, an error is generated if it yields no
+        items.

may be the following:
+        Note that if the <replaceable>path_expression</replaceable>
+        is <literal>strict</literal> and <literal>ON ERROR</literal> behavior
+        is <literal>ERROR</literal>, an error is generated if it yields no
+        items.

most of the place, you use
 <replaceable>path_expression</replaceable>
but there are two place you use:
<type>path_expression</type>
I guess that's ok, but the appearance is different.
 <replaceable> more prominent. Anyway, it is a minor issue.

+        <function>json_query</function>.  Note that scalar strings returned
+        by <function>json_value</function> always have their quotes removed,
+        equivalent to what one would get with <literal>OMIT QUOTES</literal>
+        when using <function>json_query</function>.

I think we can simplify it like the following:

+        <function>json_query</function>.  Note that scalar strings returned
+        by <function>json_value</function> always have their quotes removed,
+        equivalent to <literal>OMIT QUOTES</literal>
+        when using <function>json_query</function>.

---

From: Tomas Vondra <[email-masked]>
Date: Tue, 5 Mar 2024 22:30:20 +0100

Hi,

I know very little about sql/json and all the json internals, but I
decided to do some black box testing. I built a large JSONB table
(single column, ~7GB of data after loading). And then I did a query
transforming the data into tabular form using JSON_TABLE.

The JSON_TABLE query looks like this:

SELECT jt.* FROM
  title_jsonb t,
  json_table(t.info, '$'
    COLUMNS (
      "id" text path '$."id"',
      "type" text path '$."type"',
      "title" text path '$."title"',
      "original_title" text path '$."original_title"',
      "is_adult" text path '$."is_adult"',
      "start_year" text path '$."start_year"',
      "end_year" text path '$."end_year"',
      "minutes" text path '$."minutes"',
      "genres" text path '$."genres"',
      "aliases" text path '$."aliases"',
      "directors" text path '$."directors"',
      "writers" text path '$."writers"',
      "ratings" text path '$."ratings"',
      NESTED PATH '$."aliases"[*]'
        COLUMNS (
          "alias_title" text path '$."title"',
          "alias_region" text path '$."region"'
        ),
      NESTED PATH '$."directors"[*]'
        COLUMNS (
          "director_name" text path '$."name"',
          "director_birth_year" text path '$."birth_year"',
          "director_death_year" text path '$."death_year"'
        ),
      NESTED PATH '$."writers"[*]'
        COLUMNS (
          "writer_name" text path '$."name"',
          "writer_birth_year" text path '$."birth_year"',
          "writer_death_year" text path '$."death_year"'
        ),
      NESTED PATH '$."ratings"[*]'
        COLUMNS (
          "rating_average" text path '$."average"',
          "rating_votes" text path '$."votes"'
        )
    )
  ) as jt;

again, not particularly complex. But if I run this, it consumes multiple
gigabytes of memory, before it gets killed by OOM killer. This happens
even when ran using

  COPY (...) TO '/dev/null'

so there's nothing sent to the client. I did catch memory context info,
where it looks like this (complete stats attached):

------
TopMemoryContext: 97696 total in 5 blocks; 13056 free (11 chunks);
                  84640 used
  ...
  TopPortalContext: 8192 total in 1 blocks; 7680 free (0 chunks); ...
    PortalContext: 1024 total in 1 blocks; 560 free (0 chunks); ...
      ExecutorState: 2541764672 total in 314 blocks; 6528176 free
                     (1208 chunks); 2535236496 used
        printtup: 8192 total in 1 blocks; 7952 free (0 chunks); ...
        ...
...
Grand total: 2544132336 bytes in 528 blocks; 7484504 free
             (1340 chunks); 2536647832 used
------

I'd say 2.5GB in ExecutorState seems a bit excessive ... Seems there's
some memory management issue? My guess is we're not releasing memory
allocated while parsing the JSON or building JSON output.

I'm not attaching the data, but I can provide that if needed - it's
about 600MB compressed. The structure is not particularly complex, it's
movie info from [1] combined into a JSON document (one per movie).

regards

-- 
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

---

From: Amit Langote <[email-masked]>
Date: Wed, 6 Mar 2024 13:07:33 +0900

Hi Tomas,

> movie info from [1] combined into a JSON document (one per movie).

Thanks for the report.

Yeah, I'd like to see the data to try to drill down into what's piling
up in ExecutorState.  I want to be sure of if the 1st, query functions
patch, is not implicated in this, because I'd like to get that one out
of the way sooner than later.

-- 
Thanks, Amit Langote

---

From: Himanshu Upadhyaya <[email-masked]>
Date: Wed, 6 Mar 2024 17:28:06 +0530

On Tue, Mar 5, 2024 at 6:52 AM Amit Langote <[email-masked]> wrote:

Hi,

I am doing some random testing with the latest patch and found one scenario
that I wanted to share.
consider a below case.

‘postgres[102531]=#’SELECT * FROM JSON_TABLE(jsonb '{
         "id" : 12345678901,
         "FULL_NAME" : "JOHN DOE"}',
                '$'
                COLUMNS(
                     name varchar(20) PATH 'lax $.FULL_NAME',
                     id int PATH 'lax $.id'
      )
   )
;
ERROR:  22003: integer out of range
LOCATION:  numeric_int4_opt_error, numeric.c:4385
‘postgres[102531]=#’SELECT * FROM JSON_TABLE(jsonb '{
         "id" : "12345678901",
         "FULL_NAME" : "JOHN DOE"}',
                '$'
                COLUMNS(
                     name varchar(20) PATH 'lax $.FULL_NAME',
                     id int PATH 'lax $.id'
      )
   )
;
   name   | id
----------+----
 JOHN DOE |
(1 row)

The first query throws an error that the integer is "out of range" and is
quite expected but in the second case(when the value is enclosed with ") it
is able to process the JSON object but does not return any relevant
error(in fact processes the JSON but returns it with empty data for "id"
field). I think second query should fail with a similar error.

-- 
Regards,
Himanshu Upadhyaya

---

From: jian he <[email-masked]>
Date: Wed, 6 Mar 2024 21:22:12 +0800

> of the way sooner than later.

I did some tests. it generally looks like:

create or replace function random_text() returns text
as $$select string_agg(md5(random()::text),'') from
generate_Series(1,8) s $$ LANGUAGE SQL;
DROP TABLE if exists s;
create table s(a jsonb);
INSERT INTO s SELECT (
'{"id": "' || random_text() || '",'
'"type": "' || random_text() || '",'
'"title": "' || random_text() || '",'
'"original_title": "' || random_text() || '",'
'"is_adult": "' || random_text() || '",'
'"start_year": "' || random_text() || '",'
'"end_year": "' || random_text() || '",'
'"minutes": "' || random_text() || '",'
'"genres": "' || random_text() || '",'
'"aliases": "' || random_text() || '",'
'"genres": "' || random_text() || '",'
'"directors": "' || random_text() || '",'
'"writers": "' || random_text() || '",'
'"ratings": "' || random_text() || '",'
'"director_name": "' || random_text() || '",'
'"alias_title": "' || random_text() || '",'
'"alias_region": "' || random_text() || '",'
'"director_birth_year": "' || random_text() || '",'
'"director_death_year": "' || random_text() || '",'
'"rating_average": "' || random_text() || '",'
'"rating_votes": "' || random_text() || '"'
||'}' )::jsonb
FROM generate_series(1, 1e6);
SELECT pg_size_pretty(pg_table_size('s')); -- 5975 MB

It's less complex than Tomas's version.

attached, 3 test files:
1e5 rows, each key's value is small.  Total table size is 598 MB.
1e6 rows, each key's value is small. Total table size is 5975 MB.
27 rows, total table size is 5066 MB.
The test file's comment is the output I extracted using
pg_log_backend_memory_contexts,
mainly ExecutorState and surrounding big number memory context.

Conclusion, I come from the test:
if each json is big (5066 MB/27) , then it will take a lot of memory.
if each json is small(here is 256 byte), then it won't take a lot of
memory to process.

Another case, I did test yet: more keys in a single json, but the
value is small.

---

From: Tomas Vondra <[email-masked]>
Date: Wed, 6 Mar 2024 16:34:41 +0100

>                      name varchar(20) PATH 'lax $.FULL_NAME',

I'm pretty sure this is the correct & expected behavior. The second
query treats the value as string (because that's what should happen for
values in double quotes).

regards

---

From: jian he <[email-masked]>
Date: Thu, 7 Mar 2024 10:50:09 +0800

> Another case, I did test yet: more keys in a single json, but the

Another case attached. see the attached SQL file's comments.
a single simple jsonb, with 33 keys, each key's value with fixed length: 256.
total table size: SELECT pg_size_pretty(pg_table_size('json33keys'));  --5369 MB
number of rows: 600001.

using the previously mentioned method: pg_log_backend_memory_contexts.
all these tests under:
-Dcassert=true \
-Db_coverage=true \
-Dbuildtype=debug \

I hope someone will tell me if the test method is correct or not.

---

From: jian he <[email-masked]>
Date: Thu, 7 Mar 2024 12:08:21 +0800

> workaround have a similar capacity?

I guess because it's in the standard.
but I don't see individual sql standard Identifier, JSON_VALUE in
sql_features.txt
I do see JSON_QUERY.
mysql also have JSON_VALUE, [1]

EMPTY, ERROR: there is a standard Identifier: T825: SQL/JSON: ON EMPTY
and ON ERROR clauses

[1] https://dev.mysql.com/doc/refman/8.0/en/json-search-functions.html#function_json-value

---

From: jian he <[email-masked]>
Date: Thu, 7 Mar 2024 13:16:49 +0800

two cosmetic minor issues.

+/*
+ * JsonCoercion
+ * Information about coercing a SQL/JSON value to the specified
+ * type at runtime
+ *
+ * A node of this type is created if the parser cannot find a cast expression
+ * using coerce_type() or OMIT QUOTES is specified for JSON_QUERY.  If the
+ * latter, 'expr' may contain the cast expression; if not, the quote-stripped
+ * scalar string will be coerced by calling the target type's input function.
+ * See ExecEvalJsonCoercion.
+ */
+typedef struct JsonCoercion
+{
+ NodeTag type;
+
+ Oid targettype;
+ int32 targettypmod;
+ bool omit_quotes; /* OMIT QUOTES specified for JSON_QUERY? */
+ Node   *cast_expr; /* coercion cast expression or NULL */
+ Oid collation;
+} JsonCoercion;

comment:  'expr' may contain the cast expression;
here "exr" should be "cast_expr"?
"a SQL/JSON" should be " an SQL/JSON"?

---

From: Himanshu Upadhyaya <[email-masked]>
Date: Thu, 7 Mar 2024 10:48:02 +0530

On Wed, Mar 6, 2024 at 9:04 PM Tomas Vondra <[email-masked]>
wrote:

>  ok, Then why does the below query provide the correct conversion, even if
we enclose that in double quotes?
‘postgres[102531]=#’SELECT * FROM JSON_TABLE(jsonb '{
         "id" : "1234567890",
         "FULL_NAME" : "JOHN DOE"}',
                '$'
                COLUMNS(
                     name varchar(20) PATH 'lax $.FULL_NAME',
                     id int PATH 'lax $.id'
      )
   )
;
   name   |     id
----------+------------
 JOHN DOE | 1234567890
(1 row)

and for bigger input(string) it will leave as empty as below.
‘postgres[102531]=#’SELECT * FROM JSON_TABLE(jsonb '{
         "id" : "12345678901",
         "FULL_NAME" : "JOHN DOE"}',
                '$'
                COLUMNS(
                     name varchar(20) PATH 'lax $.FULL_NAME',
                     id int PATH 'lax $.id'
      )
   )
;
   name   | id
----------+----
 JOHN DOE |
(1 row)

seems it is not something to do with data enclosed in double quotes but
somehow related with internal casting it to integer and I think in case of
bigger input it is not able to cast it to integer(as defined under COLUMNS
as id int PATH 'lax $.id')

if it is not able to represent it to integer because of bigger input, it
should error out with a similar error message instead of leaving it empty.

Thoughts?

---

From: Amit Langote <[email-masked]>
Date: Thu, 7 Mar 2024 16:26:08 +0900

> of the way sooner than later.

I tracked this memory-hogging down to a bug in the query functions
patch (0001) after all.  The problem was with a query-lifetime cache
variable that was never set to point to the allocated memory.  So a
struct was allocated and then not freed for every row where it should
have only been allocated once.

I've fixed that bug in the attached.  I've also addressed some of
Jian's comments and made quite a few cleanups of my own.

Now I'll go look if Himanshu's concerns are a blocker for committing 0001. ;)

--
Thanks, Amit Langote

---

From: Tomas Vondra <[email-masked]>
Date: Thu, 7 Mar 2024 12:13:03 +0100

>                      name varchar(20) PATH 'lax $.FULL_NAME',

Ah, I see! Yes, that's a bit weird. Put slightly differently:

test=# SELECT * FROM JSON_TABLE(jsonb '{"id" : "2000000000"}',
                '$' COLUMNS(id int PATH '$.id'));
     id
------------
 2000000000
(1 row)

Time: 0.248 ms
test=# SELECT * FROM JSON_TABLE(jsonb '{"id" : "3000000000"}',
                '$' COLUMNS(id int PATH '$.id'));
 id
----

(1 row)

Clearly, when converting the string literal into int value, there's some
sort of error handling that realizes 3B overflows, and returns NULL
instead. I'm not sure if this is intentional.

regards

---

From: Amit Langote <[email-masked]>
Date: Thu, 7 Mar 2024 21:06:08 +0900

> On 3/7/24 06:18, Himanshu Upadhyaya wrote:

Thanks Himanshu for the testing.

>                 '$' COLUMNS(id int PATH '$.id'));

Indeed.

This boils down to the difference in the cast expression chosen to
convert the source value to int in the two cases.

The case where the source value has no quotes, the chosen cast
expression is a FuncExpr for function numeric_int4(), which has no way
to suppress errors.  When the source value has quotes, the cast
expression is a CoerceViaIO expression, which can suppress the error.
The default behavior is to suppress the error and return NULL, so the
correct behavior is when the source value has quotes.

I think we'll need either:

* fix the code in 0001 to avoid getting numeric_int4() in this case,
and generally cast functions that don't have soft-error handling
support, in favor of using IO coercion.
* fix FuncExpr (like CoerceViaIO) to respect SQL/JSON's request to
suppress errors and fix downstream functions like numeric_int4() to
comply by handling errors softly.

I'm inclined to go with the 1st option as we already have the
infrastructure in place -- input functions can all handle errors
softly.

For the latter, it uses numeric_int4() which doesn't support
soft-error handling, so throws the error.  With quotes, the

--
Thanks, Amit Langote

--
Thanks, Amit Langote

---

From: jian he <[email-masked]>
Date: Thu, 7 Mar 2024 21:46:40 +0800

> infrastructure in place -- input functions can all handle errors

not sure this is the right way.
but attached patches solved this problem.

Also, can you share the previous memory-hogging bug issue
when you are free, I want to know which part I am missing.....

---

From: Tomas Vondra <[email-masked]>
Date: Thu, 7 Mar 2024 15:04:07 +0100

Hi,

I was experimenting with the v42 patches, and I think the handling of ON
EMPTY / ON ERROR clauses may need some improvement. The grammar is
currently defined like this:

    | json_behavior ON EMPTY_P json_behavior ON ERROR_P

This means the clauses have to be defined exactly in this order, and if
someone does

    NULL ON ERROR NULL ON EMPTY

it results in syntax error. I'm not sure what the SQL standard says
about this, but it seems other databases don't agree on the order. Is
there a particular reason to not allow both orderings?

regards

---

From: Amit Langote <[email-masked]>
Date: Thu, 7 Mar 2024 23:11:03 +0900

> when you are free, I want to know which part I am missing.....

Take a look at the json_populate_type() call in ExecEvalJsonCoercion() or
specifically compare the new way of passing its void *cache parameter with
the earlier patches.

---

From: Alvaro Herrera <[email-masked]>
Date: Thu, 7 Mar 2024 15:14:15 +0100

On 2024-Mar-07, Tomas Vondra wrote:

> EMPTY / ON ERROR clauses may need some improvement.

Well, the 2023 standard says things like

<JSON value function> ::=
  JSON_VALUE <left paren>
      <JSON API common syntax>
      [ <JSON returning clause> ]
      [ <JSON value empty behavior> ON EMPTY ]
      [ <JSON value error behavior> ON ERROR ]
  <right paren>

which implies that if you specify it the other way around, it's a syntax
error.

> I'm not sure what the SQL standard says about this, but it seems other
> databases don't agree on the order. Is there a particular reason to
> not allow both orderings?

I vaguely recall that trying to also support the other ordering leads to
having more rules.  Now maybe we do want that because of compatibility
with other DBMSs, but frankly at this stage I wouldn't bother.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"I am amazed at [the pgsql-sql] mailing list for the wonderful support, and
lack of hesitasion in answering a lost soul's question, I just wished the rest
of the mailing list could be like this."                               (Fotis)
              https://postgr.es/m/[email-masked]

---

From: Amit Langote <[email-masked]>
Date: Tue, 19 Mar 2024 19:45:43 +0900

Hi,

> infrastructure in place -- input functions can all handle errors

I've adjusted the coercion-handling code to deal with this and similar
cases to use coercion by calling the target type's input function in
more cases.  The resulting refactoring allowed me to drop a bunch of
code and node structs, notably, the JsonCoercion and JsonItemCoercion
nodes.  Going with input function based coercion as opposed to using
casts means the coercion may fail in more cases than before but I
think that's acceptable.  For example, the following case did not fail
before because they'd use numeric_int() cast function to convert 1.234
to an integer:

select json_value('{"a": 1.234}', '$.a' returning int error on error);
ERROR:  invalid input syntax for type integer: "1.234"

It is same error as this case, where the source numerical value is
specified as a string:

select json_value('{"a": "1.234"}', '$.a' returning int error on error);
ERROR:  invalid input syntax for type integer: "1.234"

I had hoped to get rid of all instances of using casts and standardize
on coercion at runtime using input functions and json_populate_type(),
but there are a few cases where casts produce saner results and also
harmless (error-safe), such as cases where the target types are
domains or are types with typmod.

I've also tried to address most of Jian He's comments and a bunch of
cleanups of my own.  Attaching 0002 as the delta over v42 containing
all of those changes.

I intend to commit 0001+0002 after a bit more polishing.

-- 
Thanks, Amit Langote

---

From: jian he <[email-masked]>
Date: Wed, 20 Mar 2024 11:41:42 +0800

> I intend to commit 0001+0002 after a bit more polishing.

V43 is far more intuitive! thanks!

if (isnull ||
(exprType(expr) == JSONBOID &&
btype == default_behavior))
coerce = true;
else
coerced_expr =
coerce_to_target_type(pstate, expr, exprType(expr),
  returning->typid, returning->typmod,
  COERCION_EXPLICIT, COERCE_EXPLICIT_CAST,
  exprLocation((Node *) behavior));

obviously, there are cases where "coerce" is false, and "coerced_expr"
is not null.
so I think the bool "coerce" variable naming is not very intuitive.
maybe we can add some comments or change to a better name.

JsonPathVariableEvalContext
JsonPathVarCallback
JsonItemType
JsonExprPostEvalState
these should remove from src/tools/pgindent/typedefs.list

+/*
+ * Performs JsonPath{Exists|Query|Value}() for a given context_item and
+ * jsonpath.
+ *
+ * Result is set in *op->resvalue and *op->resnull.  Return value is the
+ * step address to be performed next.
+ *
+ * On return, JsonExprPostEvalState is populated with the following details:
+ * - error.value: true if an error occurred during JsonPath evaluation
+ * - empty.value: true if JsonPath{Query|Value}() found no matching item
+ *
+ * No return if the ON ERROR/EMPTY behavior is ERROR.
+ */
+int
+ExecEvalJsonExprPath(ExprState *state, ExprEvalStep *op,
+ ExprContext *econtext)

" No return if the ON ERROR/EMPTY behavior is ERROR."  is wrong?
counter example:
SELECT JSON_QUERY(jsonb '{"a":[12,2]}', '$.a' RETURNING int4RANGE omit
quotes error on error);
also "JsonExprPostEvalState" does not exist any more.
overall feel like ExecEvalJsonExprPath comments need to be rephrased.

---

From: jian he <[email-masked]>
Date: Wed, 20 Mar 2024 14:51:48 +0800

minor issues I found while looking through it.
other than these issues, looks good!

/*
 * Convert the a given JsonbValue to its C string representation
 *
 * Returns the string as a Datum setting *resnull if the JsonbValue is a
 * a jbvNull.
 */
static char *
ExecGetJsonValueItemString(JsonbValue *item, bool *resnull)
{
}
I think the comments are not right?

/*
 * Checks if the coercion evaluation led to an error.  If an error did occur,
 * this sets post_eval->error to trigger the ON ERROR handling steps.
 */
void
ExecEvalJsonCoercionFinish(ExprState *state, ExprEvalStep *op)
{
}
these comments on ExecEvalJsonCoercionFinish also need to be updated?

+ /*
+ * Coerce the result value by calling the input function coercion.
+ * *op->resvalue must point to C string in this case.
+ */
+ if (!*op->resnull && jsexpr->use_io_coercion)
+ {
+ FunctionCallInfo fcinfo;
+
+ fcinfo = jsestate->input_fcinfo;
+ Assert(fcinfo != NULL);
+ Assert(val_string != NULL);
+ fcinfo->args[0].value = PointerGetDatum(val_string);
+ fcinfo->args[0].isnull = *op->resnull;
+ /* second and third arguments are already set up */
+
+ fcinfo->isnull = false;
+ *op->resvalue = FunctionCallInvoke(fcinfo);
+ if (SOFT_ERROR_OCCURRED(&jsestate->escontext))
+ error = true;
+
+ jump_eval_coercion = -1;
+ }

+ /* second and third arguments are already set up */
change to
/* second and third arguments are already set up in ExecInitJsonExpr */
would be great.

commit message
<<<<
<<<<
should be removed?

+ case T_JsonFuncExpr:
+ {
+ JsonFuncExpr *jfe = (JsonFuncExpr *) node;
+
+ if (WALK(jfe->context_item))
+ return true;
+ if (WALK(jfe->pathspec))
+ return true;
+ if (WALK(jfe->passing))
+ return true;
+ if (jfe->output && WALK(jfe->output))
+ return true;
+ if (jfe->on_empty)
+ return true;
+ if (jfe->on_error)
+ return true;
+ }

+ if (jfe->output && WALK(jfe->output))
+ return true;
can be simplified:

+ if (WALK(jfe->output))
+ return true;