Hi, 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. 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. 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. 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. 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? 4) There probably should be .gitignore rule for jsonpath_gram.h, just like for other generated header files. 5) jbvType says jbvDatetime is "virtual type" but does not explain what it is. IMHO that deserves a comment or something. 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"). 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). 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 ... Also, some of the redefinitions are not really needed for example JsonbWrapItemInArray and JsonbWrapItemsInArray are not used anywhere (and neither are the json variants). 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. 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. 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. regards [1] https://github.com/obartunov/sqljsondoc/blob/master/README.jsonpath.md -- Tomas Vondra http://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
*** /home/user/work/postgres/src/test/regress/expected/json_jsonpath.out 2018-10-28 22:20:53.788520950 +0100 --- /home/user/work/postgres/src/test/regress/results/json_jsonpath.out 2018-10-28 22:22:37.786520950 +0100 *************** *** 281,291 **** (1 row) select json '{}' @* 'lax $[0]'; ! ?column? ! ---------- ! {} ! (1 row) ! select json '[1]' @* 'lax $[0]'; ?column? ---------- --- 281,287 ---- (1 row) select json '{}' @* 'lax $[0]'; ! ERROR: unknown jsonb value type: 21030544 select json '[1]' @* 'lax $[0]'; ?column? ---------- *************** *** 320,330 **** (1 row) select json '{}' @* 'lax $[last]'; ! ?column? ! ---------- ! {} ! (1 row) ! select json '[1,2,3]' @* '$[last]'; ?column? ---------- --- 316,322 ---- (1 row) select json '{}' @* 'lax $[last]'; ! ERROR: unknown jsonb value type: 21063360 select json '[1,2,3]' @* '$[last]'; ?column? ---------- ======================================================================