Re: jsonpath

On 29.10.2018 2:20, Tomas Vondra wrote:

On 10/02/2018 04:33 AM, Michael Paquier wrote:

On Sat, Sep 08, 2018 at 02:21:27AM +0300, Nikita Glukhov wrote:

Attached 18th version of the patches rebased onto the current master.

Nikita, this version fails to apply, as 0004 has conflicts with some
regression tests.  Could you rebase?  I am moving the patch to CF
2018-11, waiting for your input.
--
Michael

As Michael mentioned, the patch does not apply anymore, so it would be
good to provide a rebased version for CF 2018-11. I've managed to do
that, as the issues are due to minor bitrot, so that I can do some quick
review of the current version.

Sorry that I failed to provide rebased version earlier. 

Attached 19th version of the patches rebased onto the current master.

I haven't done much testing, pretty much just compiling, running the
usual regression tests and reading through the patches. I plan to do
more testing once the rebased patch is submitted.

Now, a couple of comments based on eye-balling the diffs.

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.

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).

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().

3) The makefile rule for scan.o does this:

+# Latest flex causes warnings in this file.
+ifeq ($(GCC),yes)
+scan.o: CFLAGS += -Wno-error
+endif

That seems a bit ugly, and we should probably try to make it work with
the latest flex, instead of hiding the warnings. I don't think we have
any such ad-hoc rules in other Makefiles. If we really need it, can't we
make it part of configure, and/or restrict it depending on flex version?

These lines seem to belong to the earliest versions of our jsonpath
implementation. There is no scan.o file now at all, there is only 
jsonpath_scan.o, so I simply removed these lines.

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

5) jbvType says jbvDatetime is "virtual type" but does not explain what
it is. IMHO that deserves a comment or something.

"Virtual type" means here that it is used only for in-memory processing and 
converted into JSON string when outputted to jsonb.  Corresponding comment 
was added.

6) I see the JsonPath definition says this about header:
   /* just version, other bits are reservedfor future use */

but the very next thing it does is defining two pieces stored in the
header - version AND "lax" mode flag. Which makes the comment invalid
(also, note the missing space after "reserved").

Fixed.

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.

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.

Also, some of the redefinitions are not really needed for example
JsonbWrapItemInArray and JsonbWrapItemsInArray are not used anywhere
(and neither are the json variants).

These definitions will be used in the next patches, so I removed them 
for now from this patch.

8) I see 0002 defines IsAJsonbScalar like this:
 #define IsAJsonbScalar(jsonbval)  (((jsonbval)->type >= jbvNull && \                                    (jsonbval)->type <= jbvBool) || \                                    (jsonbval)->type == jbvDatetime)

while 0004 does this
 #define jspIsScalar(type) ((type) >= jpiNull && (type) <= jpiBool)

I know those are for different data types (jsonb vs. jsonpath), but I
suppose jspIsScalar should include the datetime too.

jpiDatetime does not mean the same thing as jbvDatetime.
jpiDatetime is a representation of ".datetime()" item method.
jbvDatetime is a representation of datetime SQL/JSON items.

Also datetime SQL/JSON items can be represented in JSON path only by strings
with ".datetime()" method applied, they do not have their own literal:
'"2018-01-10".datetime()'   

FWIW JsonPathItemType would deserve better documentation what the
various items mean (a comment for each line would be enough). I've been
wondering if "jpiDouble" means scalar double value until I realized
there's a ".double()" function for paths.

I added per-item comments.

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?


[1] https://github.com/obartunov/sqljsondoc/blob/master/README.jsonpath.md
[2] https://github.com/postgrespro/sqljson/tree/sqljson_doc

-- 
Nikita Glukhov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company