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
