|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>|
|Views:||Raw Message | Whole Thread | Download mbox | Resend email|
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
>>>> 2. throw an error when the input or format string contains trailing
>>>> 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:
>> 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.
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company
|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|