Обсуждение: Re: SQL:2023 JSON simplified accessor support

On 29.08.24 18:33, Alexandra Wang wrote:
> I’ve implemented the member and array accessors and attached two
> alternative patches:
> 
> 1. v1-0001-Add-JSON-JSONB-simplified-accessor.patch: This patch
> enables dot access to JSON object fields and subscript access to
> indexed JSON array elements by converting "." and "[]" indirection
> into a JSON_QUERY JsonFuncExpr node.
> 
> 2. v2-0001-Transform-JSON-dot-access-to-arrow-operator.txt: This
> alternative patch implements dot access to JSON object fields by
> transforming the "." indirection into a "->" operator.
> 
> The upside of the v1 patch is that it strictly aligns with the SQL
> standard, which specifies that the simplified access is equivalent to:
> 
> JSON_QUERY (VEP, 'lax $.JC' WITH CONDITIONAL ARRAY WRAPPER NULL ON
> EMPTY NULL ON ERROR)
> 
> However, the performance of JSON_QUERY might be suboptimal due to
> function call overhead. Therefore, I implemented the v2 alternative
> using the "->" operator.

Using the operator approach would also allow taking advantage of 
optimizations such as 

<https://www.postgresql.org/message-id/flat/CAKU4AWoqAVya6PBhn%2BBCbFaBMt3z-2%3Di5fKO3bW%3D6HPhbid2Dw%40mail.gmail.com>.

> There is some uncertainty about the semantics of conditional array
> wrappers. Currently, there is at least one subtle difference between
> the "->" operator and JSON_QUERY, as shown:

That JSON_QUERY bug has been fixed.

I suggest you rebase both of your patches over this, just to double 
check everything.  But then I think you can drop the v1 patch and just 
submit a new version of v2.

The patch should eventually contain some documentation.  It might be 
good starting to look for a good spot where to put that documentation. 
It might be either near the json types documentation or near the general 
qualified identifier syntax, not sure.

On 2024-09-26 Th 11:45 AM, Alexandra Wang wrote:
> Hi,
>
> I didn’t run pgindent earlier, so here’s the updated version with the
> correct indentation. Hope this helps!


This is a really nice feature, and provides a lot of expressive power 
for such a small piece of code.

I notice this doesn't seem to work for domains over json and jsonb.


andrew@~=# create domain json_d as json;
CREATE DOMAIN
andrew@~=# create table test_json_dot(id int, test_json json_d);
CREATE TABLE
andrew@~=# insert into test_json_dot select 1, '{"a": 1, "b": 42}'::json;
INSERT 0 1      |          |
andrew@~=# select (test_json_dot.test_json).b, json_query(test_json, 
'lax $.b' WITH CONDITIONAL WRAPPER NULL ON EMPTY NULL ON ERROR) as 
expected from test_json_dot;
ERROR:  column notation .b applied to type json_d, which is not a 
composite type
LINE 1: select (test_json_dot.test_json).b, json_query(test_json, 'l...


I'm not sure that's a terribly important use case, but we should 
probably make it work. If it's a domain we should get the basetype of 
the domain. There's some example code in src/backend/utils/adt/jsonfuncs.c


cheers


andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

On Sep 26, 2024, at 16:45, Alexandra Wang <alexandra.wang.oss@gmail.com> wrote:

> I didn’t run pgindent earlier, so here’s the updated version with the
> correct indentation. Hope this helps!

Oh,  nice! I don’t suppose the standard also has defined an operator equivalent to ->>, though, has it? I tend to want
thetext output far more often than a JSON scalar. 

Best,

David

On 2024-09-27 Fr 5:49 AM, David E. Wheeler wrote:
> On Sep 26, 2024, at 16:45, Alexandra Wang <alexandra.wang.oss@gmail.com> wrote:
>
>> I didn’t run pgindent earlier, so here’s the updated version with the
>> correct indentation. Hope this helps!
> Oh,  nice! I don’t suppose the standard also has defined an operator equivalent to ->>, though, has it? I tend to
wantthe text output far more often than a JSON scalar.
 
>

That would defeat being able to chain these.


cheers


andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

On Sep 27, 2024, at 12:07, Andrew Dunstan <andrew@dunslane.net> wrote:

> That would defeat being able to chain these.

Not if it’s a different operator. But I’m fine to just keep using ->> at the end of a chain.

D

On 07.11.24 22:57, Alexandra Wang wrote:
> The v5 patch includes the following updates:
> 
> - Fixed the aforementioned issue and added more tests covering composite
> types with domains, nested domains, and arrays of domains over
> JSON/JSONB.
> 
> - Refactored the logic for parsing JSON/JSONB object fields by moving it
> from ParseFuncOrColumn() to transformIndirection() for improved
> readability. The ParseFuncOrColumn() function is already handling both
> single-argument function calls and composite types, and it has other
> callers besides transformIndirection().

This patch implements array subscripting support for the json type, but 
it does it in a non-standard way, using 
ParseJsonSimplifiedAccessorArrayElement().  This would be better done by 
providing a typsubscript function for the json type.  This is what jsonb 
already has, which is why your patch doesn't need to provide the array 
support for jsonb.  I suggest you implement the typsubscript support for 
the json type (make it a separate patch but you can keep it in this 
thread IMO) and remove the custom code from this patch.

A few comments on the tests:  The tests look good to me.  Good coverage 
of weirdly nested types.  Results look correct.

+drop table if exists test_json_dot;

This can be omitted, since we know that the table doesn't exist yet.

This code could be written in the more conventional insert ... values 
syntax:

+insert into test_json_dot select 1, '{"a": 1, "b": 42}'::json;
+insert into test_json_dot select 1, '{"a": 2, "b": {"c": 42}}'::json;
+insert into test_json_dot select 1, '{"a": 3, "b": {"c": "42"}, 
"d":[11, 12]}'::json;
+insert into test_json_dot select 1, '{"a": 3, "b": {"c": "42"}, 
"d":[{"x": [11, 12]}, {"y": [21, 22]}]}'::json;

Then the ::json casts can also go away.

Also, using a different value for "id" for each row would be more
useful, so that the subsequent tests could then be written like

     select id, (test_jsonb_dot.test_jsonb).b from test_jsonb_dot;

so we can see which result corresponds to which input row.  Also make
id the primary key in this table.

Also, let's keep the json and the jsonb variants aligned.  There are
some small differences, like the test_json_dot table having 4 rows but
the test_jsonb_dot having 3 rows.  And the array and wildcard tests in
the opposite order.  Not a big deal, but keeping these the same helps
eyeballing the test files.

Maybe add a comment somewhere in this file that you are running the
json_query equivalents to cross-check the semantics of the dot syntax.

Some documentation should be written.  This looks like this right place 
to start:

https://www.postgresql.org/docs/devel/sql-expressions.html#FIELD-SELECTION

and them maybe some cross-linking between there and the sections on JSON 
types and operators.

Hi,

On Tue, Nov 19, 2024 at 6:06 PM Nikita Glukhov <glukhov.n.a@gmail.com> wrote:
>
> Hi, hackers.
>
> I have implemented dot notation for jsonb using type subscripting back
> in April 2023, but failed post it because I left Postgres Professional
> company soon after and have not worked anywhere since, not even had
> any interest in programming.
>
> But yesterday I accidentally decided to look what is going on at
> commitfests and found this thread.  I immediately started to rebase
> code from PG16, fixed some bugs, and now I'm ready to present my
> version of the patches which is much more complex.
>
> Unfortunately, I probably won't be able to devote that much time to
> the patches as before.

Thank you so much, Nikita, for revisiting this topic and sharing your
v6 patches!

Now that we have two solutions, I’d like to summarize our current
options.

In Postgres, there are currently three ways to access json/jsonb
object fields and array elements:

1. '->' operator (Postgres-specific, predates SQL standard):

postgres=# select ('{"a": 1, "b": "c", "d": [1, 2, 3]}'::json) -> 'd'
-> 0; -- returns 1

2. jsonb subscripting (not available for the plain json type):

postgres=# select ('{"a": 1, "b": "c", "d": [1, 2,
3]}'::jsonb)['d'][0];  --returns 1

3. json_query() function:

postgres=# select json_query(jsonb '{"a": 1, "b": "c", "d": [1, 2,
3]}', 'lax $.d[0]');  --returns 1

A few weeks ago, I did the following performance benchmarking of the
three approaches:

-- setup:
create table tbl(id int, col1 jsonb);
insert into tbl select i, '{"x":"vx", "y":[{"a":[1,2,3]}, {"b":[1, 2,
{"j":"vj"}]}]}' from generate_series(1, 100000)i;

-- jsonb_operator.sql
SELECT id, col1 -> 'y' -> 1 -> 'b' -> 2 -> 'j' AS jsonb_operator FROM tbl;

-- jsonb_subscripting.sql
SELECT id, col1['y'][1]['b'][2]['j'] AS jsonb_subscript FROM tbl;

-- jsonb_path_query.sql
SELECT id, jsonb_path_query(col1, '$.y[1].b[2].j') FROM tbl;

# pgbench on my local MacOS machine, using -O3 optimization:
pgbench -n -f XXX.sql postgres -T100

Results (Latency | tps):

"->" operator: 14ms | 68
jsonb subscripting: 17ms | 58
jsonb_path_query() function: 23ms | 43

So performance from best to worst:
"->" operator > jsonb subscripting >> jsonb_path_query() function.

I’m excited to see your implementation of dot notation for jsonb using
type subscripting! This approach rounds out the three possible ways to
implement JSON simplified accessors:

## v1: json_query() implementation

Pros:
- Fully adheres to the SQL standard.

According to the SQL standard, if the JSON simplified accessor <JA> is
not a JSON item method, it is equivalent to a <JSON query>:

JSON_QUERY ( VEP, 'lax $.JC' WITH CONDITIONAL ARRAY WRAPPER NULL ON
EMPTY NULL ON ERROR)

(I’m skipping <JA> that includes a JSON item method, as it is
currently outside the scope of both sets of patches.)

- Easiest to implement

Cons:
- Slow due to function call overhead.

## v2-v5: "->" operator implementation

We initially chose this approach for its performance benefits.
However, while addressing Peter’s feedback on v5, I encountered the
following issue:

-- setup
create table test_json_dot(id serial primary key, test_json json);
insert into test_json_dot values (5, '[{"a": 1, "b": 42}, {"a": 2,
"b": {"c": 42}}]');

-- problematic query:
test1=# select id, (test_json).b, json_query(test_json, 'lax $.b' WITH
CONDITIONAL WRAPPER NULL ON EMPTY NULL ON ERROR) as expected from
test_json_dot;
 id | b |    expected
----+---+-----------------
  5 |   | [42, {"c": 42}]
(1 row)

This issue arises from the semantic differences between the "->"
operator and json_query’s "lax" mode. One possible workaround is to
redefine the "->" operator and modify its implementation. However, since
the "->" operator has been in use for a long time, such changes would
break backward compatibility.

## v6: jsonb subscription implementation

Nikita's patches pass all my functional test cases, including those
that failed with the previous approach.

Supported formats:
- JSON member accessor
- JSON wildcard member accessor (Not available in v5, so this is also a plus)
- JSON array accessor

Questions:

1. Since Nikita’s patches did not address the JSON data type, and JSON
currently does not support subscripting, should we limit the initial
feature set to JSONB dot-notation for now? In other words, if we aim
to fully support JSON simplified accessors for the plain JSON type,
should we handle support for plain JSON subscripting as a follow-up
effort?

2. I have yet to have a more thorough review of Nikita’s patches.
One area I am not familiar with is the hstore-related changes. How
relevant is hstore to the JSON simplified accessor?

Best,
Alex

On 2024-11-21 Th 3:52 PM, Alexandra Wang wrote:
> Hi,
>
> On Tue, Nov 19, 2024 at 6:06 PM Nikita Glukhov <glukhov.n.a@gmail.com> wrote:
>> Hi, hackers.
>>
>> I have implemented dot notation for jsonb using type subscripting back
>> in April 2023, but failed post it because I left Postgres Professional
>> company soon after and have not worked anywhere since, not even had
>> any interest in programming.
>>
>> But yesterday I accidentally decided to look what is going on at
>> commitfests and found this thread.  I immediately started to rebase
>> code from PG16, fixed some bugs, and now I'm ready to present my
>> version of the patches which is much more complex.
>>
>> Unfortunately, I probably won't be able to devote that much time to
>> the patches as before.
> Thank you so much, Nikita, for revisiting this topic and sharing your
> v6 patches!
>
> Now that we have two solutions, I’d like to summarize our current
> options.
>
> In Postgres, there are currently three ways to access json/jsonb
> object fields and array elements:
>
> 1. '->' operator (Postgres-specific, predates SQL standard):
>
> postgres=# select ('{"a": 1, "b": "c", "d": [1, 2, 3]}'::json) -> 'd'
> -> 0; -- returns 1
>
> 2. jsonb subscripting (not available for the plain json type):
>
> postgres=# select ('{"a": 1, "b": "c", "d": [1, 2,
> 3]}'::jsonb)['d'][0];  --returns 1
>
> 3. json_query() function:
>
> postgres=# select json_query(jsonb '{"a": 1, "b": "c", "d": [1, 2,
> 3]}', 'lax $.d[0]');  --returns 1
>
> A few weeks ago, I did the following performance benchmarking of the
> three approaches:
>
> -- setup:
> create table tbl(id int, col1 jsonb);
> insert into tbl select i, '{"x":"vx", "y":[{"a":[1,2,3]}, {"b":[1, 2,
> {"j":"vj"}]}]}' from generate_series(1, 100000)i;
>
> -- jsonb_operator.sql
> SELECT id, col1 -> 'y' -> 1 -> 'b' -> 2 -> 'j' AS jsonb_operator FROM tbl;
>
> -- jsonb_subscripting.sql
> SELECT id, col1['y'][1]['b'][2]['j'] AS jsonb_subscript FROM tbl;
>
> -- jsonb_path_query.sql
> SELECT id, jsonb_path_query(col1, '$.y[1].b[2].j') FROM tbl;
>
> # pgbench on my local MacOS machine, using -O3 optimization:
> pgbench -n -f XXX.sql postgres -T100
>
> Results (Latency | tps):
>
> "->" operator: 14ms | 68
> jsonb subscripting: 17ms | 58
> jsonb_path_query() function: 23ms | 43
>
> So performance from best to worst:
> "->" operator > jsonb subscripting >> jsonb_path_query() function.
>
> I’m excited to see your implementation of dot notation for jsonb using
> type subscripting! This approach rounds out the three possible ways to
> implement JSON simplified accessors:
>
> ## v1: json_query() implementation
>
> Pros:
> - Fully adheres to the SQL standard.
>
> According to the SQL standard, if the JSON simplified accessor <JA> is
> not a JSON item method, it is equivalent to a <JSON query>:
>
> JSON_QUERY ( VEP, 'lax $.JC' WITH CONDITIONAL ARRAY WRAPPER NULL ON
> EMPTY NULL ON ERROR)
>
> (I’m skipping <JA> that includes a JSON item method, as it is
> currently outside the scope of both sets of patches.)
>
> - Easiest to implement
>
> Cons:
> - Slow due to function call overhead.
>
> ## v2-v5: "->" operator implementation
>
> We initially chose this approach for its performance benefits.
> However, while addressing Peter’s feedback on v5, I encountered the
> following issue:
>
> -- setup
> create table test_json_dot(id serial primary key, test_json json);
> insert into test_json_dot values (5, '[{"a": 1, "b": 42}, {"a": 2,
> "b": {"c": 42}}]');
>
> -- problematic query:
> test1=# select id, (test_json).b, json_query(test_json, 'lax $.b' WITH
> CONDITIONAL WRAPPER NULL ON EMPTY NULL ON ERROR) as expected from
> test_json_dot;
>   id | b |    expected
> ----+---+-----------------
>    5 |   | [42, {"c": 42}]
> (1 row)
>
> This issue arises from the semantic differences between the "->"
> operator and json_query’s "lax" mode. One possible workaround is to
> redefine the "->" operator and modify its implementation. However, since
> the "->" operator has been in use for a long time, such changes would
> break backward compatibility.
>
> ## v6: jsonb subscription implementation
>
> Nikita's patches pass all my functional test cases, including those
> that failed with the previous approach.
>
> Supported formats:
> - JSON member accessor
> - JSON wildcard member accessor (Not available in v5, so this is also a plus)
> - JSON array accessor
>
> Questions:
>
> 1. Since Nikita’s patches did not address the JSON data type, and JSON
> currently does not support subscripting, should we limit the initial
> feature set to JSONB dot-notation for now? In other words, if we aim
> to fully support JSON simplified accessors for the plain JSON type,
> should we handle support for plain JSON subscripting as a follow-up
> effort?
>
> 2. I have yet to have a more thorough review of Nikita’s patches.
> One area I am not familiar with is the hstore-related changes. How
> relevant is hstore to the JSON simplified accessor?
>

We can't change the way the "->" operator works, as there could well be 
uses of it in the field that rely on its current behaviour. But maybe we 
could invent a new operator which is compliant with the standard 
semantics for dot access, and call that. Then we'd get the best 
performance, and also we might be able to implement it for the plain 
JSON type. If that proves not possible we can think about not 
implementing for plain JSON, but I'd rather not go there until we have to.

I don't think we should be including hstore changes here - we should 
just be aiming at implementing the standard for JSON access. hstore 
changes if any should be a separate feature. The aren't relevant to JSON 
access, although they might use some of the same infrastructure, 
depending on implementation.


cheers


andrew


--
Andrew Dunstan
EDB: https://www.enterprisedb.com

On 21.11.24 23:46, Andrew Dunstan wrote:
>> Questions:
>>
>> 1. Since Nikita’s patches did not address the JSON data type, and JSON
>> currently does not support subscripting, should we limit the initial
>> feature set to JSONB dot-notation for now? In other words, if we aim
>> to fully support JSON simplified accessors for the plain JSON type,
>> should we handle support for plain JSON subscripting as a follow-up
>> effort?
>>
>> 2. I have yet to have a more thorough review of Nikita’s patches.
>> One area I am not familiar with is the hstore-related changes. How
>> relevant is hstore to the JSON simplified accessor?
>>
> 
> We can't change the way the "->" operator works, as there could well be 
> uses of it in the field that rely on its current behaviour. But maybe we 
> could invent a new operator which is compliant with the standard 
> semantics for dot access, and call that. Then we'd get the best 
> performance, and also we might be able to implement it for the plain 
> JSON type. If that proves not possible we can think about not 
> implementing for plain JSON, but I'd rather not go there until we have to.

Yes, I think writing a custom operator that is similar to "->" but has 
the required semantics is the best way forward.  (Maybe it can be just a 
function?)

> I don't think we should be including hstore changes here - we should 
> just be aiming at implementing the standard for JSON access. hstore 
> changes if any should be a separate feature. The aren't relevant to JSON 
> access, although they might use some of the same infrastructure, 
> depending on implementation.

In a future version, the operator/function mentioned above could be a 
catalogued property of a type, similar to typsubscript.  Then you could 
also apply this to other types.  But let's leave that for later.

If I understand it correctly, Nikita's patch uses the typsubscript 
support function to handle both bracket subscripting and dot notation. 
I'm not sure if it's right to mix these two together.  Maybe I didn't 
understand that correctly.

Hi Alex,

The code comments and the commit messages help a lot when reviewing! Thanks for
the new version.

The code LGTM and check-world is happy. I've also performed some tests and
everything looks good!

Just some minor points about this new version:

## v9-0005

Typo on commit message title

## v9-0006

> + * The following functions create various types of JsonPathParseItem nodes,
> + * which are used to build JsonPath expressions for jsonb simplified accessor.
>
Just to avoid misinterpretation I think that we can replace "The following
functions" with "The make_jsonpath_item_* functions" since we can have more
functions in the future that are not fully related with these. Does that make
sense?

-- 
Matheus Alcantara

On Mon, Mar 3, 2025 at 4:16 PM Matheus Alcantara
<matheusssilv97@gmail.com> wrote:
>
> Hi Alex,
>
> The code comments and the commit messages help a lot when reviewing! Thanks for
> the new version.
>
> The code LGTM and check-world is happy. I've also performed some tests and
> everything looks good!
>
> Just some minor points about this new version:
>
> ## v9-0005
>
> Typo on commit message title
>
> ## v9-0006
>
> > + * The following functions create various types of JsonPathParseItem nodes,
> > + * which are used to build JsonPath expressions for jsonb simplified accessor.
> >
> Just to avoid misinterpretation I think that we can replace "The following
> functions" with "The make_jsonpath_item_* functions" since we can have more
> functions in the future that are not fully related with these. Does that make
> sense?
>

Sorry, I've forgotten to include a question. Do you have anything in mind about
documentation changes for this patch?

--
Matheus Alcantara

On Tue, Mar 4, 2025 at 6:05 AM Mark Dilger <mark.dilger@enterprisedb.com> wrote:

But then I tried:

+DO $$
+DECLARE
+  a jsonb := '{"": 6, "NU": [{"": [[3]]}, [6], [2], "bCi"], "aaf": [-6, -8]}'::jsonb;
+BEGIN
+  WHILE a IS NOT NULL
+  LOOP
+    RAISE NOTICE '%', a;
+    a := COALESCE(a."NU", a[2]);
+  END LOOP;
+END
+$$ LANGUAGE plpgsql;
+NOTICE:  {"": 6, "NU": [{"": [[3]]}, [6], [2], "bCi"], "aaf": [-6, -8]}
+ERROR:  missing FROM-clause entry for table "a"
+LINE 1: a := COALESCE(a."NU", a[2])
+                      ^
+QUERY:  a := COALESCE(a."NU", a[2])
+CONTEXT:  PL/pgSQL function inline_code_block line 8 at assignment

which suggests the plpgsql parser does not recognize a."NU" as we'd expect.  Any thoughts on this?

I should mention that

+DO $$

+DECLARE
+  a jsonb := '{"": 6, "NU": [{"": [[3]]}, [6], [2], "bCi"], "aaf": [-6, -8]}'::jsonb;
+BEGIN
+  WHILE a IS NOT NULL
+  LOOP
+    RAISE NOTICE '%', a;
+    a := COALESCE((a)."NU", (a)[2]);
+  END LOOP;
+END
+$$ LANGUAGE plpgsql;
+NOTICE:  {"": 6, "NU": [{"": [[3]]}, [6], [2], "bCi"], "aaf": [-6, -8]}
+NOTICE:  [{"": [[3]]}, [6], [2], "bCi"]
+NOTICE:  [2]

 

works fine.  I guess that is good enough.  Should we add these to the sql/jsonb.sql to document the expected behavior, both with the error when using plain "a" and with the correct output when using "(a)"?  The reason I mention this is that the plpgsql parser might get changed at some point, and without a test case, we might not notice if this breaks.

Yes, I think so.

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

This patch set has expanded significantly in scope recently, which is 
probably the right thing, but that means there won't be enough time to 
review and finish it for PG18.  So I'm moving this to the next 
commitfest now.

On 13.03.25 15:02, Alexandra Wang wrote:
> Hi Mark,
> 
> Thank you so much for reviewing! I have attached the new patches.
> 
> On Tue, Mar 4, 2025 at 8:05 AM Mark Dilger <mark.dilger@enterprisedb.com 
> <mailto:mark.dilger@enterprisedb.com>> wrote:
> 
> 
>     On Mon, Mar 3, 2025 at 12:23 PM Alexandra Wang
>     <alexandra.wang.oss@gmail.com <mailto:alexandra.wang.oss@gmail.com>>
>     wrote:
> 
>           I've attached v10, which addresses your feedback.
> 
> 
>     Hi Alex!  Thanks for the patches.
> 
>     In src/test/regress/sql/jsonb.sql, the section marked with "--
>     slices are not supported" should be relabeled.  That comment
>     predates these patches, and is now misleading.
> 
>     A bit further down in expected/jsonb.out, there is an expected
>     failure, but no SQL comment to indicate that it is expected:
> 
>     +SELECT (t.jb).* FROM test_jsonb_dot_notation;
>     +ERROR:  missing FROM-clause entry for table "t"
>     +LINE 1: SELECT (t.jb).* FROM test_jsonb_dot_notation;
> 
>     Perhaps a "-- fails" comment would clarify?  Then, further down,
> 
> 
> Fixed.
> 
>     +SELECT (jb).a.**.x FROM test_jsonb_dot_notation; -- not supported
>     +ERROR:  syntax error at or near "**"
>     +LINE 1: SELECT (jb).a.**.x FROM test_jsonb_dot_notation;
> 
>     I wonder if it would be better to have the parser handle this case
>     and raise a ERRCODE_FEATURE_NOT_SUPPORTED instead?
> 
> 
> In 0008 I added a new token named "DOUBLE_ASTERISK" to the lexers to
> represent "**". Hope this helps!
> 
>     I got curious about the support for this new dot notation in the
>     plpgsql parser and tried:
> 
>     +DO $$
>     +DECLARE
>     +  a jsonb := '[1,2,3,4,5,6,7]'::jsonb;
>     +BEGIN
>     +  WHILE a IS NOT NULL
>     +  LOOP
>     +    RAISE NOTICE '%', a;
>     +    a := a[2:];
>     +  END LOOP;
>     +END
>     +$$ LANGUAGE plpgsql;
>     +NOTICE:  [1, 2, 3, 4, 5, 6, 7]
>     +NOTICE:  [3, 4, 5, 6, 7]
>     +NOTICE:  [5, 6, 7]
>     +NOTICE:  7
> 
>     which looks good!  But then I tried:
> 
>     +DO $$
>     +DECLARE
>     +  a jsonb := '{"": 6, "NU": [{"": [[3]]}, [6], [2], "bCi"], "aaf":
>     [-6, -8]}'::jsonb;
>     +BEGIN
>     +  WHILE a IS NOT NULL
>     +  LOOP
>     +    RAISE NOTICE '%', a;
>     +    a := COALESCE(a."NU", a[2]);
>     +  END LOOP;
>     +END
>     +$$ LANGUAGE plpgsql;
>     +NOTICE:  {"": 6, "NU": [{"": [[3]]}, [6], [2], "bCi"], "aaf": [-6, -8]}
>     +ERROR:  missing FROM-clause entry for table "a"
>     +LINE 1: a := COALESCE(a."NU", a[2])
>     +                      ^
>     +QUERY:  a := COALESCE(a."NU", a[2])
>     +CONTEXT:  PL/pgSQL function inline_code_block line 8 at assignment
> 
>     which suggests the plpgsql parser does not recognize a."NU" as we'd
>     expect.  Any thoughts on this?
> 
> 
> Thanks for the tests! I added them to the "jsonb" regress test.
> 
>     I notice there are no changes in src/interfaces/ecpg/test, which
>     concerns me.  The sqljson.pgc and sqljson_jsontable.pgc files are
>     already testing json handling in ecpg; perhaps just extend those a bit?
> 
> Thanks for bringing this up! I have added new tests in src/interfaces/ 
> ecpg/test/sql/sqljson.pgc.
> 
> 
> Best,
> Alex

On 13/03/2025 15:02, Alexandra Wang wrote:
> Hi Mark,
>
> Thank you so much for reviewing! I have attached the new patches.
>

Hi Alex,


I am reviewing this from a feature perspective and not from a code 
perspective.  On the whole, this looks good to me from a standards point 
of view.


There are two things that stand out to me about this feature:


1) I am not seeing the ** syntax in the standard, so does it need to be 
signaled as not supported?  Perhaps I am just overlooking something.


2) There is no support for <JSON item method>. Since this appears to be 
constructing a jsonpath query, could that not be added to the syntax and 
allow jsonpath to throw the error if the function doesn't exist?


-- 

Vik Fearing

hi.

I have applied for 0001 to 0006.

static void
jsonb_subscript_transform(SubscriptingRef *sbsref,
                          List **indirection,
                          ParseState *pstate,
                          bool isSlice,
                          bool isAssignment)
{
    List       *upperIndexpr = NIL;
    ListCell   *idx;
    sbsref->refrestype = JSONBOID;
    sbsref->reftypmod = -1;
    if (jsonb_check_jsonpath_needed(*indirection))
    {
        sbsref->refjsonbpath =
            jsonb_subscript_make_jsonpath(pstate, indirection,
                                          &sbsref->refupperindexpr,
                                          &sbsref->reflowerindexpr);
        return;
    }
    foreach(idx, *indirection)
    {
        Node       *i = lfirst(idx);
        A_Indices  *ai;
        Node       *subExpr;
        Assert(IsA(i, A_Indices));
        ai = castNode(A_Indices, i);
        if (isSlice)
        {
            Node       *expr = ai->uidx ? ai->uidx : ai->lidx;
            ereport(ERROR,
                    (errcode(ERRCODE_DATATYPE_MISMATCH),
                     errmsg("jsonb subscript does not support slices"),
                     parser_errposition(pstate, exprLocation(expr))));
}

I am confused by the above error handling:
errmsg("jsonb subscript does not support slices").

we can do
select (jsonb '[1,2,3]')[0:1];
or
SELECT (jb).a[2:3].b FROM test_jsonb_dot_notation;

this is by definition, "slices"?
Anyway, I doubt this error handling will ever be reachable.

jsonb_check_jsonpath_needed checks whether the indirection contains is_slice,
but jsonb_subscript_transform already takes isSlice as an argument.
Maybe we can refactor it somehow.


T_String is a primitive node type with no subnodes.
typedef struct String
{
    pg_node_attr(special_read_write)
    NodeTag        type;
    char       *sval;
} String;
then in src/backend/nodes/nodeFuncs.c:
                    if (expr && !IsA(expr, String) && WALK(expr))
                        return true;
we can change it to
                    if (WALK(expr))
                        return true;
but in function expression_tree_walker_impl
we have to change it as

        case T_MergeSupportFunc:
        case T_String:
            /* primitive node types with no expression subnodes */
            break;

hi.

in src/backend/catalog/sql_features.txt
should we mark any T860, T861, T862, T863, T864
items as YES?


typedef struct SubscriptingRef
{
    /* expressions that evaluate to upper container indexes */
    List       *refupperindexpr;
}
SubscriptingRef.refupperindexpr meaning changed,
So the above comments also need to be changed?


v11-0004-Extract-coerce_jsonpath_subscript.patch
+ /*
+ * We known from can_coerce_type that coercion will succeed, so
+ * coerce_type could be used. Note the implicit coercion context, which is
+ * required to handle subscripts of different types, similar to overloaded
+ * functions.
+ */
+ subExpr = coerce_type(pstate,
+  subExpr, subExprType,
+  targetType, -1,
+  COERCION_IMPLICIT,
+  COERCE_IMPLICIT_CAST,
+  -1);
+ if (subExpr == NULL)
+ ereport(ERROR,
+ (errcode(ERRCODE_DATATYPE_MISMATCH),
+ errmsg("jsonb subscript must have text type"),

the targetType can be "integer", then the error message
errmsg("jsonb subscript must have text type") would be wrong?
Also this error handling is not necessary.
since we can_coerce_type already tell us that coercion will succeed.
Also, do we really put v11-0004 as a separate patch?


in gram.y we have:
                    if (!IsA($5, A_Const) ||
                        castNode(A_Const, $5)->val.node.type != T_String)
                        ereport(ERROR,
                                errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
                                errmsg("only string constants are
supported in JSON_TABLE path specification"),
so simply, in make_jsonpath_item_expr we can

    expr = transformExpr(pstate, expr, pstate->p_expr_kind);
    if (!IsA(expr, Const) || ((Const *) expr)->consttype != INT4OID)
        ereport(ERROR,
                errcode(ERRCODE_DATATYPE_MISMATCH),
                errmsg("only integer constants are supported in jsonb
simplified accessor subscripting"),
                parser_errposition(pstate, exprLocation(expr)));

because I think the current error message "got type: unknown" is not good.
select ('123'::jsonb).a['1'];
ERROR:  jsonb simplified accessor supports subscripting in type: INT4,
got type: unknown
then we don't need two "ereport(ERROR" within make_jsonpath_item_expr
we can also Assert (cnst->constisnull) is false.
see gram.y:16976


I saw you introduce the word "AST", for example
"Converts jsonpath AST into jsonpath value in binary."
I am not sure that is fine.


in jsonb_subscript_make_jsonpath we have:
+ foreach(lc, *indirection)
+ {
+
+        if (IsA(accessor, String))
+ ....
+        else if (IsA(accessor, A_Star))
+ ....
+        else if (IsA(accessor, A_Indices))
+ ....
+        else
+              break;

Is the last else branch unreliable? since indirection only support for
String, A_Star, A_Indices, we already have Assert in jsonb_check_jsonpath_needed
to ensure that.
+ *indirection = list_delete_first_n(*indirection, pathlen);
also this is not necessary,
because pathlen will be the same length as list *indirection in current design.


Please check the attached minor refactoring based on v11-0001 to v11-0006

hi.

in gram.y we have:
indirection_el:
            '.' attr_name
                {
                    $$ = (Node *) makeString($2);
                }

we can be sure that dot notation, following dot is a plain string.
then in jsonb_subscript_transform, we can transform the String Node to
a TEXTOID Const.
with that, most of the
v11-0005-Enable-String-node-as-field-accessors-in-generic.patch
would be unnecessary.
Also in v11-0006-Implement-read-only-dot-notation-for-jsonb.patch
all these with pattern
``if (IsA(expr, String)``
can be removed.


in transformContainerSubscripts we have:
    sbsref = makeNode(SubscriptingRef);
    sbsref->refcontainertype = containerType;
    sbsref->refelemtype = elementType;
    sbsref->reftypmod = containerTypMod;
    sbsref->refexpr = (Expr *) containerBase;
    sbsref->refassgnexpr = NULL;    /* caller will fill if it's an assignment */
    sbsroutines->transform(sbsref, indirection, pstate,
                           isSlice, isAssignment);

then jsonb_subscript_transform we have
        sbsref->refjsonbpath =
            jsonb_subscript_make_jsonpath(pstate, indirection,
                                          &sbsref->refupperindexpr,
                                          &sbsref->reflowerindexpr);
of course sbsref->refupperindexpr, sbsref->reflowerindexpr is NIL
since we first called jsonb_subscript_make_jsonpath.

so we can simplify the function signature as
static void jsonb_subscript_make_jsonpath(pstate, indirection, sbsref)

Within jsonb_subscript_make_jsonpath we are going to populate
refupperindexpr, reflowerindexpr, refjsonbpath.


The attached patch addresses both of these issues, along with additional related
refactoring.  It consolidates all the changes I think are appropriate, based on
patches v1-0001 to v1-0006. This will include patches previously I sent in
the earlier thread.

On Thu, 13 Mar 2025 at 15:02, Alexandra Wang
<alexandra.wang.oss@gmail.com> wrote:
> I have attached the new patches.

Okay I finally found the time to look at this. I created a draft PR
for pg_duckdb[1] to see if I would run into issues. There was only one
real issue I found by doing this. The .* syntax is encoded as NULL in
refupperindexpr, but that is currently already a valid representation
of a slice operation without specifying upper or lower. The easiest
way to reproduce the problem is:

CREATE TABLE arr(a int[]);
explain (verbose) SELECT a[:] FROM arr;
                          QUERY PLAN
───────────────────────────────────────────────────────────────
 Seq Scan on public.arr  (cost=0.00..23.60 rows=1360 width=32)
   Output: a.*

That last line should instead be
  Output: a[:]

So I think we need to add a new Star node type, that represents the *
literal after parsing.

I haven't looked too closely at the code yet.

On Wed, Jul 9, 2025 at 4:02 PM Alexandra Wang
<alexandra.wang.oss@gmail.com> wrote:
>
> Thanks again for the patch! It was really helpful! I didn't directly
> apply it as I made a few different choices, but I think I have
> addressed all the points you covered in it.
>
> Let me know your thoughts!
>

hi.

in v12-0001 and v12-0002.
in transformIndirection
        if (!newresult)
        {
            /*
             * generic subscripting failed; falling back to function call or
             * field selection for a composite type.
             */
            Node       *n;
            /* try to find function for field selection */
            newresult = ParseFuncOrColumn(pstate,
                                          list_make1(n),
                                          list_make1(result),
                                          last_srf,
                                          NULL,
                                          false,
                                          location);
}
the above comments mentioning "function call" is wrong?
you passed NULL for (FuncCall *fn) in ParseFuncOrColumn.
and ParseFuncOrColumn comments says
```If fn is null, we're dealing with column syntax not function syntax.``


I think coerce_jsonpath_subscript can be further simplified.
we already have message like:
errhint("jsonb subscript must be coercible to either integer or text."),
no need to pass the third argument a constant (INT4OID).
also
``Oid            targetType = UNKNOWNOID;``
set it as InvalidOid would be better.
attached is a minor refactoring of coerce_jsonpath_subscript
based on (v12-0001 to v12-0004).


after applied v12-0001 to v12-0006
+ /* emit warning conditionally to minimize duplicate warnings */
+ if (list_length(*indirection) > 0)
+ ereport(WARNING,
+ errcode(ERRCODE_WARNING),
+ errmsg("mixed usage of jsonb simplified accessor syntax and jsonb
subscripting."),
+ errhint("use dot-notation for member access, or use non-null integer
constants subscripting for array access."),
+ parser_errposition(pstate, warning_location));

src7=# select ('{"a": 1, "b": "c", "d": [1, 2, 3]}'::jsonb).d['1'::int8];
WARNING:  mixed usage of jsonb simplified accessor syntax and jsonb
subscripting.
LINE 1: ...t ('{"a": 1, "b": "c", "d": [1, 2, 3]}'::jsonb).d['1'::int8]...
                                                             ^
HINT:  use dot-notation for member access, or use non-null integer
constants subscripting for array access.
ERROR:  subscript type bigint is not supported
LINE 1: ...t ('{"a": 1, "b": "c", "d": [1, 2, 3]}'::jsonb).d['1'::int8]...
                                                             ^
HINT:  jsonb subscript must be coercible to either integer or text.

The above example looks very bad. location printed twice, hint message
is different.
two messages level (ERROR, WARNING).

also "or use non-null integer constants subscripting for array
access." seems wrong?
as you can see the below hint message saying it could be text or integer.

select ('{"a": 1, "b": "c", "d": [1, 2, 3]}'::jsonb)['1'::int8];
ERROR:  subscript type bigint is not supported
LINE 1: ...ect ('{"a": 1, "b": "c", "d": [1, 2, 3]}'::jsonb)['1'::int8]...
                                                             ^
HINT:  jsonb subscript must be coercible to either integer or text.

also select ('{"a": 1, "b": "c", "d": [1, 2, 3]}'::jsonb)[NULL::int4];
return NULL,  so "use non-null integer constants" is wrong.

On Thu, Jul 10, 2025 at 4:53 PM jian he <jian.universality@gmail.com> wrote:
>
> src7=# select ('{"a": 1, "b": "c", "d": [1, 2, 3]}'::jsonb).d['1'::int8];
> WARNING:  mixed usage of jsonb simplified accessor syntax and jsonb
> subscripting.
> LINE 1: ...t ('{"a": 1, "b": "c", "d": [1, 2, 3]}'::jsonb).d['1'::int8]...
>                                                              ^
> HINT:  use dot-notation for member access, or use non-null integer
> constants subscripting for array access.
> ERROR:  subscript type bigint is not supported
> LINE 1: ...t ('{"a": 1, "b": "c", "d": [1, 2, 3]}'::jsonb).d['1'::int8]...
>                                                              ^
> HINT:  jsonb subscript must be coercible to either integer or text.
>
> The above example looks very bad. location printed twice, hint message
> is different.
> two messages level (ERROR, WARNING).
>
For plainSELECT statement, we have WARNING only in
src/test/regress/expected/xml.out,  src/test/regress/expected/xml_2.out
for example:
SELECT xpath('/*', '<relativens xmlns=''relative''/>');
WARNING:  line 1: xmlns: URI relative is not absolute
<relativens xmlns='relative'/>
                            ^
                xpath
--------------------------------------
 {"<relativens xmlns=\"relative\"/>"}
(1 row)

so i am not sure a plain SELECT statement issuing WARNING is appropriate.
------------------------------------------
in jsonb_subscript_make_jsonpath we have
    foreach(lc, *indirection)
{
        if (IsA(accessor, String))
            ....
        else if (IsA(accessor, A_Indices))
        else
            /*
             * Unsupported node type for creating jsonpath. Instead of
             * throwing an ERROR, break here so that we create a jsonpath from
             * as many indirection elements as we can and let
             * transformIndirection() fallback to alternative logic to handle
             * the remaining indirection elements.
             */
              break;
}
the above ELSE branch comments look suspicious to me.
transformIndirection->transformContainerSubscripts->jsonb_subscript_transform->jsonb_subscript_make_jsonpath
As you can see, transformIndirection have a long distance from
jsonb_subscript_make_jsonpath,
let transformIndirection handle remaining indirection elements seems not good.

if you look at src/backend/parser/gram.y line 16990.
transformIndirection(ParseState *pstate, A_Indirection *ind)
ind->indirection can be be Node of String, A_Indices, A_Star

also the above ELSE branch never reached in regress tests.
------------------------------------------

typedef struct FieldAccessorExpr
{
    Expr        xpr;
    char       *fieldname;        /* name of the JSONB object field accessed via
                                 * dot notation */
    Oid            faecollid pg_node_attr(query_jumble_ignore);
    int            location;
}            FieldAccessorExpr;

first field as NodeTag should be just fine?
I am not sure the field "location" is needed now, if it is needed, it should be
type as ParseLoc.
we should add it to src/tools/pgindent/typedefs.list

On Thu, Jul 10, 2025 at 9:34 PM jian he <jian.universality@gmail.com> wrote:
>
> ------------------------------------------
> in jsonb_subscript_make_jsonpath we have
>     foreach(lc, *indirection)
> {
>         if (IsA(accessor, String))
>             ....
>         else if (IsA(accessor, A_Indices))
>         else
>             /*
>              * Unsupported node type for creating jsonpath. Instead of
>              * throwing an ERROR, break here so that we create a jsonpath from
>              * as many indirection elements as we can and let
>              * transformIndirection() fallback to alternative logic to handle
>              * the remaining indirection elements.
>              */
>               break;
> }
> the above ELSE branch comments look suspicious to me.
> transformIndirection->transformContainerSubscripts->jsonb_subscript_transform->jsonb_subscript_make_jsonpath
> As you can see, transformIndirection have a long distance from
> jsonb_subscript_make_jsonpath,
> let transformIndirection handle remaining indirection elements seems not good.
>
context v12-0001 to v12-0006.
this ELSE branch comments is wrong, because
+ if (jsonb_check_jsonpath_needed(*indirection))
+ {
+ jsonb_subscript_make_jsonpath(pstate, indirection, sbsref);
+ if (sbsref->refjsonbpath)
+ return;
+ }
in jsonb_check_jsonpath_needed we already use Assert to confirm that
list "indirection"
is either String or A_Indices Node.

in transformContainerSubscripts we have
sbsroutines->transform(sbsref, indirection, pstate,
                           isSlice, isAssignment);
    /*
     * Error out, if datatype failed to consume any indirection elements.
     */
    if (list_length(*indirection) == indirection_length)
    {
        Node       *ind = linitial(*indirection);
        if (noError)
            return NULL;
        if (IsA(ind, String))
            ereport(ERROR,
                    (errcode(ERRCODE_DATATYPE_MISMATCH),
                     errmsg("type %s does not support dot notation",
                            format_type_be(containerType)),
                     parser_errposition(pstate, exprLocation(containerBase))));
        else if (IsA(ind, A_Indices))
            ereport(ERROR,
                    (errcode(ERRCODE_DATATYPE_MISMATCH),
                     errmsg("type %s does not support array subscripting",
                            format_type_be(containerType)),
                     parser_errposition(pstate, exprLocation(containerBase))));
        else
            elog(ERROR, "invalid indirection operation: %d", nodeTag(ind));
    }
sbsroutines->transform currently will call
array_subscript_transform, hstore_subscript_transform, jsonb_subscript_transform

in jsonb_subscript_transform callee we unconditionally do:
    *indirection = list_delete_first_n(*indirection, pathlen);
   *indirection = list_delete_first_n(*indirection, list_length(upperIndexpr));
in array_subscript_transform, we do
*indirection = list_delete_first_n(*indirection, ndim);

That means, if sbsroutines->transform not error out and indirection is
not NIL (which is unlikely)
then sbsroutines->transform will consume some induction elements.
instead of the above verbose ereport(ERROR, error handling, we can use Assert
    Assert(indirection_length > list_length(*indirection));

for the above comments, i did a refactoring based on v12 (0001 to 0006).

On Thu, Aug 21, 2025 at 12:54 PM Alexandra Wang
<alexandra.wang.oss@gmail.com> wrote:
>
> Hi Jian,
>
> Thanks for reviewing! I’ve attached v13, which addresses your
> feedback.
>

hi.
in the context of v13-0001 and v13-0002.

In transformIndirection:
while (subscripts)
{
   Node       *newresult = (Node *)  =
transformContainerSubscripts(....&subscripts...)
}

In transformContainerSubscripts:
    sbsroutines->transform(sbsref, indirection, pstate,
                           isSlice, isAssignment);
    /*
     * Error out, if datatype failed to consume any indirection elements.
     */
    if (list_length(*indirection) == indirection_length)
    {
    }
As you can see from the above code pattern, "sbsroutines->transform" is
responsible for consuming the "indirection".
If it doesn’t consume it, the function should either return NULL or raise an
error.
But what happens if the elements consumed by "sbsroutines->transform"
are not contiguous?

jsonb_subscript_transform below changes in v13-0002
+ if (!IsA(lfirst(idx), A_Indices))
+ break;
may cause elements consumed not contiguous.


diff --git a/src/backend/utils/adt/jsonbsubs.c
b/src/backend/utils/adt/jsonbsubs.c
index 8ad6aa1ad4f..a0d38a0fd80 100644
--- a/src/backend/utils/adt/jsonbsubs.c
+++ b/src/backend/utils/adt/jsonbsubs.c
@@ -55,9 +55,14 @@ jsonb_subscript_transform(SubscriptingRef *sbsref,
  */
  foreach(idx, *indirection)
  {
- A_Indices  *ai = lfirst_node(A_Indices, idx);
+ A_Indices  *ai;
  Node   *subExpr;

+ if (!IsA(lfirst(idx), A_Indices))
+ break;
+
+ ai = lfirst_node(A_Indices, idx);
+
  if (isSlice)
  {
  Node   *expr = ai->uidx ? ai->uidx : ai->lidx;
@@ -160,7 +165,9 @@ jsonb_subscript_transform(SubscriptingRef *sbsref,
  sbsref->refrestype = JSONBOID;
  sbsref->reftypmod = -1;

- *indirection = NIL;
+ /* Remove processed elements */
+ if (upperIndexpr)
+ *indirection = list_delete_first_n(*indirection, list_length(upperIndexpr));
 }

If the branch ``if (!IsA(lfirst(idx), A_Indices))`` is ever reached, then
``*indirection = list_delete_first_n(*indirection, list_length(upperIndexpr));``
might produce an incorrect result?

However, it appears that this branch is currently unreachable,
at least regress tests not coverage for
+ if (!IsA(lfirst(idx), A_Indices))
+ break;

Later patch we may have resolved this issue, but in the context of
0001 and 0002,
this seem inconsistent?

On Sat, Aug 23, 2025 at 3:34 AM Alexandra Wang
<alexandra.wang.oss@gmail.com> wrote:
>
> I don’t understand the question. In the case of an unsupported Node
> type (not an Indices in patch 0001 or 0002), we break out of the loop
> to stop transforming the remaining subscripts. So there won’t be any
> ‘not contiguous’ indirection elements.
>

Sorry, my last message is wrong.
this time, I applied v13-0001 to v13-0005.
and I found some minor issues.....

v13-0002-Allow-Generic-Type-Subscripting-to-Accept-Dot-No.patch
+ /*
+ * Error out, if datatype failed to consume any indirection elements.
+ */
+ if (list_length(*indirection) == indirection_length)
+ {
+ Node   *ind = linitial(*indirection);
+
+ if (noError)
+ return NULL;
+
+ if (IsA(ind, String))
+ ereport(ERROR,
+ (errcode(ERRCODE_DATATYPE_MISMATCH),
+ errmsg("type %s does not support dot notation",
+ format_type_be(containerType)),
+ parser_errposition(pstate, exprLocation(containerBase))));
+ else if (IsA(ind, A_Indices))
+ ereport(ERROR,
+ (errcode(ERRCODE_DATATYPE_MISMATCH),
+ errmsg("type %s does not support array subscripting",
+ format_type_be(containerType)),
+ parser_errposition(pstate, exprLocation(containerBase))));
+ else
+ elog(ERROR, "invalid indirection operation: %d", nodeTag(ind));
+ }

these error message still not reached after I apply
v13-0005-Implement-read-only-dot-notation-for-jsonb.patch
maybe we can simply do

+ if (noError)
+ return NULL;
+
+ ereport(ERROR,
+ errcode(ERRCODE_DATATYPE_MISMATCH),
+ errmsg("type %s does not support subscripting",
+ format_type_be(containerType)),
+ parser_errposition(pstate, exprLocation(containerBase)));
?

for V13-0004.
in master we have
            if (subExpr == NULL)
                ereport(ERROR,
                        (errcode(ERRCODE_DATATYPE_MISMATCH),
                         errmsg("jsonb subscript must have text type"),
                         parser_errposition(pstate, exprLocation(subExpr))));
but this part is removed from v13-0004-Extract-coerce_jsonpath_subscript
?

coerce_jsonpath_subscript_to_int4_or_text
maybe we can just rename to
coerce_jsonpath_subscript,
then add some comments explaining why currently only support result node coerce
to text or int4 data type.

for v13-0005-Implement-read-only-dot-notation-for-jsonb.patch

+ i = 0;
+ foreach(lc, sbsref->refupperindexpr) {
+ Expr *e = (Expr *) lfirst(lc);
+
+ /* When slicing, individual subscript bounds can be omitted */
+ if (!e) {
+ sbsrefstate->upperprovided[i] = false;
+ sbsrefstate->upperindexnull[i] = true;
+ } else {
+ sbsrefstate->upperprovided[i] = true;
+ /* Each subscript is evaluated into appropriate array entry */
+ ExecInitExprRec(e, state,
+ &sbsrefstate->upperindex[i],
+ &sbsrefstate->upperindexnull[i]);
+ }
+ i++;
  }
curly braces should be put into the next new line.

there are two
+ if (!sbsref->refjsonbpath)
+{
+}
maybe we can put these two together.


+SELECT (jb)['b']['x'].z FROM test_jsonb_dot_notation; -- returns NULL
with warnings
+ z
+---
+
+(1 row)
there is no warning, "returns NULL with warnings" should be removed.


+-- clean up
+DROP TABLE test_jsonb_dot_notation;
\ No newline at end of file

to remove "\ No newline at end of file"
you need to add a newline to jsonb.sql, you may see other regress sql
test files.


+ foreach(lc, *indirection)
+ if()
+ {
+ }
+ else
+ {
+ /*
+ * Unexpected node type in indirection list. This should not
+ * happen with current grammar, but we handle it defensively by
+ * breaking out of the loop rather than crashing. In case of
+ * future grammar changes that might introduce new node types,
+ * this allows us to create a jsonpath from as many indirection
+ * elements as we can and let transformIndirection() fallback to
+ * alternative logic to handle the remaining indirection elements.
+ */
+ Assert(false); /* not reachable */
+ break;
+ }
+
+ /* append path item */
+ path->next = jpi;
+ path = jpi;
+ pathlen++;

If the above else branch is reached, it will crash due to `Assert(false);`,
which contradicts the preceding comments.
to make the comments accurate, we need remove
" Assert(false); /* not reachable */"
?


  /*
- * Transform and convert the subscript expressions. Jsonb subscripting
- * does not support slices, look only at the upper index.
+ * We would only reach here if json simplified accessor is not needed, or
+ * if jsonb_subscript_make_jsonpath() didn't consume any indirection
+ * element — either way, the first indirection element could not be
+ * converted into a JsonPath component. This happens when it's a non-slice
+ * A_Indices with a non-integer upper index. The code below falls back to
+ * traditional jsonb subscripting for such cases.
  */
the above comments, "non-integer" part is wrong?
For example, the below two SELECTs both use "traditional" jsonb
subscripting logic.
CREATE TABLE ts1 AS SELECT '[1, [2]]' ::jsonb jb;
SELECT (jb)[1] FROM ts1;
SELECT (jb)['a'] FROM ts1;

On Tue, Aug 26, 2025 at 11:53 AM Alexandra Wang
<alexandra.wang.oss@gmail.com> wrote:
>
> Hi Jian,
>
> I’ve attached v14, which includes only indentation and comment changes
> from v13.
>

hi.
still reviewing v14-0001 to v14-0005.

I am confused by the comments in jsonb_subscript_transform
""
     * (b) jsonb_subscript_make_jsonpath() examined the first indirection
     * element but could not turn it into a JsonPath component (for example,
     * ['a']).
"""
CREATE TABLE ts2 AS SELECT '{"a": "b"}' ::jsonb jb;
SELECT (jb)['a'] FROM ts2;
SELECT (jb).a['a'] FROM ts2;
in these two cases, ['a'], it won't reach jsonb_subscript_make_jsonpath,
because jsonb_check_jsonpath_needed will return false.

maybe I am missing something,
for the above point b, can you use some SQL example to explain it?


make_jsonpath_item_expr(ParseState *pstate, Node *expr, List **exprs)
if expr satisfies the condition, then append the transformed expr to List exprs
if not returns NULL.
The list append logic is within make_jsonpath_item_int.
seems not that intuitive (another level of indirection)
maybe we don't need make_jsonpath_item_int.

Another reason would be in make_jsonpath_item_int it's not easy found
out that exprs actually refers to SubscriptingRef->refupperindexpr

also v14-0006-Implement-Jsonb-subscripting-with-slicing.patch also doesn't use
make_jsonpath_item_int that much frequently.
--------------------------------
in v14-0005-Implement-read-only-dot-notation-for-jsonb.patch

+ * In addition to building the JsonPath expression, this function populates
+ * the following fields of the given SubscriptingRef:
+ * - refjsonbpath: the generated JsonPath
+ * - refupperindexpr: upper index expressions (object keys or array indexes)
+ * - reflowerindexpr: lower index expressions, remains NIL as slices
are not yet supported.

"reflowerindexpr" changed in v14-0006, so "reflowerindexpr" comments
in v14-0006 need to change.
but "reflowerindexpr" we didn't touch in v14-0005, so the
"reflowerindexpr" comments look weird.


+SELECT (jb).a['b'] FROM test_jsonb_dot_notation; -- returns NULL due
to strict mode
we can not specify strict just like,  ``select 'strict $'::jsonpath;``
all the patches in here are default to lax mode.
so I am confused by this comment. ("strict mode").

The last comment is about error message:

```

evantest=# select data.a from test_jsonb_types;

ERROR:  missing FROM-clause entry for table "data"

LINE 1: select data.a from test_jsonb_types;

```

“Missing FROM-clause entry” is quite confusing and not providing much useful hint to resolve the problem. When I first saw this error, I was stuck. Only after read through the commit comments, I figured out the problem. For end users, we should provide some more meaningful error messages.