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

Alexander Korotkov <a.korotkov@postgrespro.ru> writes:
> On Thu, Sep 26, 2019 at 2:12 AM Tom Lane <tgl@sss.pgh.pa.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.

            regards, tom lane