Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA1:256) (Exim 4.89) (envelope-from ) id 1iDsdk-0007QG-E3 for pgsql-committers@arkaria.postgresql.org; Fri, 27 Sep 2019 15:58:40 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.89) (envelope-from ) id 1iDsdj-00041o-9t for pgsql-committers@arkaria.postgresql.org; Fri, 27 Sep 2019 15:58:39 +0000 Received: from magus.postgresql.org ([2a02:c0:301:0:ffff::29]) by malur.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA1:256) (Exim 4.89) (envelope-from ) id 1iDsdi-00041h-VI for pgsql-committers@lists.postgresql.org; Fri, 27 Sep 2019 15:58:39 +0000 Received: from cyclops.postgrespro.ru ([93.174.131.138] helo=mail.postgrespro.ru) by magus.postgresql.org with esmtp (Exim 4.89) (envelope-from ) id 1iDsdg-0004fL-0e for pgsql-committers@lists.postgresql.org; Fri, 27 Sep 2019 15:58:38 +0000 Received: from localhost (localhost [127.0.0.1]) by mail.postgrespro.ru (Postfix) with ESMTP id 3B2F221C4A8E; Fri, 27 Sep 2019 18:58:34 +0300 (MSK) X-Virus-Scanned: Debian amavisd-new at postgrespro.ru X-Spam-Flag: NO X-Spam-Score: 0 X-Spam-Level: X-Spam-Status: No, score=x tagged_above=-99 required=4 WHITELISTED tests=[] autolearn=unavailable Received: from [10.100.236.30] (unknown [195.82.154.49]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (Client did not present a certificate) by mail.postgrespro.ru (Postfix) with ESMTPSA id ED5A521C296A; Fri, 27 Sep 2019 18:58:33 +0300 (MSK) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=postgrespro.ru; s=mail; t=1569599914; bh=TvtF0V/8/zzsCkXh3AECl2ZunwRh9xLjJKwgodSfYrU=; h=In-Reply-To:References:To:From:Subject:Date; b=bxwJGuVMceYzldXqgN16mzTrKj+avI2qfhllJMuFlvGwUVYkT7rZkJalCRJfZhIN8 5O7BVUn2MqUEr3MQofR83slIyDskjjFjrCug5KDNShgBcdQCfGhoI8hCuDupxxlyiM YZ3I2RTcZwKsLKW6NxgEomCdd2VfziPv8gzih1+Q= In-Reply-To: References: To: pgsql-committers , Alexander Korotkov , Tom Lane From: Nikita Glukhov Subject: Re: pgsql: Implement jsonpath .datetime() method Organization: Postgres Professional Message-ID: Date: Fri, 27 Sep 2019 18:55:31 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.8.0 MIME-Version: 1.0 Content-Type: multipart/alternative; boundary="------------48B196863DFE685DE8AEAA16" Content-Language: en-US List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Precedence: bulk This is a multi-part message in MIME format. --------------48B196863DFE685DE8AEAA16 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit On Thu, Sep 26, 2019 at 2:57 AM Tom Lane wrote: > Alexander Korotkov writes: > > On Thu, Sep 26, 2019 at 2:12 AM Tom Lane 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 --------------48B196863DFE685DE8AEAA16 Content-Type: text/html; charset=utf-8 Content-Transfer-Encoding: 7bit 
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
--------------48B196863DFE685DE8AEAA16--