Re: pgsql: Implement jsonpath .datetime() method - Mailing list pgsql-committers

On Thu, Sep 26, 2019 at 2:57 AM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Alexander Korotkov <a(dot)korotkov(at)postgrespro(dot)ru> writes:
> > On Thu, Sep 26, 2019 at 2:12 AM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> >> The proximate problem seems to be that compareItems() is insufficiently
> >> careful to ensure that both values are non-null before passing them
> >> off to datatype-specific code.  The code accidentally fails to crash
> >> on 64-bit machines, but it's still giving garbage answers, I think.
>
> > I've found compareItems() code to not apply appropriate cast to/from
> > Datum.  Fixed in 7881bb14f4.  This makes test pass on my local 32-bit
> > machine.  I'll keep look on buildfarm.
>
> Hm.  dromedary seems not to crash either with that fix, but I'm not
> sure why not, because when I was running the previous tree by hand,
> the stack trace showed pretty clearly that we were getting to
> timestamp_cmp with one null and one non-null argument.  So I don't
> believe your argument that that's impossible, and even if it is,
> I do not think it's sane for compareItems to depend on that ---
> especially when one of its code paths *does* check for nulls.
>
> I do not have a very good opinion about the quality of this code
> upon my first glance at it.  Just looking at compareDatetime:


The patch with compareDatetime() refactoring was posted in the original 
thread in pgsql-hackers [1].


> * The code is schizophrenic about whether it's allowed to pass a
> null have_error pointer or not.  It is not very sensible to have
> some code doing
>                     if (have_error && *have_error)
>                         return 0;
> when other code branches will dump core for null have_error.
> Given that if this test actually was doing anything, what it
> would be doing is failing to detect error conditions, I think
> the only sensible design is to insist that have_error mustn't be
> null, in which case these checks for null pointer should be removed,
> because (a) they waste code and cycles and (b) they mislead the
> reader as to what the API of compareDatetime actually is.
>
> * At least some of the code paths will malfunction if the caller
> didn't initialize *have_error to false.  If that is an intended API
> requirement, it is not acceptable for the function header comment
> not to say so.  (For bonus points, it'd be nice if the header
> comment agreed with the code as to the name of the variable.)
> If this isn't an intended requirement, you need to fix the code,
> probably by initializing "*have_error = false;" up at the top.
>
> * This is silly:
>
>         if (*have_error)
>                 return 0;
>
>         *have_error = false;
>
> * Also, given that you have that "if (*have_error)" where you do,
> the have_error tests inside the switch are useless redundancy.
> You might as well just remove them completely and let the final
> test handle falling out if a conversion failed.  Alternatively
> you could drop the final test, because as the code stands right
> now, it's visibly impossible to get there with *have_error true.

Yes, oddities in have_error handling seem to appear during numerous 
reworks of the patch.  have_error is really non-NULL now, and its
handling was simplified in the patch.


> * It's a bit schizophrenic also that some of the switches
> lack default:s (and rely on the if (!cmpfunc) below), while
> the outer switch does have its own, completely redundant
> default:.  I'd get rid of that default: and instead add
> a comment explaining that the !cmpfunc test substitutes for
> default branches.

Default cases with elog()s were added to the all switches.  Previously, 
the default case in the outer switch was used to report invalid type1, 
and cmpfunc was used to report invalid type2.


> * OIDs are unsigned, so if you must print them, use %u not %d.

Fixed.

> * The errhints don't follow project message style.

Fixed, but I'm not sure about "*_tz()".  Maybe it's worth to pass current 
jsonb_xxx function name to compareDatetime() through JsonPathExecContext?


> * The blank lines before "break"s aren't really per project
> style either, IMO.  They certainly aren't doing anything to
> improve readability, and they help limit how much code you
> can see at once.

Fixed. If I recall it correctly, these lines were added by pgindent.


> * More generally, it's completely unclear why some error conditions
> are thrown as errors and others just result in returning *have_error.
> In particular, it seems weird that some unsupported datatype combinations
> cause hard errors while others do not.  Maybe that's fine, but if so,
> the function header comment is falling down on the job by not explaining
> the reasoning.

All cast errors are caught by jsonpath predicate.  Comparison of the 
uncomparable datetime types (time[tz] to dated types) also returns Unknown.
And only if datatype conversion requires current timezone, which is not 
available in immutable family of jsonb_xxx() functions, hard error is thrown.
This behavior is specific only for our jsonpath implementation.  But I'm 
really not sure if we should throw an error or return Unknown in this case.

[1]: https://www.postgresql.org/message-id/d9244568-08bb-5dcf-db25-540412e2e61f%40postgrespro.ru

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