Re: pgsql: Implement jsonpath .datetime() method

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 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.
>
> * 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.
>
> * 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.
>
> * 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.
>
> * OIDs are unsigned, so if you must print them, use %u not %d.
>
> * The errhints don't follow project message style.
>
> * 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.

Thank you for feedback. Nikita and me will provide patches to fix
pointed issues during next couple of days.

------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company