Re: Support for jsonpath .datetime() method

From: Anastasia Lubennikova <lubennikovaav(at)gmail(dot)com>
To: pgsql-hackers(at)lists(dot)postgresql(dot)org
Cc: Alexander Korotkov <a(dot)korotkov(at)postgrespro(dot)ru>, Nikita Glukhov <n(dot)gluhov(at)postgrespro(dot)ru>
Subject: Re: Support for jsonpath .datetime() method
Date: 2019-07-15 12:45:43
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

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


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.

+ * 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.
+ */
+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",
+ "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)
+ 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

In response to


Browse pgsql-hackers by date

  From Date Subject
Next Message Robert Haas 2019-07-15 13:11:25 Re: Introduce timeout capability for ConditionVariableSleep
Previous Message Luis Carril 2019-07-15 12:39:00 Re: Option to dump foreign data in pg_dump