Thread: SQL/JSON: JSON_TABLE

Attached patches implementing JSON_TABLE.

This patchset depends on the 8th version of SQL/JSON functions patchset 
that was posted in 
https://www.postgresql.org/message-id/cd0bb935-0158-78a7-08b5-904886deac4b%40postgrespro.ru

-- 
Nikita Glukhov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Attached 9th version of JSON_TABLE patches rebased onto the latest master.

Documentation drafts written by Oleg Bartunov:
https://github.com/obartunov/sqljsondoc

-- 
Nikita Glukhov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Attached 10th version of JSON_TABLE patches rebased onto the latest master.

-- 
Nikita Glukhov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Attached 11th version of JSON_TABLE patches rebased onto the latest master.

Fixed PLAN DEFAULT flags assignment in gram.y.

-- 
Nikita Glukhov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Attached 12th version of JSON_TABLE patches rebased onto the latest master.

Fixed JSON_TABLE plan validation.

-- 
Nikita Glukhov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Attached 13th version of JSON_TABLE patches rebased onto the latest master.

-- 
Nikita Glukhov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Attached 15th version of JSON_TABLE patches.

Implicit root path name assignment was disabled (it is unclear from standard).
Now all JSON path names are required if the explicit PLAN clause is used.


The documentation for JSON_TABLE can be found now in a separate patch:
https://www.postgresql.org/message-id/732208d3-56c3-25a4-8f08-3be1d54ad51b%40postgrespro.ru

-- 
Nikita Glukhov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Attached 16th version of JSON_TABLE patches.

Changed only results of regression tests after the implicit coercion via I/O
was removed from JSON_VALUE.

-- 
Nikita Glukhov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

> On Tue, Jul 3, 2018 at 4:50 PM Nikita Glukhov <n.gluhov@postgrespro.ru> wrote:
>
> Attached 16th version of JSON_TABLE patches.
>
> Changed only results of regression tests after the implicit coercion via I/O
> was removed from JSON_VALUE.

Thank you for working on this patch! Unfortunately, the current version of
patch 0015-json_table doesn't not apply anymore without conflicts, could you
please rebase it? In the meantime I'll try to provide some review.

On 26.11.2018 15:55, Dmitry Dolgov wrote:

>> On Tue, Jul 3, 2018 at 4:50 PM Nikita Glukhov <n.gluhov@postgrespro.ru> wrote:
>>
>> Attached 16th version of JSON_TABLE patches.
>>
>> Changed only results of regression tests after the implicit coercion via I/O
>> was removed from JSON_VALUE.
> Thank you for working on this patch! Unfortunately, the current version of
> patch 0015-json_table doesn't not apply anymore without conflicts, could you
> please rebase it? In the meantime I'll try to provide some review.

Attached 20th version of the patches rebased onto the current master.

-- 
Nikita Glukhov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Attached 21st version of the patches rebased onto the current master.


You can also see all SQL/JSON v21 patches successfully applied in our GitHub 
repository on the following branches:
https://github.com/postgrespro/sqljson/tree/sqljson_v21  (one commit per patch)
https://github.com/postgrespro/sqljson/tree/sqljson   (original commit history)

Attached 34th version of the patches.

On Thu, Feb 28, 2019 at 8:19 PM Nikita Glukhov <n.gluhov@postgrespro.ru> wrote:
> Attached 34th version of the patches.

Kinda strange version numbering -- the last post on this thread is v21.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 01.03.2019 19:17, Robert Haas wrote:

On Thu, Feb 28, 2019 at 8:19 PM Nikita Glukhov <n.gluhov@postgrespro.ru> wrote:

Attached 34th version of the patches.

Kinda strange version numbering -- the last post on this thread is v21.

For simplicity of dependence tracking, version numbering of JSON_TABLE patches
matches the version numbering of the patches on which it  depends -- jsonpath
and SQL/JSON.  The corresponding jsonpath patch has version v34 now.

-- 
Nikita Glukhov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Attached 36th version of patches rebased onto jsonpath v36.

-- 
Nikita Glukhov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

On 29.06.2019 8:40, Pavel Stehule wrote:

Hi

so 29. 6. 2019 v 7:26 odesílatel Nikita Glukhov <n.gluhov@postgrespro.ru> napsal:

Attached 36th version of patches rebased onto jsonpath v36.

I cannot to apply these patches on master. Please, can you check these patches?

Now this is one giant patchset ... and at least the first patch seems to
have more than one thing within -- even the commit message says so.  It
seems clear that this is going to take a long time to digest; maybe if
we can get it in smaller pieces we can try to have a little at a time?
In other words, may I suggest to split it up in pieces that can be
reviewed and committed independently?

v37 no longer applies so it requires a rebase, and also typedefs.list
was wrongly merged.

Please update.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 03.09.2019 20:29, Alvaro Herrera wrote:

Now this is one giant patchset ... and at least the first patch seems to
have more than one thing within -- even the commit message says so.  It
seems clear that this is going to take a long time to digest; maybe if
we can get it in smaller pieces we can try to have a little at a time?
In other words, may I suggest to split it up in pieces that can be
reviewed and committed independently?

Patch 0001 is simply a squash of all 7 v38 patches from the thread 
"SQL/JSON: functions".  These patches are preliminary for JSON_TABLE.

Patch 0002 only needs to be review in this thread.

v37 no longer applies so it requires a rebase, and also typedefs.list
was wrongly merged.

typedefs.list was fixed.

Please update.

Attached 38th version of the patches.


-- 
Nikita Glukhov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

I got warning

ar crs libpgcommon.a base64.o config_info.o controldata_utils.o d2s.o exec.o f2s.o file_perm.o ip.o keywords.o kwlookup.o link-canary.o md5.o pg_lzcompress.o pgfnames.o psprintf.o relpath.o rmtree.o saslprep.o scram-common.o string.o unicode_norm.o username.o wait_error.>
...skipping...
clauses.c:1076:3: warning: this ‘if’ clause does not guard... [-Wmisleading-indentation]
 1076 |   if (ExecEvalJsonNeedsSubTransaction(jsexpr, NULL))
      |   ^~
clauses.c:1078:4: note: ...this statement, but the latter is misleadingly indented as if it were guarded by the ‘if’
 1078 |    return true;
      |    ^~~~~~
gcc -Wall -Wmissing-protot

Fixed in 38th version. Thanks.

Regress tests diff is not empty - see attached file

Unfortunately, this is not reproducible on my machine, but really seems to be 
a bug.

some strange fragments from code:

    deparseExpr(node->arg, context);
-   if (node->relabelformat != COERCE_IMPLICIT_CAST)
+   if (node->relabelformat != COERCE_IMPLICIT_CAST &&
+       node->relabelformat == COERCE_INTERNAL_CAST)

There obviously should be

node->relabelformat != COERCE_INTERNAL_CAST

Fixed in 38th version. Thanks.

Now, "format"  is type_func_name_keyword, so when you use it, then nobody 
can use "format" as column name. It can break lot of application. "format" 
is common name. It is relatively unhappy, and it can touch lot of users.

FORMAT was moved to unreserved_keywords in the 38th version. I remember that
there was conflicts with FORMAT, but now it works as unreserved_keyword.

This patch set (JSON functions & JSON_TABLE) has more tha 20K rows. More, there are more than few features are implemented.

Is possible to better (deeper) isolate these features, please? I have nothing against any implemented feature, but it is hard to work intensively (hard test) on this large patch. JSON_TABLE has only 184kB, can we start with this patch?

SQLJSON_FUNCTIONS has 760kB - it is maybe too much for one feature, one patch.

Patch 0001 is simply a squash of all 7 patches from the thread 
"SQL/JSON: functions".  These patches are preliminary for JSON_TABLE.

Patch 0002 only needs to be review in this thread.

Attached 39th version of the patches rebased onto current master.

-- 
Nikita Glukhov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

This patch is still pretty big - it is about 6000 lines (without any documentation). I checked the standard - and this patch try to implement

JSON_TABLE as part of T821

Plan clause T824

Plan default clause T838.

Unfortunately for last two features there are few documentation other than standard, and probably other databases doesn't implement these features (I didn't find it in Oracle, MySQL, MSSQL and DB2) . Can be this patch divided by these features? I hope so separate review and commit can increase a chance to merge this code (or the most significant part) in this release.

It is pretty hard to do any deeper review without documentation and without other information sources.

What do you think?

Regress tests fails on my comp - intel 64bit Linux, gcc 9.2.1

Comments:

* +<->/* Only XMLTABLE and JSON_TABLE are supported currently */

this comment has not sense more. Can be removed. Probably long time there will not be new format like XML or JSON

* there are new 600 lines to parse_clause.c, maybe this code can be placed in new file parse_jsontable.c ? parse_clause.c is pretty long already (json_table has very complex syntax)

*

+<->if (list_length(ci->passing.values) > 0)
+<->{
+<-><-->ListCell   *exprlc;
+<-><-->ListCell   *namelc;
+

It's uncommon usage of list_length function. More common is just "if (ci->passing.values) {}". Is there any reason for list_length?

* I tested some examples that I found on net. It works very well. Minor issues are white chars for json type. Probably json_table should to trim returned values, because after cutting from document, original white chars lost sense. It is not a problem jsonb type, that reduce white chars on input.

I did only simple tests and I didn't find any other issues than white chars problems for json type. I'll continue in some deeper tests. Please, prepare documentation. Without documentation there is not clean what features are supported. I have to do blind testing.

I have added some documentation to the patches which has simply been copied from [1], but It still needs some work.

[1] https://www.postgresql.org/message-id/732208d3-56c3-25a4-8f08-3be1d54ad51b%40postgrespro.ru

On 12.11.2019 20:54, Pavel Stehule wrote:

> Hi
>
> please, can you rebase 0001-SQL-JSON-functions-v40.patch. I have a 
> problem with patching
>
> Pavel

Attached 41th version of the patches rebased onto current master.

-- 
Nikita Glukhov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

On 17.11.2019 13:35, Pavel Stehule wrote:

Hi

út 12. 11. 2019 v 22:51 odesílatel Nikita Glukhov <n.gluhov@postgrespro.ru> napsal:

On 12.11.2019 20:54, Pavel Stehule wrote:

> Hi
>
> please, can you rebase 0001-SQL-JSON-functions-v40.patch. I have a
> problem with patching
>
> Pavel

Attached 41th version of the patches rebased onto current master.

I testing functionality - randomly testing some examples that I found on internet.

Thank you for testing JSON_TABLE.

I found:

a) Oracle & MySQL (Oracle) supports EXISTS clause, this implementation not. 
I think should be useful support this clause too. 

SELECT * FROM JSON_TABLE('...', '...' COLUMNS x INT EXISTS PATH ...

EXISTS PATH clause can be emulated with jsonpath EXISTS() predicate:

=# SELECT *   FROM JSON_TABLE('{"a": 1}', '$'                  COLUMNS (                    a bool PATH 'exists($.a)',                    b bool PATH 'exists($.b)'                  ));a | b 
---+---t | f
(1 row)

But this works as expected only in lax mode.  In strict mode EXISTS() returns 
Unknown that transformed into SQL NULL:

=# SELECT *   FROM JSON_TABLE('{"a": 1}', '$'                   COLUMNS (                    a bool PATH 'strict exists($.a)',                    b bool PATH 'strict exists($.b)'                  ));a | b 
---+---t | 
(1 row)

There is no easy way to return false without external COALESCE(),
DEFAULT false ON ERROR also does not help.  

So, I think it's worth to add EXISTS PATH clause to our implementation.

There is a question how to map boolean result to other data types. 

Now, boolean result can be used in JSON_TABLE columns of bool, int4, text, 
json[b], and other types which have CAST from bool:

SELECT * 
FROM JSON_TABLE('{"a": 1}', '$'               COLUMNS (                 a int PATH 'exists($.a)',                 b text PATH 'exists($.b)'               ));a |   b 
---+-------1 | false
(1 row)

b) When searched value is not scalar, then it returns null. This behave can be 
suppressed by clause FORMAT Json. I found a different behave, and maybe I found
a bug.  On MySQL this clause is by default for JSON values (what has sense).

SELECT *FROM
      JSON_TABLE(
        '[{"a":[1,2]}]',
        '$[*]'
        COLUMNS(
         aj JSON PATH '$.a' DEFAULT '{"x": 333}' ON EMPTY 
        )
      ) AS tt;

It returns null, although it should to return [1,2]. 

Yes, regular (non-formatted) JSON_TABLE columns can accept only scalar values. 
Otherwise an error is thrown, which can be caught by ON ERROR clause. This 
behavior is specified by the standard.

FORMAT JSON is not implicitly added for json[b] columns now. The current SQL
standard does not have any json data types, so I think we can add implicit 
FORMAT JSON for json[b] typed-columns.  But I'm a bit afraid that different 
behavior can be standardized after introduction of json data types in SQL.

There is another bug maybe. Although there is DEFAULT clause. It returns NULL.

ON ERROR should be used if "not a scalar" error needs to be caught:

SELECT *
FROM   JSON_TABLE(       '[{"a":[1,2]}]',       '$[*]'       COLUMNS(           aj JSON PATH '$.a' DEFAULT '{"x": 333}' ON ERROR       )   ) AS tt;
    aj     
------------{"x": 333}
(1 row)


ON EMPTY catches only empty-result case (for example, non-existent path in 
lax mode):

SELECT *
FROM   JSON_TABLE(       '[{"a":[1,2]}]',       '$[*]'       COLUMNS(           aj JSON PATH '$.foo' DEFAULT '{"x": 333}' ON EMPTY        )   ) AS tt;    aj     
------------{"x": 333}
(1 row)

I got correct result when I used FORMAT JSON clause. 
I think it should be default behave for json and jsonb columns.

I agree that FORMAT JSON could be implicit for json[b] columns.  But I think
there could be one minor problem if we want to verify that returned value is 
scalar.

Without FORMAT JSON this is verified by the underlying JSON_VALUE expression:

SELECT *
FROM   JSON_TABLE(       '[{"a":[1,2]}]',       '$[*]'       COLUMNS (           aj JSON PATH 'lax $.a' ERROR ON ERROR        )   ) AS tt;
ERROR:  JSON path expression in JSON_VALUE should return singleton scalar item

(This error message with the reference to implicit JSON_VALUE needs to be fixed.)


But with FORMAT JSON we need to construct complex jsonpath with a filter and 
override ON EMPTY behavior:

SELECT *
FROM   JSON_TABLE(       '[{"a":[1,2]}]',       '$[*]'       COLUMNS (           aj JSON FORMAT JSON            -- strict mode is mandatory to prevent array unwrapping           PATH 'strict $.a ? (@.type() != "array" && @.type() != "object")'           ERROR ON EMPTY ERROR ON ERROR       )   ) AS tt;
ERROR:  no SQL/JSON item

Another question - when I used FORMAT JSON clause, then I got syntax error
on DEFAULT keyword .. . Is it correct? 

Why I cannot to use together FORMAT JSON and DEFAULT clauses?

JSON_TABLE columns with FORMAT JSON, like JSON_QUERY, can have only 
ERROR, NULL, EMPTY ARRAY, EMPTY OBJECT behaviors.

This syntax is specified in the SQL standard:

<JSON table formatted column definition> ::= <column name> <data type> FORMAT <JSON representation> [ PATH <JSON table column path specification> ] [ <JSON table formatted column wrapper behavior> WRAPPER ] [ <JSON table formatted column quotes behavior> QUOTES [ ON SCALAR STRING ] ] [ <JSON table formatted column empty behavior> ON EMPTY ] [ <JSON table formatted column error behavior> ON ERROR ]

<JSON table formatted column empty behavior> ::= ERROR | NULL | EMPTY ARRAY | EMPTY OBJECT

<JSON table formatted column error behavior> ::= ERROR | NULL | EMPTY ARRAY | EMPTY OBJECT


But I also think that DEFAULT clause could be very useful in JSON_QUERY and 
formatted JSON_TABLE columns.

Note - this behave is not described in documentation.

There are references to JSON_QUERY and JSON_VALUE behavior in the definitions 
of JSON_TABLE columns, but their behavior still seems to be unclear.  This 
needs to be fixed.

Attached 42th version of the patches rebased onto current master.


Changes from the previous version:* added EXISTS PATH columns* added DEFAULT clause for FORMAT JSON columns* added implicit FORMAT JSON for columns of json[b], array and composite types

čt 21. 11. 2019 v 17:31 odesílatel Nikita Glukhov <n.gluhov@postgrespro.ru> napsal:

On 17.11.2019 13:35, Pavel Stehule wrote:
I found:

a) Oracle & MySQL (Oracle) supports EXISTS clause, this implementation not. 
I think should be useful support this clause too. 

SELECT * FROM JSON_TABLE('...', '...' COLUMNS x INT EXISTS PATH ...

EXISTS PATH clause can be emulated with jsonpath EXISTS() predicate:

=# SELECT *   FROM JSON_TABLE('{"a": 1}', '$'                  COLUMNS (                    a bool PATH 'exists($.a)',                    b bool PATH 'exists($.b)'                  ));a | b 
---+---t | f
(1 row)

But this works as expected only in lax mode.  In strict mode EXISTS() returns 
Unknown that transformed into SQL NULL:

=# SELECT *   FROM JSON_TABLE('{"a": 1}', '$'                   COLUMNS (                    a bool PATH 'strict exists($.a)',                    b bool PATH 'strict exists($.b)'                  ));a | b 
---+---t | 
(1 row)

There is no easy way to return false without external COALESCE(),
DEFAULT false ON ERROR also does not help.  

So, I think it's worth to add EXISTS PATH clause to our implementation.

There is a question how to map boolean result to other data types. 

Now, boolean result can be used in JSON_TABLE columns of bool, int4, text, 
json[b], and other types which have CAST from bool:

SELECT * 
FROM JSON_TABLE('{"a": 1}', '$'               COLUMNS (                 a int PATH 'exists($.a)',                 b text PATH 'exists($.b)'               ));a |   b 
---+-------1 | false
(1 row)

EXISTS PATH columns were added.  Only column types having CASTS 
from boolean type are accepted.

Example:

SELECT * 
FROM JSON_TABLE( '{"foo": "bar"}', '$'  COLUMNS (    foo_exists boolean EXISTS PATH '$.foo',    foo int EXISTS,    err text EXISTS PATH '$ / 0' TRUE ON ERROR  )
);
foo_exists | foo |  err 
------------+-----+------t          |   1 | true   
(1 row)

b) When searched value is not scalar, then it returns null. This behave can be 
suppressed by clause FORMAT Json. I found a different behave, and maybe I found
a bug.  On MySQL this clause is by default for JSON values (what has sense).

SELECT *FROM
      JSON_TABLE(
        '[{"a":[1,2]}]',
        '$[*]'
        COLUMNS(
         aj JSON PATH '$.a' DEFAULT '{"x": 333}' ON EMPTY 
        )
      ) AS tt;

It returns null, although it should to return [1,2]. 

Yes, regular (non-formatted) JSON_TABLE columns can accept only scalar values. 
Otherwise an error is thrown, which can be caught by ON ERROR clause. This 
behavior is specified by the standard.

FORMAT JSON is not implicitly added for json[b] columns now. The current SQL
standard does not have any json data types, so I think we can add implicit 
FORMAT JSON for json[b] typed-columns.  But I'm a bit afraid that different 
behavior can be standardized after introduction of json data types in SQL.

There is another bug maybe. Although there is DEFAULT clause. It returns NULL.

ON ERROR should be used if "not a scalar" error needs to be caught:

SELECT *
FROM   JSON_TABLE(       '[{"a":[1,2]}]',       '$[*]'       COLUMNS(           aj JSON PATH '$.a' DEFAULT '{"x": 333}' ON ERROR       )   ) AS tt;
    aj     
------------{"x": 333}
(1 row)


ON EMPTY catches only empty-result case (for example, non-existent path in 
lax mode):

SELECT *
FROM   JSON_TABLE(       '[{"a":[1,2]}]',       '$[*]'       COLUMNS(           aj JSON PATH '$.foo' DEFAULT '{"x": 333}' ON EMPTY        )   ) AS tt;    aj     
------------{"x": 333}
(1 row)

I got correct result when I used FORMAT JSON clause. 
I think it should be default behave for json and jsonb columns.

I agree that FORMAT JSON could be implicit for json[b] columns.  But I think
there could be one minor problem if we want to verify that returned value is 
scalar.

Without FORMAT JSON this is verified by the underlying JSON_VALUE expression:

SELECT *
FROM   JSON_TABLE(       '[{"a":[1,2]}]',       '$[*]'       COLUMNS (           aj JSON PATH 'lax $.a' ERROR ON ERROR        )   ) AS tt;
ERROR:  JSON path expression in JSON_VALUE should return singleton scalar item

(This error message with the reference to implicit JSON_VALUE needs to be fixed.)


But with FORMAT JSON we need to construct complex jsonpath with a filter and 
override ON EMPTY behavior:

SELECT *
FROM   JSON_TABLE(       '[{"a":[1,2]}]',       '$[*]'       COLUMNS (           aj JSON FORMAT JSON            -- strict mode is mandatory to prevent array unwrapping           PATH 'strict $.a ? (@.type() != "array" && @.type() != "object")'           ERROR ON EMPTY ERROR ON ERROR       )   ) AS tt;
ERROR:  no SQL/JSON item

please, check the behave of other databases. I think so good conformance with other RDBMS is important. More this method for checking if value is object or not looks little bit scary.

maybe we can implement some functions like JSON_IS_OBJECT(), JSON_IS_ARRAY(), JSON_IS_VALUE()?

 

More - we have this functionality already

ostgres=# select json_typeof('[10,20]');
┌─────────────┐
│ json_typeof │
╞═════════════╡
│ array       │
└─────────────┘
(1 row)

Implicit FORMAT JSON is used for columns of json[b], array and composite types now.
The behavior is similar to behavior of json_populate_record().

Example:

CREATE TYPE test_record AS (foo text[], bar int);

SELECT * 
FROM JSON_TABLE( '{"foo": ["bar", 123, null]}', '$'  COLUMNS (    js json PATH '$',     jsonb_arr jsonb[] PATH '$.foo',     text_arr text[] PATH '$.foo',     int_arr int[] PATH '$.foo' DEFAULT '{}' ON ERROR,     rec test_record PATH '$'  )
);            js              |      jsonb_arr       |    text_arr    | int_arr |         rec         
-----------------------------+----------------------+----------------+---------+---------------------{"foo": ["bar", 123, null]} | {"\"bar\"",123,NULL} | {bar,123,NULL} | {}      | ("{bar,123,NULL}",)
(1 row)

Another question - when I used FORMAT JSON clause, then I got syntax error
on DEFAULT keyword .. . Is it correct? 

Why I cannot to use together FORMAT JSON and DEFAULT clauses?

JSON_TABLE columns with FORMAT JSON, like JSON_QUERY, can have only 
ERROR, NULL, EMPTY ARRAY, EMPTY OBJECT behaviors.

This syntax is specified in the SQL standard:

<JSON table formatted column definition> ::= <column name> <data type> FORMAT <JSON representation> [ PATH <JSON table column path specification> ] [ <JSON table formatted column wrapper behavior> WRAPPER ] [ <JSON table formatted column quotes behavior> QUOTES [ ON SCALAR STRING ] ] [ <JSON table formatted column empty behavior> ON EMPTY ] [ <JSON table formatted column error behavior> ON ERROR ]

<JSON table formatted column empty behavior> ::= ERROR | NULL | EMPTY ARRAY | EMPTY OBJECT

<JSON table formatted column error behavior> ::= ERROR | NULL | EMPTY ARRAY | EMPTY OBJECT


But I also think that DEFAULT clause could be very useful in JSON_QUERY and 
formatted JSON_TABLE columns.

DEFAULT clause was enabled in JSON_QUERY() and formatted JSON_TABLE columns:

SELECT * 
FROM JSON_TABLE( '{"foo": "bar"}', '$'  COLUMNS (    baz json FORMAT JSON DEFAULT '"empty"' ON EMPTY  )
);  baz   
---------"empty"
(1 row)

On 23.03.2020 19:24, Pavel Stehule wrote:

> This patch needs rebase


Attached 43rd version of the patches based on the latest (v47) SQL/JSON 
functions patches.

Nothing significant has changed from the previous version, excluding 
removed support for json type.

-- 
Nikita Glukhov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

On Mon, Mar 23, 2020 at 08:33:34PM +0300, Nikita Glukhov wrote:
> On 23.03.2020 19:24, Pavel Stehule wrote:
> > This patch needs rebase
> 
> Attached 43rd version of the patches based on the latest (v47) SQL/JSON
> functions patches.

It looks like this needs to be additionally rebased - I will set cfbot to
"Waiting".

-- 
Justin

On Sun, Jul 05, 2020 at 12:15:58PM -0500, Justin Pryzby wrote:

It looks like this needs to be additionally rebased - I will set cfbot to
"Waiting".

...  Something that has not happened in four weeks, so this is marked
as returned with feedback.  Please feel free to resubmit once a rebase
is done.
--
Michael

Atatched 44th version of the pacthes rebased onto current master
(#0001 corresponds to v51 of SQL/JSON patches).

Thank you for review.

Attached 45th version of the patches. "SQL/JSON functions" patch corresponds to 
v52 patch set posted in the separate thread.

For new files introduced in the patches:

+ * Portions Copyright (c) 1996-2019, PostgreSQL Global Development Group

2021 is a few days ahead. It would be good to update the year range.

Fixed.

For transformJsonTableColumn:

+   jfexpr->op =
+       jtc->coltype == JTC_REGULAR ? IS_JSON_VALUE :
+       jtc->coltype == JTC_EXISTS ? IS_JSON_EXISTS : IS_JSON_QUERY;

Should IS_JSON_EXISTS be mentioned in the comment preceding the method ?

Added mention of EXISTS PATH column to the comment.

For JsonTableDestroyOpaque():

+   state->opaque = NULL;

Should the memory pointed to by opaque be freed ?

I think it's not necessary, because FunctionScan, caller of
JsonTableDestroyOpaque(), will free it and also all opaque's fields using
MemoryContextReset().

+   l2 = list_head(tf->coltypes);
+   l3 = list_head(tf->coltypmods);
+   l4 = list_head(tf->colvalexprs);

Maybe the ListCell pointer variables can be named corresponding to the lists they iterate so that the code is easier to understand.

Variable were renamed, also foreach() loop was refactored using forfour() macro.

+typedef enum JsonTablePlanJoinType
+{
+   JSTP_INNER = 0x01,
+   JSTP_OUTER = 0x02,
+   JSTP_CROSS = 0x04,

Since plan type enum starts with JSTP_, I think the plan join type should start with JSTPJ_ or JSTJ_. Otherwise the following would be a bit hard to read:

+   else if (plan->plan_type == JSTP_JOINED)
+   {
+       if (plan->join_type == JSTP_INNER ||
+           plan->join_type == JSTP_OUTER)

since different fields are checked in the two if statements but the prefixes don't convey that.

Enumeration members were renames using prefix JSTPJ_.

+      Even though the path names are not incuded into the <literal>PLAN DEFAULT</literal>

Typo: incuded -> included

Fixed.

+   int         nchilds = 0;

nchilds -> nchildren

Fixed.

+#if 0 /* XXX it' unclear from the standard whether root path name is mandatory or not */
+   if (plan && plan->plan_type != JSTP_DEFAULT && !rootPathName)

Do you plan to drop the if block ?

If it becomes clear, I will drop it.

On 1/20/21 8:42 PM, Nikita Glukhov wrote:
> Thank you for review.
> 
> Attached 45th version of the patches. "SQL/JSON functions" patch corresponds to
> v52 patch set posted in the separate thread.

Another rebase needed (http://cfbot.cputube.org/patch_32_2902.log), 
marked Waiting on Author.

I can see that Álvaro suggested that the patch be split up so it can be 
reviewed and committed in pieces. It looks like you've done that to some 
extent, but I wonder if more can be done. In particular, it looks like 
that first patch could be broken up -- at lot.

Regards,
-- 
-David
david@pgmasters.net

On 3/25/21 8:10 AM, David Steele wrote:
> On 1/20/21 8:42 PM, Nikita Glukhov wrote:
>> Thank you for review.
>>
>> Attached 45th version of the patches. "SQL/JSON functions" patch
>> corresponds to
>> v52 patch set posted in the separate thread.
>
> Another rebase needed (http://cfbot.cputube.org/patch_32_2902.log),
> marked Waiting on Author.
>
> I can see that Álvaro suggested that the patch be split up so it can
> be reviewed and committed in pieces. It looks like you've done that to
> some extent, but I wonder if more can be done. In particular, it looks
> like that first patch could be broken up -- at lot.
>
>

I've rebased this. Note that the large first patch is just the
accumulated patches from the 'SQL/JSON functions' thread, and should be
reviewed there. Only patches 2 thru 4 should be reviewed here. In fact
there are no changes at all in those patches from the previous set other
than a little patch fuzz. The only substantial changes are in patch 1,
which had bitrotted. However, I'm posting a new series to keep the
numbering in sync.


If the cfbot is happy I will set back to 'Needs review'


cheers


andrew

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

> On 2021.03.26. 21:28 Andrew Dunstan <andrew@dunslane.net> wrote:
> On 3/25/21 8:10 AM, David Steele wrote:
> > On 1/20/21 8:42 PM, Nikita Glukhov wrote:
> >> Thank you for review.
> >>
> >> Attached 45th version of the patches. "SQL/JSON functions" patch
> >> corresponds to
> >> v52 patch set posted in the separate thread.
> >
> > Another rebase needed (http://cfbot.cputube.org/patch_32_2902.log),
> > marked Waiting on Author.
> >
> > I can see that Álvaro suggested that the patch be split up so it can
> > be reviewed and committed in pieces. It looks like you've done that to
> > some extent, but I wonder if more can be done. In particular, it looks
> > like that first patch could be broken up -- at lot.
> >
> >
>
> I've rebased this. Note that the large first patch is just the
> accumulated patches from the 'SQL/JSON functions' thread, and should be
> reviewed there. Only patches 2 thru 4 should be reviewed here. In fact
> there are no changes at all in those patches from the previous set other
> than a little patch fuzz. The only substantial changes are in patch 1,
> which had bitrotted. However, I'm posting a new series to keep the
> numbering in sync.
>
> If the cfbot is happy I will set back to 'Needs review'

> 0001-SQL-JSON-functions-v46.patch
> 0002-JSON_TABLE-v46.patch
> 0003-JSON_TABLE-PLAN-DEFAULT-clause-v46.patch
> 0004-JSON_TABLE-PLAN-clause-v46.patch


Hi,

The four v46 patches apply fine, but on compile I get (debian/gcc):

make --quiet -j 4
make[3]: *** No rule to make target 'parse_jsontable.o', needed by 'objfiles.txt'.  Stop.
make[3]: *** Waiting for unfinished jobs....
make[2]: *** [parser-recursive] Error 2
make[2]: *** Waiting for unfinished jobs....
make[1]: *** [all-backend-recurse] Error 2
make: *** [all-src-recurse] Error 2
common.mk:39: recipe for target 'parser-recursive' failed
Makefile:42: recipe for target 'all-backend-recurse' failed
GNUmakefile:11: recipe for target 'all-src-recurse' failed


Erik

On 3/26/21 4:48 PM, Erik Rijkers wrote:
>> On 2021.03.26. 21:28 Andrew Dunstan <andrew@dunslane.net> wrote:
>> On 3/25/21 8:10 AM, David Steele wrote:
>>> On 1/20/21 8:42 PM, Nikita Glukhov wrote:
>>>> Thank you for review.
>>>>
>>>> Attached 45th version of the patches. "SQL/JSON functions" patch
>>>> corresponds to
>>>> v52 patch set posted in the separate thread.
>>> Another rebase needed (http://cfbot.cputube.org/patch_32_2902.log),
>>> marked Waiting on Author.
>>>
>>> I can see that Álvaro suggested that the patch be split up so it can
>>> be reviewed and committed in pieces. It looks like you've done that to
>>> some extent, but I wonder if more can be done. In particular, it looks
>>> like that first patch could be broken up -- at lot.
>>>
>>>
>> I've rebased this. Note that the large first patch is just the
>> accumulated patches from the 'SQL/JSON functions' thread, and should be
>> reviewed there. Only patches 2 thru 4 should be reviewed here. In fact
>> there are no changes at all in those patches from the previous set other
>> than a little patch fuzz. The only substantial changes are in patch 1,
>> which had bitrotted. However, I'm posting a new series to keep the
>> numbering in sync.
>>
>> If the cfbot is happy I will set back to 'Needs review'
>> 0001-SQL-JSON-functions-v46.patch
>> 0002-JSON_TABLE-v46.patch
>> 0003-JSON_TABLE-PLAN-DEFAULT-clause-v46.patch
>> 0004-JSON_TABLE-PLAN-clause-v46.patch
>
> Hi,
>
> The four v46 patches apply fine, but on compile I get (debian/gcc):
>
> make --quiet -j 4
> make[3]: *** No rule to make target 'parse_jsontable.o', needed by 'objfiles.txt'.  Stop.
> make[3]: *** Waiting for unfinished jobs....
> make[2]: *** [parser-recursive] Error 2
> make[2]: *** Waiting for unfinished jobs....
> make[1]: *** [all-backend-recurse] Error 2
> make: *** [all-src-recurse] Error 2
> common.mk:39: recipe for target 'parser-recursive' failed
> Makefile:42: recipe for target 'all-backend-recurse' failed
> GNUmakefile:11: recipe for target 'all-src-recurse' failed
>


Yeah, I messed up :-( Forgot to git-add some files.


will repost soon.


cheers


andrew

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

Attached 47th version of the patches.

On 3/26/21 4:48 PM, Erik Rijkers wrote:

Hi,

The four v46 patches apply fine, but on compile I get (debian/gcc):

make --quiet -j 4
make[3]: *** No rule to make target 'parse_jsontable.o', needed by 'objfiles.txt'.  Stop.
make[3]: *** Waiting for unfinished jobs....
make[2]: *** [parser-recursive] Error 2
make[2]: *** Waiting for unfinished jobs....
make[1]: *** [all-backend-recurse] Error 2
make: *** [all-src-recurse] Error 2
common.mk:39: recipe for target 'parser-recursive' failed
Makefile:42: recipe for target 'all-backend-recurse' failed
GNUmakefile:11: recipe for target 'all-src-recurse' failed

Yeah, I messed up :-( Forgot to git-add some files.


will repost soon.

I have added forgotten files and fixed the first patch.

> On 2021.03.27. 02:12 Nikita Glukhov <n.gluhov@postgrespro.ru> wrote:
> 
> Attached 47th version of the patches.
> 
[..]
> 
> I have added forgotten files and fixed the first patch.
> 
> [0001-SQL-JSON-functions-v47.patch]
> [0002-JSON_TABLE-v47.patch]
> [0003-JSON_TABLE-PLAN-DEFAULT-clause-v47.patch]
> [0004-JSON_TABLE-PLAN-clause-v47.patch]

Hi,

Apply, build all fine.  It also works quite well, and according to specification, as far as I can tell.

But today I ran into:

ERROR:  function ExecEvalJson not in llvmjit_types.c

I think that it is caused by:

set enable_bitmapscan = off;

(I installed llvm a few days ago. llvm-3.9-dev on this debian stretch).


This is the test sql I concocted, which runs fine with enable_bitmapscan on (the default):

select jt1.* 
from myjsonfile100k as t(js, id) 
  , json_table(
      t.js
   , '$' columns (
        "lastname"   text    path  '$. "lastname"     '
      , "firstname"  text    path  '$. "firstname"    '
      , "date"       text    path  '$. "date"         '
      , "city"       text    path  '$. "city"         '
      , "country"    text    path  '$. "country"      '
      , "name 0(1)"  text    path  '$. "array"[0]     '
      , "name 4(5)"  text    path  '$. "array"[4]     '
      , "names"      text[]  path  '$. "array"        '
      , "randfloat"  float   path  '$. "random float" '
    )
) as jt1
where  js @> ('[ { "city": "Santiago de Cuba" } ]')
   and js[0]->>'firstname' = 'Gilda'
;
ERROR:  function ExecEvalJson not in llvmjit_types.c

That statement only errors out if the table is large enough. I have no time now to make a sample table but if no-one
understandsthe problem off-hand, I'll try to construct such a table later this week (the one I'm using is large, 1.5
GB).


Erik Rijkers

On 30.03.2021 19:56, Erik Rijkers wrote:

On 2021.03.27. 02:12 Nikita Glukhov <n.gluhov@postgrespro.ru> wrote:

Attached 47th version of the patches.

Hi,

Apply, build all fine.  It also works quite well, and according to specification, as far as I can tell.

But today I ran into:

ERROR:  function ExecEvalJson not in llvmjit_types.c

I think that it is caused by:

set enable_bitmapscan = off;

(I installed llvm a few days ago. llvm-3.9-dev on this debian stretch).


This is the test sql I concocted, which runs fine with enable_bitmapscan on (the default):

select jt1.* 
from myjsonfile100k as t(js, id)   , json_table(      t.js   , '$' columns (        "lastname"   text    path  '$. "lastname"     '      , "firstname"  text    path  '$. "firstname"    '      , "date"       text    path  '$. "date"         '      , "city"       text    path  '$. "city"         '      , "country"    text    path  '$. "country"      '      , "name 0(1)"  text    path  '$. "array"[0]     '      , "name 4(5)"  text    path  '$. "array"[4]     '      , "names"      text[]  path  '$. "array"        '      , "randfloat"  float   path  '$. "random float" '    )
) as jt1
where  js @> ('[ { "city": "Santiago de Cuba" } ]')   and js[0]->>'firstname' = 'Gilda'
;
ERROR:  function ExecEvalJson not in llvmjit_types.c

That statement only errors out if the table is large enough. I have no time now to make a sample table but if no-one understands the problem off-hand, I'll try to construct such a table later this week (the one I'm using is large, 1.5 GB).

Thank you for testing. 


I think you can try to add 3 missing functions references to the end of 
src/backend/jit/llvm/llvmjit_types.c:

 void       *referenced_functions[] = {
     ...
     ExecEvalXmlExpr,
+    ExecEvalJsonConstructor,
+    ExecEvalIsJsonPredicate,
+    ExecEvalJson,
     MakeExpandedObjectReadOnlyInternal,
     ... };


If this fixes problem, I will add this to the new version of the patches.


-- 
Nikita Glukhov
Postgres Professional: http://www.postgrespro.co
The Russian Postgres Company

> On 2021.03.30. 22:25 Nikita Glukhov <n.gluhov@postgrespro.ru> wrote:
>
>
> On 30.03.2021 19:56, Erik Rijkers wrote:
>
> >> On 2021.03.27. 02:12 Nikita Glukhov <n.gluhov@postgrespro.ru> wrote:
> >>
> >> Attached 47th version of the patches.
> > Hi,
> >
> > Apply, build all fine.  It also works quite well, and according to specification, as far as I can tell.
> >
> > But today I ran into:
> >
> > ERROR:  function ExecEvalJson not in llvmjit_types.c
> >
> > I think that it is caused by:
> >
> > set enable_bitmapscan = off;
> >
> > (I installed llvm a few days ago. llvm-3.9-dev on this debian stretch).
> >
> >
> > This is the test sql I concocted, which runs fine with enable_bitmapscan on (the default):
> >
> > select jt1.*
> > from myjsonfile100k as t(js, id)
> >    , json_table(
> >        t.js
> >     , '$' columns (
> >          "lastname"   text    path  '$. "lastname"     '
> >        , "firstname"  text    path  '$. "firstname"    '
> >        , "date"       text    path  '$. "date"         '
> >        , "city"       text    path  '$. "city"         '
> >        , "country"    text    path  '$. "country"      '
> >        , "name 0(1)"  text    path  '$. "array"[0]     '
> >        , "name 4(5)"  text    path  '$. "array"[4]     '
> >        , "names"      text[]  path  '$. "array"        '
> >        , "randfloat"  float   path  '$. "random float" '
> >      )
> > ) as jt1
> > where  js @> ('[ { "city": "Santiago de Cuba" } ]')
> >     and js[0]->>'firstname' = 'Gilda'
> > ;
> > ERROR:  function ExecEvalJson not in llvmjit_types.c
> >
> > That statement only errors out if the table is large enough. I have no time now to make a sample table but if
no-oneunderstands the problem off-hand, I'll try to construct such a table later this week (the one I'm using is large,
1.5GB). 
>
> Thank you for testing.
>
>
> I think you can try to add 3 missing functions references to the end of
> src/backend/jit/llvm/llvmjit_types.c:
>
>   void       *referenced_functions[] =
>   {
>       ...
>       ExecEvalXmlExpr,
> +    ExecEvalJsonConstructor,
> +    ExecEvalIsJsonPredicate,
> +    ExecEvalJson,
>       MakeExpandedObjectReadOnlyInternal,
>       ...
>   };
>
>
> If this fixes problem, I will add this to the new version of the patches.

It does almost fix it, but in the above is a typo:
+  ExecEvalIsJsonPredicate     should to be changed to:
+  ExecEvalJsonIsPredicate.

With that change the problem vanishes.

Thanks!

Erik Rijkers








>
> --
> Nikita Glukhov
> Postgres Professional:http://www.postgrespro.co   <http://www.postgrespro.com>The Russian Postgres Company

> On 2021.03.27. 02:12 Nikita Glukhov <n.gluhov@postgrespro.ru> wrote:
> Attached 47th version of the patches.

We're past feature freeze for 14 and alas, JSON_TABLE has not made it.

I have tested quite a bit with it and because I didn't find any trouble with functionality or speed, I wanted to at
leastmention that here once.
 

I looked at v47, these files
> [0001-SQL-JSON-functions-v47.patch]
> [0002-JSON_TABLE-v47.patch]
> [0003-JSON_TABLE-PLAN-DEFAULT-clause-v47.patch]
> [0004-JSON_TABLE-PLAN-clause-v47.patch]
> [manual_addition_fixed.patch]  # for this see [1], [2]

   (v47 doesn't apply anymore, as cfbot shows, but instances can still be built on top of 6131ffc43ff from 30 march
2021)

I hope it will fare better next round, version 15.

Thanks,

Erik Rijkers

[1] https://www.postgresql.org/message-id/69eefc5a-cabc-8dd3-c689-93da038c0d6a%40postgrespro.ru
[2] https://www.postgresql.org/message-id/19181987.22943.1617141503618%40webmailclassic.xs4all.nl


> -- 
> Nikita Glukhov
> Postgres Professional: http://www.postgrespro.com
> The Russian Postgres Company

On 4/12/21 11:34 AM, Erik Rijkers wrote:
>> On 2021.03.27. 02:12 Nikita Glukhov <n.gluhov@postgrespro.ru> wrote:
>> Attached 47th version of the patches.
> We're past feature freeze for 14 and alas, JSON_TABLE has not made it.
>
> I have tested quite a bit with it and because I didn't find any trouble with functionality or speed, I wanted to at
leastmention that here once.
 
>
> I looked at v47, these files
>> [0001-SQL-JSON-functions-v47.patch]
>> [0002-JSON_TABLE-v47.patch]
>> [0003-JSON_TABLE-PLAN-DEFAULT-clause-v47.patch]
>> [0004-JSON_TABLE-PLAN-clause-v47.patch]
>> [manual_addition_fixed.patch]  # for this see [1], [2]
>    (v47 doesn't apply anymore, as cfbot shows, but instances can still be built on top of 6131ffc43ff from 30 march
2021)
>
> I hope it will fare better next round, version 15.
>


Me too. Here's a set that should remove the bitrot.


cheers


andrew


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

On 5/8/21 2:23 PM, Andrew Dunstan wrote:
> On 4/12/21 11:34 AM, Erik Rijkers wrote:
>>> On 2021.03.27. 02:12 Nikita Glukhov <n.gluhov@postgrespro.ru> wrote:
>>> Attached 47th version of the patches.
>> We're past feature freeze for 14 and alas, JSON_TABLE has not made it.
>>
>> I have tested quite a bit with it and because I didn't find any trouble with functionality or speed, I wanted to at
leastmention that here once.
 
>>
>> I looked at v47, these files
>>> [0001-SQL-JSON-functions-v47.patch]
>>> [0002-JSON_TABLE-v47.patch]
>>> [0003-JSON_TABLE-PLAN-DEFAULT-clause-v47.patch]
>>> [0004-JSON_TABLE-PLAN-clause-v47.patch]
>>> [manual_addition_fixed.patch]  # for this see [1], [2]
>>    (v47 doesn't apply anymore, as cfbot shows, but instances can still be built on top of 6131ffc43ff from 30 march
2021)
>>
>> I hope it will fare better next round, version 15.
>>
>
> Me too. Here's a set that should remove the bitrot.
>
>

Rebased for removal of serial schedule


cheers


andrew


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

On 5/18/21 9:23 PM, Andrew Dunstan wrote:
> 
> On 5/8/21 2:23 PM, Andrew Dunstan wrote:
>> On 4/12/21 11:34 AM, Erik Rijkers wrote:
>>>> On 2021.03.27. 02:12 Nikita Glukhov <n.gluhov@postgrespro.ru> wrote:
>>>> Attached 47th version of the patches.
>>> We're past feature freeze for 14 and alas, JSON_TABLE has not made it.
>>>
>>> I have tested quite a bit with it and because I didn't find any trouble with functionality or speed, I wanted to at
leastmention that here once.
 
>>>
>>> I looked at v47, these files
>>>> [0001-SQL-JSON-functions-v47.patch]
>>>> [0002-JSON_TABLE-v47.patch]
>>>> [0003-JSON_TABLE-PLAN-DEFAULT-clause-v47.patch]
>>>> [0004-JSON_TABLE-PLAN-clause-v47.patch]
>>>> [manual_addition_fixed.patch]  # for this see [1], [2]
>>>     (v47 doesn't apply anymore, as cfbot shows, but instances can still be built on top of 6131ffc43ff from 30
march2021)
 
>>>
>>> I hope it will fare better next round, version 15.
>>
>> Me too. Here's a set that should remove the bitrot.
> 
> Rebased for removal of serial schedule

Can one of you please add or integrate this patch to the JSON_TABLE changes?


It contains the fix for a bug that I reported earlier (on 2021-03-30 see 
[1]). Nikita did diagnose this fix but today I noticed it was still not 
included in the latest version, v49.


Thanks,

Erik Rijkers


[1] 
https://www.postgresql.org/message-id/2101814418.20240.1617123418368%40webmailclassic.xs4all.nl





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

Hi

Here are the 4 unchanged patches from v49, to which I added 2 patches, 
which are small changes wrt usage of  'JsonIs'  versus  'IsJson'.

That should make the cfbot green again.


Erik Rijkers

Below are a few small comments from a casual read-through.  I noticed that
there was a new version posted after I had finished perusing, but it seems to
address other aspects.

+     Gerenates a column and inserts a composite SQL/JSON
s/Gerenates/Generates/

+      into both child and parrent columns for all missing values.
s/parrent/parent/

-         objectname = "xmltable";
+         objectname = rte->tablefunc ?
+                 rte->tablefunc->functype == TFT_XMLTABLE ?
+                 "xmltable" : "json_table" : NULL;
In which case can rte->tablefunc be NULL for a T_TableFuncScan?  Also, nested
ternary operators are confusing at best, I think this should be rewritten as
plain if statements.

In general when inspecting functype I think it's better to spell it out with if
statements rather than ternary since it allows for grepping the code easier.
Having to grep for TFT_XMLTABLE to find json_table isn't all that convenient.
That also removes the need for comments stating why a ternary operator is Ok in
the first place.

+         errmsg("JSON_TABLE() is not yet implemented for json type"),
I can see this being potentially confusing to many, en errhint with a reference
to jsonb seems like a good idea.

+/* Recursively register column name in the path name list. */
+static void
+registerJsonTableColumn(JsonTableContext *cxt, char *colname)
This comment is misleading since the function isn't actually recursive, but a
helper function for a recursive function.

+        switch (get_typtype(typid))
+        {
+                case TYPTYPE_COMPOSITE:
+                        return true;
+
+                case TYPTYPE_DOMAIN:
+                        return typeIsComposite(getBaseType(typid));
+        }
switch statements without a default runs the risk of attracting unwanted
compiler warning attention, or make static analyzers angry.  This one can
easily be rewritten with two if-statements on a cached get_typtype()
returnvalue.

+ * Returned false at the end of a scan, true otherwise.
s/Returned/Returns/ (applies at two places)

+         /* state->ordinal--; */ /* skip current outer row, reset counter */
Is this dead code to be removed, or left in there as a reminder to fix
something?

--
Daniel Gustafsson        https://vmware.com/

On 7/22/21 3:49 AM, Erik Rijkers wrote:
> Hi
>
> Here are the 4 unchanged patches from v49, to which I added 2 patches,
> which are small changes wrt usage of  'JsonIs'  versus  'IsJson'.
>
> That should make the cfbot green again.



Apparently not, but I have rebased this and the sql/json function patch
set and incorporated your changes in both.


cheers


andrew


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

On 9/2/21 8:52 PM, Andrew Dunstan wrote:
> 
> On 7/22/21 3:49 AM, Erik Rijkers wrote:
>> Hi
>>
>> Here are the 4 unchanged patches from v49, to which I added 2 patches,
>> which are small changes wrt usage of  'JsonIs'  versus  'IsJson'.
>>
>> That should make the cfbot green again.
> 
> 
> Apparently not, but I have rebased this and the sql/json function patch
> set and incorporated your changes in both.


 > [0001-SQL-JSON-functions-v50.patch]
 > [0002-JSON_TABLE-v50.patch]
 > [0003-JSON_TABLE-PLAN-DEFAULT-clause-v50.patch]
 > [0004-JSON_TABLE-PLAN-clause-v50.patch]


These don't apply any more, could you have a look?

Thanks,

Erik Rijkers



(output from gcc 11.2:)

parse_jsontable.c: In function ‘makeStringConst’:
parse_jsontable.c:57:15: error: ‘union ValUnion’ has no member named ‘type’
    57 |         n->val.type = T_String;
       |               ^
parse_jsontable.c:58:16: error: ‘union ValUnion’ has no member named 
‘val’; did you mean ‘ival’?
    58 |         n->val.val.str = str;
       |                ^~~
       |                ival
parse_jsontable.c: In function ‘transformJsonTable’:
parse_jsontable.c:714:61: error: ‘union ValUnion’ has no member named ‘type’
   714 |                 castNode(A_Const, 
jt->common->pathspec)->val.type != T_String)
       |                                                             ^
parse_jsontable.c:721:65: error: ‘union ValUnion’ has no member named 
‘val’; did you mean ‘ival’?
   721 |         rootPath = castNode(A_Const, 
jt->common->pathspec)->val.val.str;
       |                                                                 ^~~
       | 
  ival
make[3]: *** [parse_jsontable.o] Error 1
make[3]: *** Waiting for unfinished jobs....
make[2]: *** [parser-recursive] Error 2
make[2]: *** Waiting for unfinished jobs....
make[1]: *** [all-backend-recurse] Error 2
make: *** [all-src-recurse] Error 2
../../../src/Makefile.global:938: recipe for target 'parse_jsontable.o' 
failed
common.mk:39: recipe for target 'parser-recursive' failed
Makefile:42: recipe for target 'all-backend-recurse' failed
GNUmakefile:11: recipe for target 'all-src-recurse' failed

On 9/13/21 5:41 AM, Erik Rijkers wrote:
> On 9/2/21 8:52 PM, Andrew Dunstan wrote:
>>
>> On 7/22/21 3:49 AM, Erik Rijkers wrote:
>>> Hi
>>>
>>> Here are the 4 unchanged patches from v49, to which I added 2 patches,
>>> which are small changes wrt usage of  'JsonIs'  versus  'IsJson'.
>>>
>>> That should make the cfbot green again.
>>
>>
>> Apparently not, but I have rebased this and the sql/json function patch
>> set and incorporated your changes in both.
>
>
> > [0001-SQL-JSON-functions-v50.patch]
> > [0002-JSON_TABLE-v50.patch]
> > [0003-JSON_TABLE-PLAN-DEFAULT-clause-v50.patch]
> > [0004-JSON_TABLE-PLAN-clause-v50.patch]
>
>
> These don't apply any more, could you have a look?
>
>
>


Yeah, we ran foul of the removal of the Value node type. Here's a rebase.


cheers


andrew

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

On 9/14/21 2:53 PM, Andrew Dunstan wrote:
> On 9/13/21 5:41 AM, Erik Rijkers wrote:
>> On 9/2/21 8:52 PM, Andrew Dunstan wrote:
>>

 >> [0001-SQL-JSON-functions-v51.patch]
 >> [0002-JSON_TABLE-v51.patch]
 >> [0003-JSON_TABLE-PLAN-DEFAULT-clause-v51.patch]
 >> [0004-JSON_TABLE-PLAN-clause-v51.patch]

Thanks, builds fine now.

But I found that the server crashes on certain forms of SQL when 
postgresql.conf has a 'shared_preload_libraries' that contains module 
'pg_stat_statements' (my value was: 
'pg_stat_statements,auth_delay,auto_explain,passwordcheck').  Only 
pg_stat_statements seems to cause the problem.

The offending SQL (I took it from the jsonb_sqljson.sql test file):

testdb=# SELECT JSON_EXISTS(jsonb '{"a": 1, "b": 2}', '$.* ? (@ > $x && 
@ < $y)' PASSING 0 AS x, 2 AS y);
server closed the connection unexpectedly
         This probably means the server terminated abnormally
         before or while processing the request.
The connection to the server was lost. Attempting reset: Failed.
Time: 2.551 ms
!?>

(Of course, that SQL running during regression testing has no problems 
as there is then no pg_stat_statements.)

The statement sometimes succeeds but never very often.

The same crash was there before but I only now saw the connection with 
the 'shared_preload_libraries/pg_stat_statements'.

I seem to remember some things changed in pg_stat_statements but I 
didn't follow and don't know who to CC for it.

Thanks,

Erik Rijkers

On 9/14/21 2:04 PM, Erik Rijkers wrote:
> On 9/14/21 2:53 PM, Andrew Dunstan wrote:
>> On 9/13/21 5:41 AM, Erik Rijkers wrote:
>>> On 9/2/21 8:52 PM, Andrew Dunstan wrote:
>>>
>
> >> [0001-SQL-JSON-functions-v51.patch]
> >> [0002-JSON_TABLE-v51.patch]
> >> [0003-JSON_TABLE-PLAN-DEFAULT-clause-v51.patch]
> >> [0004-JSON_TABLE-PLAN-clause-v51.patch]
>
> Thanks, builds fine now.
>
> But I found that the server crashes on certain forms of SQL when
> postgresql.conf has a 'shared_preload_libraries' that contains module
> 'pg_stat_statements' (my value was:
> 'pg_stat_statements,auth_delay,auto_explain,passwordcheck').  Only
> pg_stat_statements seems to cause the problem.
>
> The offending SQL (I took it from the jsonb_sqljson.sql test file):
>
> testdb=# SELECT JSON_EXISTS(jsonb '{"a": 1, "b": 2}', '$.* ? (@ > $x
> && @ < $y)' PASSING 0 AS x, 2 AS y);
> server closed the connection unexpectedly
>         This probably means the server terminated abnormally
>         before or while processing the request.
> The connection to the server was lost. Attempting reset: Failed.
> Time: 2.551 ms
> !?>
>
> (Of course, that SQL running during regression testing has no problems
> as there is then no pg_stat_statements.)
>
> The statement sometimes succeeds but never very often.
>
> The same crash was there before but I only now saw the connection with
> the 'shared_preload_libraries/pg_stat_statements'.
>
> I seem to remember some things changed in pg_stat_statements but I
> didn't follow and don't know who to CC for it.
>
>


Yeah, I had to make a change in that area, looks like I got it wrong.
I'll follow up. Thanks for the report.


cheers


andrew

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

On 9/14/21 3:18 PM, Andrew Dunstan wrote:
> On 9/14/21 2:04 PM, Erik Rijkers wrote:
>> On 9/14/21 2:53 PM, Andrew Dunstan wrote:
>>> On 9/13/21 5:41 AM, Erik Rijkers wrote:
>>>> On 9/2/21 8:52 PM, Andrew Dunstan wrote:
>>>>
>>>> [0001-SQL-JSON-functions-v51.patch]
>>>> [0002-JSON_TABLE-v51.patch]
>>>> [0003-JSON_TABLE-PLAN-DEFAULT-clause-v51.patch]
>>>> [0004-JSON_TABLE-PLAN-clause-v51.patch]
>> Thanks, builds fine now.
>>
>> But I found that the server crashes on certain forms of SQL when
>> postgresql.conf has a 'shared_preload_libraries' that contains module
>> 'pg_stat_statements' (my value was:
>> 'pg_stat_statements,auth_delay,auto_explain,passwordcheck').  Only
>> pg_stat_statements seems to cause the problem.
>>
>> The offending SQL (I took it from the jsonb_sqljson.sql test file):
>>
>> testdb=# SELECT JSON_EXISTS(jsonb '{"a": 1, "b": 2}', '$.* ? (@ > $x
>> && @ < $y)' PASSING 0 AS x, 2 AS y);
>> server closed the connection unexpectedly
>>         This probably means the server terminated abnormally
>>         before or while processing the request.
>> The connection to the server was lost. Attempting reset: Failed.
>> Time: 2.551 ms
>> !?>
>>
>> (Of course, that SQL running during regression testing has no problems
>> as there is then no pg_stat_statements.)
>>
>> The statement sometimes succeeds but never very often.
>>
>> The same crash was there before but I only now saw the connection with
>> the 'shared_preload_libraries/pg_stat_statements'.
>>
>> I seem to remember some things changed in pg_stat_statements but I
>> didn't follow and don't know who to CC for it.
>>
>>
>
> Yeah, I had to make a change in that area, looks like I got it wrong.
> I'll follow up. Thanks for the report.
>

Rebased and fixed. It's actually an old bug, I reproduced it with a
previous patch set.


cheers


andrew

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

On 9/16/21 10:55, Andrew Dunstan wrote:
> On 9/14/21 3:18 PM, Andrew Dunstan wrote:
>> On 9/14/21 2:04 PM, Erik Rijkers wrote:
>>> On 9/14/21 2:53 PM, Andrew Dunstan wrote:
>>>> On 9/13/21 5:41 AM, Erik Rijkers wrote:
>>>>> On 9/2/21 8:52 PM, Andrew Dunstan wrote:
>>>>>
>>>>> [0001-SQL-JSON-functions-v51.patch]
>>>>> [0002-JSON_TABLE-v51.patch]
>>>>> [0003-JSON_TABLE-PLAN-DEFAULT-clause-v51.patch]
>>>>> [0004-JSON_TABLE-PLAN-clause-v51.patch]
>>> Thanks, builds fine now.
>>>
>>> But I found that the server crashes on certain forms of SQL when
>>> postgresql.conf has a 'shared_preload_libraries' that contains module
>>> 'pg_stat_statements' (my value was:
>>> 'pg_stat_statements,auth_delay,auto_explain,passwordcheck').  Only
>>> pg_stat_statements seems to cause the problem.
>>>
>>> The offending SQL (I took it from the jsonb_sqljson.sql test file):
>>>
>>> testdb=# SELECT JSON_EXISTS(jsonb '{"a": 1, "b": 2}', '$.* ? (@ > $x
>>> && @ < $y)' PASSING 0 AS x, 2 AS y);
>>> server closed the connection unexpectedly
>>>         This probably means the server terminated abnormally
>>>         before or while processing the request.
>>> The connection to the server was lost. Attempting reset: Failed.
>>> Time: 2.551 ms
>>> !?>
>>>
>>> (Of course, that SQL running during regression testing has no problems
>>> as there is then no pg_stat_statements.)
>>>
>>> The statement sometimes succeeds but never very often.
>>>
>>> The same crash was there before but I only now saw the connection with
>>> the 'shared_preload_libraries/pg_stat_statements'.
>>>
>>> I seem to remember some things changed in pg_stat_statements but I
>>> didn't follow and don't know who to CC for it.
>>>
>>>
>> Yeah, I had to make a change in that area, looks like I got it wrong.
>> I'll follow up. Thanks for the report.
>>
> Rebased and fixed. It's actually an old bug, I reproduced it with a
> previous patch set.
>
>

rebased again.


cheers


andrew


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

Hi,

On Tue, Jan 04, 2022 at 09:03:05AM -0500, Andrew Dunstan wrote:
> 
> rebased again.

This version conflicts with recent c4cc2850f4d1 (Rename value node fields).
Can you send a rebased version?

On 1/18/22 01:32, Julien Rouhaud wrote:
> Hi,
>
> On Tue, Jan 04, 2022 at 09:03:05AM -0500, Andrew Dunstan wrote:
>> rebased again.
> This version conflicts with recent c4cc2850f4d1 (Rename value node fields).
> Can you send a rebased version?


attached


cheers


andrew


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

On 1/18/22 16:23, Andrew Dunstan wrote:
> On 1/18/22 01:32, Julien Rouhaud wrote:
>> Hi,
>>
>> On Tue, Jan 04, 2022 at 09:03:05AM -0500, Andrew Dunstan wrote:
>>> rebased again.
>> This version conflicts with recent c4cc2850f4d1 (Rename value node fields).
>> Can you send a rebased version?
>
> attached
>

rebased with some review comments attended to.


cheers


andrew


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

On Wed, Feb 2, 2022 at 12:44 AM Andrew Dunstan <andrew@dunslane.net> wrote:
>
>
> rebased with some review comments attended to.

I am in process of reviewing these patches, initially, have started
with 0002-JSON_TABLE-v55.patch.
Tested many different scenarios with various JSON messages and these
all are working as expected. Just one question on the below output.

‘postgres[1406146]=#’SELECT * FROM JSON_TABLE(jsonb '1', '$' COLUMNS
(a int PATH '$.a' ERROR ON EMPTY)) jt;
 a
---

(1 row)

‘postgres[1406146]=#’SELECT * FROM JSON_TABLE(jsonb '1', '$' COLUMNS
(a int PATH '$.a' ERROR ON ERROR)) jt;
 a
---

(1 row)

is not "ERROR ON ERROR" is expected to give error?

There are a few minor comments on the patch:
1)
Few Typo
+      Sibling columns are joined using
+      <literal>FULL OUTER JOIN ON FALSE</literal>, so that both
parent and child
+      rows are included into the output, with NULL values inserted
+      into both child and parrent columns for all missing values.

Parrent should be parent.

+    <para>
+     Gerenates a column and inserts a boolean item into each row of
this column.
+    </para>
Gerenates should be Generates.

+    <para>
+     Extracts SQL/JSON items from nested levels of the row pattern,
+     gerenates one or more columns as defined by the <literal>COLUMNS</literal>
+     subclause, and inserts the extracted SQL/JSON items into each
row of these columns.
+     The <replaceable>json_table_column</replaceable> expression in the
+     <literal>COLUMNS</literal> subclause uses the same syntax as in the
+     parent <literal>COLUMNS</literal> clause.
+    </para>

Gerenates should be Generates.

+/*-------------------------------------------------------------------------
+ *
+ * parse_jsontable.c
+ *       pasring of JSON_TABLE

pasring should be parsing.

2) Albhabatic include.
+
+#include "postgres.h"
+
+#include "miscadmin.h"
+
include files are not in alphabetic order.

3)
+-- JSON_TABLE: nested paths and plans
+-- Should fail (column names anf path names shall be distinct)
+SELECT * FROM JSON_TABLE(
+       jsonb '[]', '$'
+       COLUMNS (
+               a int,
+               b text,
+               a jsonb
+       )
+) jt;
+ERROR:  duplicate JSON_TABLE column name: a
+HINT:  JSON_TABLE path names and column names shall be distinct from
one another

HINT is not much relevant, can't we simply say "JSON_TABLE column
names should be distinct from one another"?

4)
@@ -4837,6 +4844,7 @@ ExecEvalJsonExprSubtrans(JsonFunc func, ExprEvalStep *op,
        /* Want to execute expressions inside function's memory context */
        MemoryContextSwitchTo(oldcontext);

+

we can remove this empty line.

5)
/*
 * The production for a qualified relation name has to exactly match the
 * production for a qualified func_name, because in a FROM clause we cannot
 * tell which we are parsing until we see what comes after it ('(' for a
 * func_name, something else for a relation). Therefore we allow 'indirection'
 * which may contain subscripts, and reject that case in the C code.
 */

I think the sentence "because in a FROM clause we cannot
 * tell which we are parsing..." should be changed to "because in a
FROM clause we cannot
 * tell what we are parsing "

6)
@@ -696,7 +696,7 @@ transformRangeTableFunc(ParseState *pstate,
RangeTableFunc *rtf)
        char      **names;
        int                     colno;

-       /* Currently only XMLTABLE is supported */

can we change(and not remove) the comment to "/* Currently only
XMLTABLE and JSON_TABLE is supported */"

7)
/*
 * TableFunc - node for a table function, such as XMLTABLE.
 *
 * Entries in the ns_names list are either String nodes containing
 * literal namespace names, or NULL pointers to represent DEFAULT.
 */
typedef struct TableFunc

can we change the comment to "...such as XMLTABLE or JSON_TABLE."?

--
Regards,
Himanshu Upadhyaya
EnterpriseDB: http://www.enterprisedb.com

On 2/9/22 08:22, Himanshu Upadhyaya wrote:
> On Wed, Feb 2, 2022 at 12:44 AM Andrew Dunstan <andrew@dunslane.net> wrote:
>>
>> rebased with some review comments attended to.
> I am in process of reviewing these patches, initially, have started
> with 0002-JSON_TABLE-v55.patch.
> Tested many different scenarios with various JSON messages and these
> all are working as expected. Just one question on the below output.
>
> ‘postgres[1406146]=#’SELECT * FROM JSON_TABLE(jsonb '1', '$' COLUMNS
> (a int PATH '$.a' ERROR ON EMPTY)) jt;
>  a
> ---
>
> (1 row)
>
> ‘postgres[1406146]=#’SELECT * FROM JSON_TABLE(jsonb '1', '$' COLUMNS
> (a int PATH '$.a' ERROR ON ERROR)) jt;
>  a
> ---
>
> (1 row)
>
> is not "ERROR ON ERROR" is expected to give error?
>
> There are a few minor comments on the patch:
> 1)
> Few Typo
> +      Sibling columns are joined using
> +      <literal>FULL OUTER JOIN ON FALSE</literal>, so that both
> parent and child
> +      rows are included into the output, with NULL values inserted
> +      into both child and parrent columns for all missing values.
>
> Parrent should be parent.
>
> +    <para>
> +     Gerenates a column and inserts a boolean item into each row of
> this column.
> +    </para>
> Gerenates should be Generates.
>
> +    <para>
> +     Extracts SQL/JSON items from nested levels of the row pattern,
> +     gerenates one or more columns as defined by the <literal>COLUMNS</literal>
> +     subclause, and inserts the extracted SQL/JSON items into each
> row of these columns.
> +     The <replaceable>json_table_column</replaceable> expression in the
> +     <literal>COLUMNS</literal> subclause uses the same syntax as in the
> +     parent <literal>COLUMNS</literal> clause.
> +    </para>
>
> Gerenates should be Generates.
>
> +/*-------------------------------------------------------------------------
> + *
> + * parse_jsontable.c
> + *       pasring of JSON_TABLE
>
> pasring should be parsing.
>
> 2) Albhabatic include.
> +
> +#include "postgres.h"
> +
> +#include "miscadmin.h"
> +
> include files are not in alphabetic order.
>
> 3)
> +-- JSON_TABLE: nested paths and plans
> +-- Should fail (column names anf path names shall be distinct)
> +SELECT * FROM JSON_TABLE(
> +       jsonb '[]', '$'
> +       COLUMNS (
> +               a int,
> +               b text,
> +               a jsonb
> +       )
> +) jt;
> +ERROR:  duplicate JSON_TABLE column name: a
> +HINT:  JSON_TABLE path names and column names shall be distinct from
> one another
>
> HINT is not much relevant, can't we simply say "JSON_TABLE column
> names should be distinct from one another"?
>
> 4)
> @@ -4837,6 +4844,7 @@ ExecEvalJsonExprSubtrans(JsonFunc func, ExprEvalStep *op,
>         /* Want to execute expressions inside function's memory context */
>         MemoryContextSwitchTo(oldcontext);
>
> +
>
> we can remove this empty line.
>
> 5)
> /*
>  * The production for a qualified relation name has to exactly match the
>  * production for a qualified func_name, because in a FROM clause we cannot
>  * tell which we are parsing until we see what comes after it ('(' for a
>  * func_name, something else for a relation). Therefore we allow 'indirection'
>  * which may contain subscripts, and reject that case in the C code.
>  */
>
> I think the sentence "because in a FROM clause we cannot
>  * tell which we are parsing..." should be changed to "because in a
> FROM clause we cannot
>  * tell what we are parsing "
>
> 6)
> @@ -696,7 +696,7 @@ transformRangeTableFunc(ParseState *pstate,
> RangeTableFunc *rtf)
>         char      **names;
>         int                     colno;
>
> -       /* Currently only XMLTABLE is supported */
>
> can we change(and not remove) the comment to "/* Currently only
> XMLTABLE and JSON_TABLE is supported */"
>
> 7)
> /*
>  * TableFunc - node for a table function, such as XMLTABLE.
>  *
>  * Entries in the ns_names list are either String nodes containing
>  * literal namespace names, or NULL pointers to represent DEFAULT.
>  */
> typedef struct TableFunc
>
> can we change the comment to "...such as XMLTABLE or JSON_TABLE."?
>


This set of patches deals with items 1..7 above, but not yet the ERROR
ON ERROR issue. It also makes some message cleanups, but there is more
to come in that area.

It is based on the latest SQL/JSON Functions patch set, which does not
include the sql_json GUC patch.


cheers


andrew


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

Op 04-03-2022 om 17:33 schreef Andrew Dunstan:
> 

> This set of patches deals with items 1..7 above, but not yet the ERROR
> ON ERROR issue. It also makes some message cleanups, but there is more
> to come in that area.
> 
> It is based on the latest SQL/JSON Functions patch set, which does not
> include the sql_json GUC patch.
> 
 > [0001-SQL-JSON-functions-without-sql_json-GUC-v56.patch]
 > [0002-JSON_TABLE-v56.patch]
 > [0003-JSON_TABLE-PLAN-DEFAULT-clause-v56.patch]
 > [0004-JSON_TABLE-PLAN-clause-v56.patch]

Hi,

I quickly tried the tests I already had and there are two statements 
that stopped working:

testdb=# SELECT JSON('{"a": 123, "b": [true, "foo"], "a2": "bar"}' 
RETURNING jsonb);
ERROR:  syntax error at or near "RETURNING"
LINE 1: ...SON('{"a": 123, "b": [true, "foo"], "a2": "bar"}' RETURNING ...

testdb=# select JSON_SCALAR(123.45 returning jsonb);
ERROR:  syntax error at or near "returning"
LINE 1: select JSON_SCALAR(123.45 returning jsonb)

   (the '^' pointer in both cases underneath 'RETURNING'


thanks,

Erik Rijkers





> 
> cheers
> 
> 
> andrew
> 
> 
> --
> Andrew Dunstan
> EDB: https://www.enterprisedb.com

On 3/4/22 13:13, Erikjan Rijkers wrote:
> Op 04-03-2022 om 17:33 schreef Andrew Dunstan:
>>
>
>> This set of patches deals with items 1..7 above, but not yet the ERROR
>> ON ERROR issue. It also makes some message cleanups, but there is more
>> to come in that area.
>>
>> It is based on the latest SQL/JSON Functions patch set, which does not
>> include the sql_json GUC patch.
>>
> > [0001-SQL-JSON-functions-without-sql_json-GUC-v56.patch]
> > [0002-JSON_TABLE-v56.patch]
> > [0003-JSON_TABLE-PLAN-DEFAULT-clause-v56.patch]
> > [0004-JSON_TABLE-PLAN-clause-v56.patch]
>
> Hi,
>
> I quickly tried the tests I already had and there are two statements
> that stopped working:
>
> testdb=# SELECT JSON('{"a": 123, "b": [true, "foo"], "a2": "bar"}'
> RETURNING jsonb);
> ERROR:  syntax error at or near "RETURNING"
> LINE 1: ...SON('{"a": 123, "b": [true, "foo"], "a2": "bar"}' RETURNING
> ...
>
> testdb=# select JSON_SCALAR(123.45 returning jsonb);
> ERROR:  syntax error at or near "returning"
> LINE 1: select JSON_SCALAR(123.45 returning jsonb)
>
>   (the '^' pointer in both cases underneath 'RETURNING'
>
>
>


Yes, you're right, that was implemented as part of the GUC patch. I'll
try to split that out and send new patchsets with the RETURNING clause
but without the GUC (see upthread for reasons)


cheers


andrew

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

On 3/4/22 15:05, Andrew Dunstan wrote:
> On 3/4/22 13:13, Erikjan Rijkers wrote:
>> Op 04-03-2022 om 17:33 schreef Andrew Dunstan:
>>> This set of patches deals with items 1..7 above, but not yet the ERROR
>>> ON ERROR issue. It also makes some message cleanups, but there is more
>>> to come in that area.
>>>
>>> It is based on the latest SQL/JSON Functions patch set, which does not
>>> include the sql_json GUC patch.
>>>
>>> [0001-SQL-JSON-functions-without-sql_json-GUC-v56.patch]
>>> [0002-JSON_TABLE-v56.patch]
>>> [0003-JSON_TABLE-PLAN-DEFAULT-clause-v56.patch]
>>> [0004-JSON_TABLE-PLAN-clause-v56.patch]
>> Hi,
>>
>> I quickly tried the tests I already had and there are two statements
>> that stopped working:
>>
>> testdb=# SELECT JSON('{"a": 123, "b": [true, "foo"], "a2": "bar"}'
>> RETURNING jsonb);
>> ERROR:  syntax error at or near "RETURNING"
>> LINE 1: ...SON('{"a": 123, "b": [true, "foo"], "a2": "bar"}' RETURNING
>> ...
>>
>> testdb=# select JSON_SCALAR(123.45 returning jsonb);
>> ERROR:  syntax error at or near "returning"
>> LINE 1: select JSON_SCALAR(123.45 returning jsonb)
>>
>>   (the '^' pointer in both cases underneath 'RETURNING'
>>
>>
>>
>
> Yes, you're right, that was implemented as part of the GUC patch. I'll
> try to split that out and send new patchsets with the RETURNING clause
> but without the GUC (see upthread for reasons)
>
>

Here's a patchset with RETURNING for JSON() and JSON_SCALAR() but
without the GUC


cheers


andrew


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

On 2/9/22 08:22, Himanshu Upadhyaya wrote:
> On Wed, Feb 2, 2022 at 12:44 AM Andrew Dunstan <andrew@dunslane.net> wrote:
>>
>> rebased with some review comments attended to.
> I am in process of reviewing these patches, initially, have started
> with 0002-JSON_TABLE-v55.patch.
> Tested many different scenarios with various JSON messages and these
> all are working as expected. Just one question on the below output.
>
> ‘postgres[1406146]=#’SELECT * FROM JSON_TABLE(jsonb '1', '$' COLUMNS
> (a int PATH '$.a' ERROR ON EMPTY)) jt;
>  a
> ---
>
> (1 row)
>
> ‘postgres[1406146]=#’SELECT * FROM JSON_TABLE(jsonb '1', '$' COLUMNS
> (a int PATH '$.a' ERROR ON ERROR)) jt;
>  a
> ---
>
> (1 row)
>
> is not "ERROR ON ERROR" is expected to give error?


I think I understand what's going on here. In the first example 'ERROR
ON EMPTY' causes an error condition, but as the default action for an
error condition is to return null that's what happens. To get an error
raised you would need to say 'ERROR ON EMPTY ERROR ON ERROR'. I don't
know if that's according to spec. It seems kinda screwy, arguably a POLA
violation, although that would hardly be a first for the SQL Standards
body.  But I'm speculating here, I'm not a standards lawyer.

In the second case it looks like there isn't really an error. There
would be if you used 'strict' in the path expression.


This whole area needs more documentation.


cheers


andrew

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

On 3/22/22 09:28, Oleg Bartunov wrote:
>
>
> On Tue, Mar 22, 2022 at 12:53 PM Matthias Kurz <m.kurz@irregular.at>
> wrote:
>
>     Hi everyone!
>
>     I am watching this thread since quite a while and I am waiting
>     eagerly a long time already that this feature finally lands in
>     PostgreSQL.
>     Given that in around 2 weeks PostgreSQL 15 will go into feature
>     freeze (in the last years that usually happened around the 8th of
>     April AFAIK), is there any chance this will be committed? As far
>     as I understand the patches are almost ready.
>
>
> We are waiting too :)


I'm planning on pushing the functions patch set this week and json-table
next week.


cheers


andrew


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

On 2022-Mar-22, Andrew Dunstan wrote:

> I'm planning on pushing the functions patch set this week and json-table
> next week.

I think it'd be a good idea to push the patches one by one and let a day
between each.  That would make it easier to chase buildfarm issues
individually, and make sure they are fixed before the next step.
Each patch in each series is already big enough.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/

> On 22 Mar 2022, at 16:31, Andrew Dunstan <andrew@dunslane.net> wrote:

> I'm planning on pushing the functions patch set this week and json-table
> next week.

My comments from 30827B3C-EDF6-4D41-BBF1-2981818744A8@yesql.se are yet to be
addressed (or at all responded to) in this patchset.  I'll paste the ones which
still apply to make it easier:

+ into both child and parrent columns for all missing values.
s/parrent/parent/

-           objectname = "xmltable";
+           objectname = rte->tablefunc ?
+               rte->tablefunc->functype == TFT_XMLTABLE ?
+               "xmltable" : "json_table" : NULL;
In which case can rte->tablefunc be NULL for a T_TableFuncScan? Also, nested
ternary operators are confusing at best, I think this should be rewritten as
plain if statements.

In general when inspecting functype I think it's better to spell it out with if
statements rather than ternary since it allows for grepping the code easier.
Having to grep for TFT_XMLTABLE to find where TFT_JSON_TABLE could be used
isn't all that convenient.

+ errmsg("JSON_TABLE() is not yet implemented for json type"),
I can see this being potentially confusing to many, en errhint with a reference
to jsonb seems like a good idea.

+/* Recursively register column name in the path name list. */
+static void
+registerJsonTableColumn(JsonTableContext *cxt, char *colname)
This comment is IMO misleading since the function isn't actually recursive, but a
helper function for a recursive function.

+   switch (get_typtype(typid))
+   {
+       case TYPTYPE_COMPOSITE:
+           return true;
+
+       case TYPTYPE_DOMAIN:
+           return typeIsComposite(getBaseType(typid));
+   }
switch statements without a default runs the risk of attracting unwanted
compiler warning attention, or make static analyzers angry. This one can
easily be rewritten with two if-statements on a cached get_typtype()
returnvalue.

+ * Returned false at the end of a scan, true otherwise.
s/Returned/Returns/ (applies at two places)

+ /* state->ordinal--; */ /* skip current outer row, reset counter */
Is this dead code to be removed, or left in there as a reminder to fix
something?

--
Daniel Gustafsson        https://vmware.com/

On 3/22/22 10:53, Alvaro Herrera wrote:
> On 2022-Mar-22, Andrew Dunstan wrote:
>
>> I'm planning on pushing the functions patch set this week and json-table
>> next week.
> I think it'd be a good idea to push the patches one by one and let a day
> between each.  That would make it easier to chase buildfarm issues
> individually, and make sure they are fixed before the next step.
> Each patch in each series is already big enough.



OK, can do it that way. First one will be later today then.


cheers


andrew


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

On 3/22/22 10:55, Daniel Gustafsson wrote:
>> On 22 Mar 2022, at 16:31, Andrew Dunstan <andrew@dunslane.net> wrote:
>> I'm planning on pushing the functions patch set this week and json-table
>> next week.
> My comments from 30827B3C-EDF6-4D41-BBF1-2981818744A8@yesql.se are yet to be
> addressed (or at all responded to) in this patchset.  I'll paste the ones which
> still apply to make it easier:


Thanks for reminding me. I will make sure these are attended to.


cheers


andrew



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

On 3/22/22 11:27, Andrew Dunstan wrote:
> On 3/22/22 10:53, Alvaro Herrera wrote:
>> On 2022-Mar-22, Andrew Dunstan wrote:
>>
>>> I'm planning on pushing the functions patch set this week and json-table
>>> next week.
>> I think it'd be a good idea to push the patches one by one and let a day
>> between each.  That would make it easier to chase buildfarm issues
>> individually, and make sure they are fixed before the next step.
>> Each patch in each series is already big enough.
>
>
> OK, can do it that way. First one will be later today then.
>
>

OK, pushed the first of the functions patches. That means the cfbot will
break on these two CF items, but it's way too much trouble to have to
remake the patch sets every day, so we can just live without the cfbot
on these for a bit. I will of course test before committing and fix any
bitrot.


cheers


andrew


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

On 3/5/22 09:35, Andrew Dunstan wrote:
> On 3/4/22 15:05, Andrew Dunstan wrote:
>> On 3/4/22 13:13, Erikjan Rijkers wrote:
>>> Op 04-03-2022 om 17:33 schreef Andrew Dunstan:
>>>> This set of patches deals with items 1..7 above, but not yet the ERROR
>>>> ON ERROR issue. It also makes some message cleanups, but there is more
>>>> to come in that area.
>>>>
>>>> It is based on the latest SQL/JSON Functions patch set, which does not
>>>> include the sql_json GUC patch.
>>>>
>>>> [0001-SQL-JSON-functions-without-sql_json-GUC-v56.patch]
>>>> [0002-JSON_TABLE-v56.patch]
>>>> [0003-JSON_TABLE-PLAN-DEFAULT-clause-v56.patch]
>>>> [0004-JSON_TABLE-PLAN-clause-v56.patch]
>>> Hi,
>>>
>>> I quickly tried the tests I already had and there are two statements
>>> that stopped working:
>>>
>>> testdb=# SELECT JSON('{"a": 123, "b": [true, "foo"], "a2": "bar"}'
>>> RETURNING jsonb);
>>> ERROR:  syntax error at or near "RETURNING"
>>> LINE 1: ...SON('{"a": 123, "b": [true, "foo"], "a2": "bar"}' RETURNING
>>> ...
>>>
>>> testdb=# select JSON_SCALAR(123.45 returning jsonb);
>>> ERROR:  syntax error at or near "returning"
>>> LINE 1: select JSON_SCALAR(123.45 returning jsonb)
>>>
>>>   (the '^' pointer in both cases underneath 'RETURNING'
>>>
>>>
>>>
>> Yes, you're right, that was implemented as part of the GUC patch. I'll
>> try to split that out and send new patchsets with the RETURNING clause
>> but without the GUC (see upthread for reasons)
>>
>>
> Here's a patchset with RETURNING for JSON() and JSON_SCALAR() but
> without the GUC
>

Here's a new set with the latest sql/json functions patch and fixes for
some further node handling  inadequacies.


cheers


andrew


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

On 3/22/22 10:55, Daniel Gustafsson wrote:
>> On 22 Mar 2022, at 16:31, Andrew Dunstan <andrew@dunslane.net> wrote:
>> I'm planning on pushing the functions patch set this week and json-table
>> next week.
> My comments from 30827B3C-EDF6-4D41-BBF1-2981818744A8@yesql.se are yet to be
> addressed (or at all responded to) in this patchset.  I'll paste the ones which
> still apply to make it easier:
>


I think I have fixed all those. See attached. I haven't prepared a new
patch set for SQL/JSON functions because there's just one typo to fix,
but I won't forget it. Please let me know if there's anything else you see.


At this stage I think I have finished with the actual code, and I'm
concentrating on improving the docs a bit.


cheers


andrew


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

Op 25-03-2022 om 21:30 schreef Andrew Dunstan:
> 
> On 3/22/22 10:55, Daniel Gustafsson wrote:
>>> On 22 Mar 2022, at 16:31, Andrew Dunstan <andrew@dunslane.net> wrote:
>>> I'm planning on pushing the functions patch set this week and json-table
>>> next week.
>> My comments from 30827B3C-EDF6-4D41-BBF1-2981818744A8@yesql.se are yet to be
>> addressed (or at all responded to) in this patchset.  I'll paste the ones which
>> still apply to make it easier:
>>
> 
> 
> I think I have fixed all those. See attached. I haven't prepared a new
> patch set for SQL/JSON functions because there's just one typo to fix,
> but I won't forget it. Please let me know if there's anything else you see.
> 
> 
> At this stage I think I have finished with the actual code, and I'm
> concentrating on improving the docs a bit.

 > [ v59 ]


FWIW, I went through func.sgml (of v59) once.


Erik Rijkers

On 3/26/22 07:29, Erik Rijkers wrote:
> Op 25-03-2022 om 21:30 schreef Andrew Dunstan:
>>
>> On 3/22/22 10:55, Daniel Gustafsson wrote:
>>>> On 22 Mar 2022, at 16:31, Andrew Dunstan <andrew@dunslane.net> wrote:
>>>> I'm planning on pushing the functions patch set this week and
>>>> json-table
>>>> next week.
>>> My comments from 30827B3C-EDF6-4D41-BBF1-2981818744A8@yesql.se are
>>> yet to be
>>> addressed (or at all responded to) in this patchset.  I'll paste the
>>> ones which
>>> still apply to make it easier:
>>>
>>
>>
>> I think I have fixed all those. See attached. I haven't prepared a new
>> patch set for SQL/JSON functions because there's just one typo to fix,
>> but I won't forget it. Please let me know if there's anything else
>> you see.
>>
>>
>> At this stage I think I have finished with the actual code, and I'm
>> concentrating on improving the docs a bit.
>
> > [ v59 ]
>
>
> FWIW, I went through func.sgml (of v59) once.
>
>
>


Thanks, That will help.


cheers


andrew


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

On 3/24/22 18:54, Andrew Dunstan wrote:
> On 3/5/22 09:35, Andrew Dunstan wrote:
>> On 3/4/22 15:05, Andrew Dunstan wrote:
>>> On 3/4/22 13:13, Erikjan Rijkers wrote:
>>>> Op 04-03-2022 om 17:33 schreef Andrew Dunstan:
>>>>> This set of patches deals with items 1..7 above, but not yet the ERROR
>>>>> ON ERROR issue. It also makes some message cleanups, but there is more
>>>>> to come in that area.
>>>>>
>>>>> It is based on the latest SQL/JSON Functions patch set, which does not
>>>>> include the sql_json GUC patch.
>>>>>
>>>>> [0001-SQL-JSON-functions-without-sql_json-GUC-v56.patch]
>>>>> [0002-JSON_TABLE-v56.patch]
>>>>> [0003-JSON_TABLE-PLAN-DEFAULT-clause-v56.patch]
>>>>> [0004-JSON_TABLE-PLAN-clause-v56.patch]
>>>> Hi,
>>>>
>>>> I quickly tried the tests I already had and there are two statements
>>>> that stopped working:
>>>>
>>>> testdb=# SELECT JSON('{"a": 123, "b": [true, "foo"], "a2": "bar"}'
>>>> RETURNING jsonb);
>>>> ERROR:  syntax error at or near "RETURNING"
>>>> LINE 1: ...SON('{"a": 123, "b": [true, "foo"], "a2": "bar"}' RETURNING
>>>> ...
>>>>
>>>> testdb=# select JSON_SCALAR(123.45 returning jsonb);
>>>> ERROR:  syntax error at or near "returning"
>>>> LINE 1: select JSON_SCALAR(123.45 returning jsonb)
>>>>
>>>>   (the '^' pointer in both cases underneath 'RETURNING'
>>>>
>>>>
>>>>
>>> Yes, you're right, that was implemented as part of the GUC patch. I'll
>>> try to split that out and send new patchsets with the RETURNING clause
>>> but without the GUC (see upthread for reasons)
>>>
>>>
>> Here's a patchset with RETURNING for JSON() and JSON_SCALAR() but
>> without the GUC
>>
> Here's a new set with the latest sql/json functions patch and fixes for
> some further node handling  inadequacies.
>
>


I have been wrestling with the docs in these patches, which are somewhat
haphazardly spread across the various patches, and tying myself up in
knots. Fixing them so I don't cause later merge pain is difficult. I'm
therefore going to commit this series (staggered as previously
requested) without documentation and then commit a consolidated
documentation patch for them.


cheers


andrew

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

FYI I think the patch failure in the cfbot is spurious because the
cfbot got confused by Erik's patch.

On 3/28/22 15:48, Greg Stark wrote:
> FYI I think the patch failure in the cfbot is spurious because the
> cfbot got confused by Erik's patch.



The cfbot is likely to be confused until I am finished committing the
SQL/JSON patches. Just disregard it.


cheers


andrew


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

Hi,

On 2022-03-27 16:53:57 -0400, Andrew Dunstan wrote:
> I'm therefore going to commit this series

The new jsonb_sqljson test is, on my machine, the slowest test in the main
regression tests:

4639 ms jsonb_sqljson
2401 ms btree_index
2166 ms stats_ext
2027 ms alter_table
1616 ms triggers
1498 ms brin
1489 ms join_hash
1367 ms foreign_key
1345 ms tuplesort
1202 ms plpgsql

Any chance the slowness isn't required slowness?

Greetings,

Andres Freund

Andres Freund <andres@anarazel.de> writes:
> The new jsonb_sqljson test is, on my machine, the slowest test in the main
> regression tests:
> ...
> Any chance the slowness isn't required slowness?

In general, there's been a serious bump in the buildfarm cycle
time in the last month --- for example, on my admittedly slow
animal prairiedog, the cycle time excluding the "make" phase
(which is really variable because ccache) has gone from about
4:26 a month ago to 5:25 in its last run.

I don't want to worry about this before feature freeze, but after that
we should take a hard look at cutting out unnecessary test cycles.

            regards, tom lane

On 4/5/22 22:21, Andres Freund wrote:
> Hi,
>
> On 2022-03-27 16:53:57 -0400, Andrew Dunstan wrote:
>> I'm therefore going to commit this series
> The new jsonb_sqljson test is, on my machine, the slowest test in the main
> regression tests:
>
> 4639 ms jsonb_sqljson
> 2401 ms btree_index
> 2166 ms stats_ext
> 2027 ms alter_table
> 1616 ms triggers
> 1498 ms brin
> 1489 ms join_hash
> 1367 ms foreign_key
> 1345 ms tuplesort
> 1202 ms plpgsql
>
> Any chance the slowness isn't required slowness?



I'll take a look.


cheers


andrew


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

On 4/6/22 09:20, Andrew Dunstan wrote:
> On 4/5/22 22:21, Andres Freund wrote:
>> Hi,
>>
>> On 2022-03-27 16:53:57 -0400, Andrew Dunstan wrote:
>>> I'm therefore going to commit this series
>> The new jsonb_sqljson test is, on my machine, the slowest test in the main
>> regression tests:
>>
>> 4639 ms jsonb_sqljson
>> 2401 ms btree_index
>> 2166 ms stats_ext
>> 2027 ms alter_table
>> 1616 ms triggers
>> 1498 ms brin
>> 1489 ms join_hash
>> 1367 ms foreign_key
>> 1345 ms tuplesort
>> 1202 ms plpgsql
>>
>> Any chance the slowness isn't required slowness?
>
>
> I'll take a look.
>
>

I've committed a change that should reduce it substantially, but there
might be more work to do.


cheers


andrew


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

Greetings,

* Andrew Dunstan (andrew@dunslane.net) wrote:
> On 4/6/22 09:20, Andrew Dunstan wrote:
> > On 4/5/22 22:21, Andres Freund wrote:
> >> On 2022-03-27 16:53:57 -0400, Andrew Dunstan wrote:
> >>> I'm therefore going to commit this series
> >> The new jsonb_sqljson test is, on my machine, the slowest test in the main
> >> regression tests:
> >>
> >> 4639 ms jsonb_sqljson
> >> 2401 ms btree_index
> >> 2166 ms stats_ext
> >> 2027 ms alter_table
> >> 1616 ms triggers
> >> 1498 ms brin
> >> 1489 ms join_hash
> >> 1367 ms foreign_key
> >> 1345 ms tuplesort
> >> 1202 ms plpgsql
> >>
> >> Any chance the slowness isn't required slowness?
> >
> >
> > I'll take a look.
>
> I've committed a change that should reduce it substantially, but there
> might be more work to do.

All for improving the speed, but this broke the recovery tests (as
noticed by the buildfarm).  Maybe we should add
--no-unlogged-table-data to those pg_dumpall runs?

Thanks,

Stephen

On 4/6/22 11:11, Stephen Frost wrote:
> Greetings,
>
> * Andrew Dunstan (andrew@dunslane.net) wrote:
>> On 4/6/22 09:20, Andrew Dunstan wrote:
>>> On 4/5/22 22:21, Andres Freund wrote:
>>>> On 2022-03-27 16:53:57 -0400, Andrew Dunstan wrote:
>>>>> I'm therefore going to commit this series
>>>> The new jsonb_sqljson test is, on my machine, the slowest test in the main
>>>> regression tests:
>>>>
>>>> 4639 ms jsonb_sqljson
>>>> 2401 ms btree_index
>>>> 2166 ms stats_ext
>>>> 2027 ms alter_table
>>>> 1616 ms triggers
>>>> 1498 ms brin
>>>> 1489 ms join_hash
>>>> 1367 ms foreign_key
>>>> 1345 ms tuplesort
>>>> 1202 ms plpgsql
>>>>
>>>> Any chance the slowness isn't required slowness?
>>>
>>> I'll take a look.
>> I've committed a change that should reduce it substantially, but there
>> might be more work to do.
> All for improving the speed, but this broke the recovery tests (as
> noticed by the buildfarm).  Maybe we should add
> --no-unlogged-table-data to those pg_dumpall runs?



I think we should, but I think here the obvious solution is to drop the
table when we're done with it. I'll test that.


cheers


andrew


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

On 4/6/22 11:33, Andrew Dunstan wrote:
> On 4/6/22 11:11, Stephen Frost wrote:
>> Greetings,
>>
>> * Andrew Dunstan (andrew@dunslane.net) wrote:
>>> On 4/6/22 09:20, Andrew Dunstan wrote:
>>>> On 4/5/22 22:21, Andres Freund wrote:
>>>>> On 2022-03-27 16:53:57 -0400, Andrew Dunstan wrote:
>>>>>> I'm therefore going to commit this series
>>>>> The new jsonb_sqljson test is, on my machine, the slowest test in the main
>>>>> regression tests:
>>>>>
>>>>> 4639 ms jsonb_sqljson
>>>>> 2401 ms btree_index
>>>>> 2166 ms stats_ext
>>>>> 2027 ms alter_table
>>>>> 1616 ms triggers
>>>>> 1498 ms brin
>>>>> 1489 ms join_hash
>>>>> 1367 ms foreign_key
>>>>> 1345 ms tuplesort
>>>>> 1202 ms plpgsql
>>>>>
>>>>> Any chance the slowness isn't required slowness?
>>>> I'll take a look.
>>> I've committed a change that should reduce it substantially, but there
>>> might be more work to do.
>> All for improving the speed, but this broke the recovery tests (as
>> noticed by the buildfarm).  Maybe we should add
>> --no-unlogged-table-data to those pg_dumpall runs?
>
>
> I think we should, but I think here the obvious solution is to drop the
> table when we're done with it. I'll test that.
>
>

It does work, but Tom prefers not to have the test at all, so I'll just
rip it out.


cheers


andrew

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

Andrew Dunstan <andrew@dunslane.net> writes:
>> I think we should, but I think here the obvious solution is to drop the
>> table when we're done with it. I'll test that.

> It does work, but Tom prefers not to have the test at all, so I'll just
> rip it out.

Perhaps moving it to some other place (test/modules/something?) would
be appropriate.  But I don't think that the core regression tests,
which are currently run four or more times per buildfarm cycle,
are an appropriate place for million-row test cases.

            regards, tom lane

Hi,

On 2022-04-06 11:50:11 -0400, Andrew Dunstan wrote:
> It does work, but Tom prefers not to have the test at all, so I'll just
> rip it out.

If I understand correctly the reason a large table is needed is to test
parallelism, right? Wouldn't the better fix be to just tweak the parallelism
settings for that table? See select_parallel.sql:

-- encourage use of parallel plans
set parallel_setup_cost=0;
set parallel_tuple_cost=0;
set min_parallel_table_scan_size=0;
set max_parallel_workers_per_gather=4;

might be worth also setting
set parallel_leader_participation = off;

to avoid the leader processing everything before workers have even started up.

Greetings,

Andres Freund

On 4/6/22 12:59, Andres Freund wrote:
> Hi,
>
> On 2022-04-06 11:50:11 -0400, Andrew Dunstan wrote:
>> It does work, but Tom prefers not to have the test at all, so I'll just
>> rip it out.
> If I understand correctly the reason a large table is needed is to test
> parallelism, right? Wouldn't the better fix be to just tweak the parallelism
> settings for that table? See select_parallel.sql:
>
> -- encourage use of parallel plans
> set parallel_setup_cost=0;
> set parallel_tuple_cost=0;
> set min_parallel_table_scan_size=0;
> set max_parallel_workers_per_gather=4;
>
> might be worth also setting
> set parallel_leader_participation = off;
>
> to avoid the leader processing everything before workers have even started up.
>

OK, done that way, thanks. I also kept the table as unlogged and dropped
it at the end.


cheers


andrew


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

Hi,

On 2022-04-06 11:11:42 -0400, Stephen Frost wrote:
> Maybe we should add --no-unlogged-table-data to those pg_dumpall runs?

Yes, I think we should. And then we should explicitly add an unlogged table
that isn't dropped. That way we get pg_upgrade testing etc.

Thomas, what do you think?

Greetings,

Andres Freund