Re: Support for jsonpath .datetime() method

The following review has been posted through the commitfest application:
make installcheck-world: tested, passed
Implements feature: tested, passed
Spec compliant: tested, passed
Documentation: not tested

Hi,

In general, the feature looks good. It is consistent with the standard and the code around.
It definitely needs more documentation - datetime() and new jsonb_path_*_tz() functions are not documented.

Here are also minor questions on implementation and code style:

1) + case jbvDatetime:
elog(ERROR, "unexpected jbvBinary value");
We should use separate error message for jvbDatetime here.

2) + *jentry = JENTRY_ISSTRING | len;
Here we can avoid using JENTRY_ISSTRING since it defined to 0x0.
I propose to do so to be consistent with jbvString case.

3)
+ * Default time-zone for tz types is specified with 'tzname'. If 'tzname' is
+ * NULL and the input string does not contain zone components then "missing tz"
+ * error is thrown.
+ */
+Datum
+parse_datetime(text *date_txt, text *fmt, bool strict, Oid *typid,
+ int32 *typmod, int *tz)

The comment about 'tzname' is outdated.

4) Some typos:

+ * Convinience macros for error handling
> * Convenience macros for error handling

+ * Two macros below helps handling errors in functions, which takes
> * Two macros below help to handle errors in functions, which take

5) + * RETURN_ERROR() macro intended to wrap ereport() calls. When have_error
+ * argument is provided, then instead of ereport'ing we set *have_error flag

have_error is not a macro argument, so I suggest to rephrase this comment.

Shouldn't we pass have_error explicitly?
In case someone will change the name of the variable, this macro will work incorrectly.

6) * When no argument is supplied, first fitting ISO format is selected.
+ /* Try to recognize one of ISO formats. */
+ static const char *fmt_str[] =
+ {
+ "yyyy-mm-dd HH24:MI:SS TZH:TZM",
+ "yyyy-mm-dd HH24:MI:SS TZH",
+ "yyyy-mm-dd HH24:MI:SS",
+ "yyyy-mm-dd",
+ "HH24:MI:SS TZH:TZM",
+ "HH24:MI:SS TZH",
+ "HH24:MI:SS"
+ };

How do we choose the order of formats to check? Is it in standard?
Anyway, I think this struct needs a comment that explains that changing of order can affect end-user.

7) + if (res == jperNotFound)
+ RETURN_ERROR(ereport(ERROR,
+ (errcode(ERRCODE_INVALID_ARGUMENT_FOR_JSON_DATETIME_FUNCTION),
+ errmsg("invalid argument for SQL/JSON datetime function"),
+ errdetail("unrecognized datetime format"),
+ errhint("use datetime template argument for explicit format specification"))));

The hint is confusing. If I understand correctly, no-arg datetime function supports all formats,
so if parsing failed, it must be an invalid argument and providing format explicitly won't help.

The new status of this patch is: Waiting on Author