Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA1:256) (Exim 4.89) (envelope-from ) id 1iA11m-0008Er-Vu for pgsql-hackers@arkaria.postgresql.org; Tue, 17 Sep 2019 00:07:31 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.89) (envelope-from ) id 1iA11k-0005xJ-Nr for pgsql-hackers@arkaria.postgresql.org; Tue, 17 Sep 2019 00:07:28 +0000 Received: from magus.postgresql.org ([2a02:c0:301:0:ffff::29]) by malur.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA1:256) (Exim 4.89) (envelope-from ) id 1iA11k-0005wz-Bq for pgsql-hackers@lists.postgresql.org; Tue, 17 Sep 2019 00:07:28 +0000 Received: from cyclops.postgrespro.ru ([93.174.131.138] helo=mail.postgrespro.ru) by magus.postgresql.org with esmtp (Exim 4.89) (envelope-from ) id 1iA11g-0005yO-72 for pgsql-hackers@postgresql.org; Tue, 17 Sep 2019 00:07:27 +0000 Received: from localhost (localhost [127.0.0.1]) by mail.postgrespro.ru (Postfix) with ESMTP id 5B18121C3F98; Tue, 17 Sep 2019 03:07:23 +0300 (MSK) X-Virus-Scanned: Debian amavisd-new at postgrespro.ru X-Spam-Flag: NO X-Spam-Score: 0 X-Spam-Level: X-Spam-Status: No, score=x tagged_above=-99 required=4 WHITELISTED tests=[] autolearn=unavailable Received: from [10.100.236.30] (unknown [195.82.154.27]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (Client did not present a certificate) by mail.postgrespro.ru (Postfix) with ESMTPSA id C095E21C3F7D; Tue, 17 Sep 2019 03:07:22 +0300 (MSK) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=postgrespro.ru; s=mail; t=1568678843; bh=Lyqksy0Akh/ugQbW2ogbU29JQUxZVYAMdYa9uxncsbI=; h=Subject:To:Cc:References:From:Date:In-Reply-To; b=OhewAmNzZs/Zg2uWuGRJLMHC9vU+sOi7N+V4dVFFm62ZhvXigsxlZN3fC6H/D7B2e ISfPxI5eflVUDZGgqKOqZfKEBJOFv/Pi/2qSwtU3WRHfPKyHKW27FJwxE1yCB4LTId gVOYUjK2B3h9h+1mKiMwPyAoL/6SnWmDyBv+Jubo= Subject: Re: SQL/JSON: JSON_TABLE To: Pavel Stehule , PostgreSQL-development Cc: Robert Haas , Dmitry Dolgov <9erthalion6@gmail.com>, Oleg Bartunov , Andrew Dunstan References: <132f26c4-dfc6-f8fd-4764-2cbf455a3aec@postgrespro.ru> <0f26e0d7-fbf9-0bbe-7fe2-4237d2538e5a@postgrespro.ru> <3e32fbfd-d046-8f02-24e2-c766415eb5f2@postgrespro.ru> <78894d50-010d-0876-23cb-55cba86404e2@postgrespro.ru> <5ded9f9d-cf93-5bd6-a4c3-ad8937245ced@postgrespro.ru> <6f5255ce-ae02-d213-49aa-7b86004ab080@postgrespro.ru> <7f23e8bd-d9fa-1c88-12ad-ded12ea5f221@postgrespro.ru> From: Nikita Glukhov Organization: Postgres Professional Message-ID: <94ab028a-07aa-38e5-b184-3d640f642bfb@postgrespro.ru> Date: Tue, 17 Sep 2019 03:04:29 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.8.0 MIME-Version: 1.0 In-Reply-To: Content-Type: multipart/alternative; boundary="------------ED77F89D92E7EB724FE72DD0" Content-Language: en-US List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Precedence: bulk This is a multi-part message in MIME format. --------------ED77F89D92E7EB724FE72DD0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit On 23.07.2019 16:58, Pavel Stehule wrote: > > 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. -- Nikita Glukhov Postgres Professional: http://www.postgrespro.com The Russian Postgres Company --------------ED77F89D92E7EB724FE72DD0 Content-Type: text/html; charset=utf-8 Content-Transfer-Encoding: 8bit
On 23.07.2019 16:58, Pavel Stehule wrote:

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.
--
Nikita Glukhov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company
--------------ED77F89D92E7EB724FE72DD0--