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
