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