Subject: Re: jsonpath
To: Nikita Glukhov <n.gluhov@postgrespro.ru>
Cc: Michael Paquier <michael@paquier.xyz>,
 Stas Kelvich <s.kelvich@postgrespro.ru>,
 PostgreSQL-development <pgsql-hackers@postgresql.org>,
 Thomas Munro <thomas.munro@enterprisedb.com>,
 David Steele <david@pgmasters.net>, Tom Lane <tgl@sss.pgh.pa.us>,
 Andrew Dunstan <andrew.dunstan@2ndquadrant.com>,
 Alexander Korotkov <a.korotkov@postgrespro.ru>,
 Robert Haas <robertmhaas@gmail.com>, Oleg Bartunov <obartunov@gmail.com>,
 Pavel Stehule <pavel.stehule@gmail.com>
References: 
 <CAA8=A796mY7kJdKARt7B+faW70-c8mRh+Ff_joBJ+NyCFLv3Gg@mail.gmail.com>
 <19506.1521524178@sss.pgh.pa.us>
 <880bcbc6-1dba-cf82-7945-c10528f602c4@postgrespro.ru>
 <892ae4c4-e549-ab21-c57c-37c5fd47fda0@pgmasters.net>
 <8dd1cd76-cd4e-9be7-9515-ef0b8b21cb67@postgrespro.ru>
 <CAEepm=0jJ4fkEWLHc2Rqt5F4Und6qd+60pxFQmCnbf0nYYjrcg@mail.gmail.com>
 <3bc15ece-5219-e4ca-0b5c-fcd2b276aa26@postgrespro.ru>
 <4e2aaa88-3bbf-6af3-8dad-d5687fbec89f@postgrespro.ru>
 <42272F2B-16D1-43D6-B22F-273AE1DA6D6B@postgrespro.ru>
 <715c1c44-9bb9-6b9f-7c59-61b66578f107@postgrespro.ru>
 <20181002023317.GI11712@paquier.xyz>
 <063560d8-bc0b-945b-8796-5ba998d79dc6@2ndquadrant.com>
 <9a175c2d-b0a9-475f-078b-febc185282ff@postgrespro.ru>
From: Tomas Vondra <tomas.vondra@2ndquadrant.com>
Message-ID: <86a0e7df-443b-3f20-70b0-655a1f5dcbb2@2ndquadrant.com>
Date: Tue, 6 Nov 2018 16:48:36 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101
 Thunderbird/60.2.1
MIME-Version: 1.0
In-Reply-To: <9a175c2d-b0a9-475f-078b-febc185282ff@postgrespro.ru>
Content-Type: text/plain; charset=windows-1252; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 8bit
Precedence: bulk

On 11/6/18 3:31 PM, Nikita Glukhov wrote:
> On 29.10.2018 2:20, Tomas Vondra wrote:>
 >
 > ...>
> Thank you for your review.
> 
>> 1) There are no docs, which makes it pretty much non-committable for
>> now. I know there is [1] and it was a good intro for the review, but we
>> need something as a part of our sgml docs.
> 
> I think that jsonpath part of documentation can be extracted from [2] and
> added to the patch set.
> 

Yes, please let's do that. The patch could not get into RFC without 
docs, so let's deal with it now.

>> 2) 0001 says this:
>>
>>      *typmod = -1; /* TODO implement FF1, ..., FF9 */
>>
>> but I have no idea what FF1 or FF9 are. I guess it needs to be
>> implemented, or explained better.
> 
> FF1-FF9 are standard datetime template fields used for specifying of fractional
> seconds.  FF3/FF6 are analogues of our MS/US.  I decided simply to implement
> this feature (see patch 0001, I also can supply it in the separate patch).
> 

Understood. Thanks for the explanation.

> But FF7-FF9 are questionable since the maximal supported precision is only 6.
> They are optional by the standard:
> 
>    95) Specifications for Feature F555, “Enhanced seconds precision”:
>      d) Subclause 9.44, “Datetime templates”:
>        i) Without Feature F555, “Enhanced seconds precision”,
>           a <datetime template fraction> shall not be FF7, FF8 or FF9.
> 
> So I decided to allow FF7-FF9 only on the output in to_char().
> 

Hmmmm, isn't that against the standard then? I mean, if our precision is 
only 6, that probably means we don't have the F555 feature, which means 
FF7-9 should not be available.

It also seems like a bit surprising behavior that we only allow those 
fields in to_char() and not in other places.

>> 4) There probably should be .gitignore rule for jsonpath_gram.h, just
>> like for other generated header files.
> 
> I see 3 rules in /src/backend/utils/adt/.gitignore:
> 
> /jsonpath_gram.h
> /jsonpath_gram.c
> /jsonpath_scan.c
> 

But there's a symlink in src/include/utils/jsonpath_gram.h and that's 
not in .gitignore.

>> FWIW, I'd use JSONPATH_STRICT instead of JSONPATH_LAX. The rest of the
>> codebase works with "strict" flags passed around, and it's easy to
>> forget to negate the flag somewhere (at least that's my experience).
> 
> Jsonpath lax/strict mode flag is used only in executeJsonPath() where it is
> saved  in "laxMode" field.  New "strict" flag passed to datetime functions
> is unrelated to jsonpath.
> 

OK.

>> 7) I see src/include/utils/jsonpath_json.h adds support for plain json
>> by undefining various jsonb macros and redirecting them to the json
>> variants. I find that rather suspicious - imagine you're investigating
>> something in code using those jsonb macros, and wondering why it ends up
>> calling the json stuff. I'd be pretty confused ...
> 
> I agree, this is rather simple but doubtful solution.  That's why json support
> was in a separate patch until the 18th version of the patches.
> 
> But if we do not want to compile jsonpath.c twice with different definitions,
> then we need some kind of run-time wrapping over json strings and jsonb
> containers, which seems a bit harder to implement.
> 
> To simplify debugging I can also suggest to explicitly preprocess jsonpath.c
> into jsonpath_json.c using redefinitions from jsonpath_json.h before its
> compilation.
> 

Not sure what's the right solution. But I agree the current approach 
probably is not it.

>> 
>> 9) It's generally a good idea to make the individual pieces committable
>> separately, but that means e.g. the regression tests have to pass after
>> each patch. At the moment that does not seem to be the case for 0002,
>> see the attached file. I'm running with -DRANDOMIZE_ALLOCATED_MEMORY,
>> not sure if that's related.
> 
> This should definitely be a bug in json support, but I can't reproduce
> it simply by defining -DRANDOMIZE_ALLOCATED_MEMORY.  Could you provide
> a stack trace at least?
> 
I'll try.

regards

-- 
Tomas Vondra                  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services