Re: pgsql: Implement jsonpath .datetime() method - Mailing list pgsql-committers
From | Alexander Korotkov |
---|---|
Subject | Re: pgsql: Implement jsonpath .datetime() method |
Date | |
Msg-id | CAPpHfdu1GTv0BZ4=7ZeJ+vG2XmnK44f3HzsLJAhMy9QmKgWd5w@mail.gmail.com Whole thread Raw |
In response to | Re: pgsql: Implement jsonpath .datetime() method (Tom Lane <tgl@sss.pgh.pa.us>) |
List | pgsql-committers |
On Thu, Sep 26, 2019 at 2:57 AM Tom Lane <tgl@sss.pgh.pa.us> wrote: > 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. 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
pgsql-committers by date: