From: | Nikita Glukhov <n(dot)gluhov(at)postgrespro(dot)ru> |
---|---|
To: | Andrew Dunstan <andrew(dot)dunstan(at)2ndquadrant(dot)com>, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> |
Cc: | PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org>, Oleg Bartunov <obartunov(at)gmail(dot)com> |
Subject: | Re: jsonpath |
Date: | 2018-01-23 13:18:14 |
Message-ID: | 604ffe9c-b42e-7c42-e16b-aa44c3a12c3c@postgrespro.ru |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
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
From | Date | Subject | |
---|---|---|---|
Next Message | Amit Kapila | 2018-01-23 13:31:07 | Re: [HACKERS] Restrict concurrent update/delete with UPDATE of partition key |
Previous Message | Ildus Kurbangaliev | 2018-01-23 13:04:54 | Re: [HACKERS] Custom compression methods |