From: Tomas Vondra <tomas.vondra@2ndquadrant.com>
To: Nikita Glukhov <n.gluhov@postgrespro.ru>
Cc: Michael Paquier <michael@paquier.xyz>
Cc: Stas Kelvich <s.kelvich@postgrespro.ru>
Cc: PostgreSQL-development <pgsql-hackers@postgresql.org>
Cc: Thomas Munro <thomas.munro@enterprisedb.com>
Cc: David Steele <david@pgmasters.net>
Cc: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Andrew Dunstan <andrew.dunstan@2ndquadrant.com>
Cc: Alexander Korotkov <a.korotkov@postgrespro.ru>
Cc: Robert Haas <robertmhaas@gmail.com>
Cc: Oleg Bartunov <obartunov@gmail.com>
Cc: Pavel Stehule <pavel.stehule@gmail.com>
Subject: Re: jsonpath
Date: Tue, 6 Nov 2018 16:48:36 +0100
Message-ID: <86a0e7df-443b-3f20-70b0-655a1f5dcbb2@2ndquadrant.com> (raw)
In-Reply-To: <9a175c2d-b0a9-475f-078b-febc185282ff@postgrespro.ru>
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>
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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: tomas.vondra@2ndquadrant.com, n.gluhov@postgrespro.ru, michael@paquier.xyz, s.kelvich@postgrespro.ru, thomas.munro@enterprisedb.com, david@pgmasters.net, tgl@sss.pgh.pa.us, andrew.dunstan@2ndquadrant.com, a.korotkov@postgrespro.ru, robertmhaas@gmail.com, obartunov@gmail.com, pavel.stehule@gmail.com
Subject: Re: jsonpath
In-Reply-To: <86a0e7df-443b-3f20-70b0-655a1f5dcbb2@2ndquadrant.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox