Re: jsonpath

On Tue, Dec 4, 2018 at 2:23 AM Nikita Glukhov <n(dot)gluhov(at)postgrespro(dot)ru> wrote:
> 2) We define both DCH_FF# and DCH_ff#, but we never ever use the
> lower-case version. Heck, it's not mentioned even in DCH_keywords, which
> does this:
>
> ...
> {"FF1", 3, DCH_FF1, false, FROM_CHAR_DATE_NONE}, /* F */
> ...
> {"ff1", 3, DCH_FF1, false, FROM_CHAR_DATE_NONE}, /* F */
> ...
>
> Compare that to DCH_DAY, DCH_Day and DCH_day, mapped to "DAY", "Day" and
> "day".
>
> Yes, "ff#" are mapped to DCH_FF# like "mi" is mapped DCH_MI.
>
> "Day", "day" are not mapped to DCH_DAY because they determine letter case in the
> output, but "ff1" and "FF#" output contains only digits.

Right, DCH_poz is also offset in DCH_keywords array. So, if array has
an entry for "ff1" then enum should have a DCH_ff1 member in the same
position.

I got some other questions regarding this patchset.

1) Why do we parse FF7-FF9 if we're not supporting them anyway?
Without defining FF7-FF9 we can also don't throw errors for them
everywhere. That would save us some code lines.

2) + DCH_to_char_fsec("%01d", in->fsec / INT64CONST(100000));
Why do we use INT64CONST() here and in the similar places assuming
that fsec is only uint32?

3) wrapItem() is unused in
0002-Jsonpath-engine-and-operators-v21.patch, but used in
0006-Jsonpath-syntax-extensions-v21.patch. Please, move it to
0006-Jsonpath-syntax-extensions-v21.patch?

4) I also got these couple of warning during compilation.

jsonpath_exec.c:1485:1: warning: unused function
'recursiveExecuteNested' [-Wunused-function]
recursiveExecuteNested(JsonPathExecContext *cxt, JsonPathItem *jsp,
^
1 warning generated.
jsonpath_scan.l:444:6: warning: implicit declaration of function
'jsonpath_yyparse' is invalid in C99 [-Wimplicit-function-declaration]
if (jsonpath_yyparse((void*)&parseresult) != 0)
^
1 warning generated.

Perhaps recursiveExecuteNested() is unsed in this patchset. It's
probably used by some subsequent SQL/JSON-related patchset. So,
please, move it there.

5) I think each usage of PG_TRY()/PG_CATCH() deserves comment
describing why it's safe to use without subtransaction. In fact we
should just state that no function called inside performs data
modification.

------
Alexander Korotkov

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