Re: jsonpath

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