Subject: Re: jsonpath
To: Andrew Dunstan <andrew.dunstan@2ndquadrant.com>,
 Pavel Stehule <pavel.stehule@gmail.com>
Cc: PostgreSQL-development <pgsql-hackers@postgresql.org>,
 Oleg Bartunov <obartunov@gmail.com>
References: <fcc6fc6a-b497-f39a-923d-aa34d0c588e8@2ndQuadrant.com>
 <CAFj8pRAhPnD=sPu-E=rwfcbgeZ8PLczYEzCsvgNYLtybSy9-=g@mail.gmail.com>
 <9f845926-0d7d-7c4d-75e3-b1e921a1677a@2ndQuadrant.com>
 <CAFj8pRBCvhgLCROfSCPGmwWZpSY+dKQN4O9xRe+1-wjBiXiHkA@mail.gmail.com>
 <bad98003-31ba-523e-d76c-3679efb2e732@2ndQuadrant.com>
 <CAFj8pRDWSCWd1JxA8srwbfL5cPgz2b5minq2UsM=cxgsFF-orw@mail.gmail.com>
 <48f13b75-0be7-c356-ff26-1db743add56e@postgrespro.ru>
 <7b4b8973-43f5-71c5-bf86-51cb5798dd88@2ndQuadrant.com>
 <f0b44fe7-d47a-eff2-592c-d9dbe4a63f75@2ndQuadrant.com>
 <5290ddd1-f23e-a53a-cae0-830dbde0a5fe@2ndQuadrant.com>
From: Nikita Glukhov <n.gluhov@postgrespro.ru>
Organization: Postgres Professional
Message-ID: <604ffe9c-b42e-7c42-e16b-aa44c3a12c3c@postgrespro.ru>
Date: Tue, 23 Jan 2018 16:18:14 +0300
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101
 Thunderbird/52.5.0
MIME-Version: 1.0
In-Reply-To: <5290ddd1-f23e-a53a-cae0-830dbde0a5fe@2ndQuadrant.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 8bit
Content-Language: en-US
Precedence: bulk

On 23.01.2018 01:41, Andrew Dunstan wrote:

> On 01/17/2018 04:01 PM, Andrew Dunstan wrote:
>> On 01/15/2018 07:24 PM, Andrew Dunstan wrote:
>>> On 01/10/2018 05:42 PM, Nikita Glukhov wrote:
>>>> Attached new 8th version of jsonpath related patches. Complete
>>>> documentation is still missing.
>>>>
>>>> The first 4 small patches are necessary datetime handling in jsonpath:
>>>> 1. simple refactoring, extracted function that will be used later in
>>>> jsonpath
>>>> 2. throw an error when the input or format string contains trailing
>>>> elements
>>>> 3. avoid unnecessary cstring to text conversions
>>>> 4. add function for automatic datetime type recognition by the
>>>> presence of formatting components
>>>>
>>>> Should they be posted in a separate thread?
>>>>
>>> The first of these refactors the json/jsonb timestamp formatting into a
>>> single function, removing a lot of code duplication. The involves
>>> exposing time2tm() and timetz2tm(). I don't think that's a tragedy, so
>>> unless there is any objection I propose to commit it shortly.
>>>
>>> The next three expose a bit more of the date/time API. I'm still
>>> reviewing those.
>> I have committed the first of these patches.
>>
>> I have reviewed the next three, and I think they are generally good.
>> There is no real point in committing them ahead of the jsonpath patch
>> since there would be no point in having them at all but for that patch.
>>
>> Note that these do export the following hitherto internal bits of the
>> datetime functionality:
>>
>>      tm2time
>>      tm2timetz
>>      AdjustTimeForTypmod
>>      AdjustTimestampForTypmod
>>
>> Moving on to review the main jsonpath patch.
>
> OK, I understand a good deal more than I did about how the jsopnpath
> code works, but the commenting is abysmal.
Thank you for reviewing.

> I got quite nervous about adding a new datetime variant to JsonbValue.
> However, my understanding from the code is that this will only ever be
> used in an intermediate jsonpath processing result, and it won't be used
> in a stored or build jsonb object. Is that right?
Yes, you are right. Datetime JsonbValues are used only for for in-memory 
representation of SQL/JSON datetime items, they converted into ordinary 
JSON strings in ISO format when json/jsonb encoded into a datum.

> If it is we need to say so, and moreover we need to warn coders in the
> header file about the restricted use of this variant.  I'm not sure we
> can enforce it with an Assert, but If we can we should. I'm not 100%
> sure that doing it this way, i.e. by extending and resuing jsonbValue,
> is desirable, I'd like to know some of the thinking behind the design.
Datetime support was added to our jsonpath implementation when there was 
already a lot of code using plain JsonbValue. So, the simplest are most 
effective solution I found was JsonbValue extension. We could also 
introduce extended struct SqlJsonItem, but it seems that there will be a 
lot of unnecessary conversions between SqlJsonItem and JsonbValue.

> The encoding of a jsonpath value into a binary string is quite nifty,
> but it needs to be documented. Once I understood it better some of my
> fears about parser overhead were alleviated.
> The use of a function pointer inside JsonPathVariable seems unnecessary
> and baroque. It would be much more straightforward to set an isnull flag
> in the struct and process the value in an "if" statement accordingly,
> avoiding the function pointer altogether.
Callback in JsonPathVariable is used for on-demand evaluation of 
SQL/JSON PASSING parameters (see EvalJsonPathVar() from patch 
0010-sqljson-v08.patch). For jsonpath itself it is really unnecessary.

> I am going to be travelling for a few days, then back online for a day
> or two, then gone for a week. I hope we can make progress on these
> features, but this needs lots more eyeballs, reviewing code as well as
> testing, and lots more responsiveness. The whole suite is enormous.
>
> cheers
>
> andrew

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