On 05.01.2019 3:11, Alexander Korotkov wrote:> 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.FF7-FF9 were removed.> 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?Fixed.

> 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?wraptItem() was moved into patch #6.

Prototype for jsonpath_yyparse() should be in the Bison-generated filesrc/include/adt/jsonpath_gram.h.

> 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.Comments were added.