Re: jsonpath - Mailing list pgsql-hackers
| From | Nikita Glukhov |
|---|---|
| Subject | Re: jsonpath |
| Date | |
| Msg-id | 64e3108c-888f-092e-30a2-58dea157707e@postgrespro.ru Whole thread Raw |
| In response to | Re: jsonpath (Tomas Vondra <tomas.vondra@2ndquadrant.com>) |
| Responses |
Re: jsonpath
(Tomas Vondra <tomas.vondra@2ndquadrant.com>)
|
| List | pgsql-hackers |
Attached 20th version the jsonpath patches. On 08.11.2018 4:52, Tomas Vondra wrote:
On 11/6/18 4:48 PM, Tomas Vondra wrote:On 11/6/18 3:31 PM, Nikita Glukhov wrote:On 29.10.2018 2:20, Tomas Vondra wrote:>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.Not sure why you can't reproduce the failures, it's perfectly reproducible for me. For the record, I'm doing this: ./configure --prefix=/home/user/pg-jsonpath --enable-debug --enable-cassert CFLAGS="-O0 -DRANDOMIZE_ALLOCATED_MEMORY" && make -s clean && make -s -j4 && make check After sticking Assert(false) to JsonEncodeJsonbValue (to the default case), I get a failure like this: select json '{}' @* 'lax $[0]'; ! WARNING: unknown jsonb value type: 20938064 ! server closed the connection unexpectedly ! This probably means the server terminated abnormally ! before or while processing the request. ! connection to server was lost The backtrace is attached. My guess is JsonValueListGetList in jsonb_jsonpath_query only does shallow copy instead of copying the pieces into funcctx->multi_call_memory_ctx, so it gets corrupted on subsequent calls. I also attach valgrind report, but I suppose the reported issues are a consequence of the same bug.
Than you for your help, I finally have managed to reproduce this bug. The problem was really related to copying values but it was not located in the jsonb_jsonpath_query() where the whole value list should already be allocated in funcctx->multi_call_memory_ctx (executeJsonPath() is called in that context and also input json/jsonb is copied into it). Stack-allocated root JsonbValue was not copied to the heap due to wrong copy condition in recursiveExecuteNoUnwrap() (jpiIndexArray case): @@ -1754,1 +1754,1 @@ recursiveExecuteNoUnwrap(JsonPathExecContext *cxt, ... - res = recursiveExecuteNext(cxt, jsp, &elem, v, found, !binary); + res = recursiveExecuteNext(cxt, jsp, &elem, v, found, singleton || !binary); I have refactored a bit this place extracting explicit 'copy' flag. Also I found a similar problem in makePassingVars() where Numerics were not copied into 'multi_call_memory_ctx' from the input json object. I decided to use datumCopy() inside makePassingVars() instead of copying the whole input json in jsonb_jsonpath_query() using PG_GETARG_JSONB_P_COPY(). But I think that processing of the input json with variables should ideally be bone lazily, just don't know if this refactoring is worth it because 3rd argument of json[b]_jsonpath_xxx() functions is used only for testing of variables passing, SQL/JSON functions use the different mechanism.
On 06.11.2018 18:48, Tomas Vondra wrote:
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.
Ok, now error is thrown if FF7-FF9 are found in the format string.
On 06.11.2018 18:48, Tomas Vondra wrote:
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.cBut there's a symlink in src/include/utils/jsonpath_gram.h and that's not in .gitignore.
Added jsonpath_gram.h to /src/include/utils/.gitignore.-- Nikita Glukhov Postgres Professional: http://www.postgrespro.com The Russian Postgres Company
Attachment
pgsql-hackers by date: