Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtp (Exim 4.84_2) (envelope-from ) id 1dun1M-0007yP-Ph for pgsql-hackers@arkaria.postgresql.org; Wed, 20 Sep 2017 21:59:05 +0000 Received: from localhost ([127.0.0.1] helo=postgresql.org) by malur.postgresql.org with smtp (Exim 4.84_2) (envelope-from ) id 1dun1L-0007Sf-L4 for pgsql-hackers@arkaria.postgresql.org; Wed, 20 Sep 2017 21:59:03 +0000 Received: from makus.postgresql.org ([2001:4800:1501:1::229]) by malur.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA384:256) (Exim 4.84_2) (envelope-from ) id 1dun1K-0007SE-H8 for pgsql-hackers@postgresql.org; Wed, 20 Sep 2017 21:59:02 +0000 Received: from mail.postgrespro.ru ([93.174.131.138]) by makus.postgresql.org with esmtp (Exim 4.84_2) (envelope-from ) id 1dun1C-0007jj-4U for pgsql-hackers@postgresql.org; Wed, 20 Sep 2017 21:59:01 +0000 Received: from localhost (localhost [127.0.0.1]) by mail.postgrespro.ru (Postfix) with ESMTP id 1686D21C1D22; Thu, 21 Sep 2017 00:58:51 +0300 (MSK) X-Virus-Scanned: Debian amavisd-new at postgrespro.ru Received: from [10.100.236.30] (unknown [195.82.154.27]) (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 B72FF21C1D06; Thu, 21 Sep 2017 00:58:50 +0300 (MSK) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=postgrespro.ru; s=mail; t=1505944730; bh=bq0g08k7C7y21ZAYGAXBbg5q3pICBfRh2nXdY6ZP88I=; h=Subject:To:Cc:References:From:Date:In-Reply-To; b=PUQLzZpLljyVNiLmezyt+1BB6c9yOsi69aU3MDa3Yf+LT/4kA519fcnkMrC1n9NIJ 5Ois15REELYJ5H7pX9CFd95WILtX0uEat1TLpSpXAKk4xDanuuDt23PalEUfx9EzEu AAYsEDh5Xf64WOXOPVmbi5nSsITtcDB3jsHQIa9g= Subject: Re: compress method for spgist - 2 To: Alexander Korotkov , Tom Lane Cc: Darafei Praliaskouski , pgsql-hackers , Heikki Linnakangas , Fedor Sigaev References: <54907069.1030506@sigaev.ru> <20170920190059.1354.29457.pgcf@coridan.postgresql.org> <31999.1505938053@sss.pgh.pa.us> From: Nikita Glukhov Organization: Postgres Professional Message-ID: <60e048d3-aa29-8289-5438-3b0c4c1d8d84@postgrespro.ru> Date: Thu, 21 Sep 2017 00:57:49 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.3.0 MIME-Version: 1.0 In-Reply-To: Content-Type: multipart/alternative; boundary="------------68285CBD2F17E6F2C12C1103" Content-Language: en-US List-Archive: List-Help: List-ID: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: X-Mailing-List: pgsql-hackers Precedence: bulk Sender: pgsql-hackers-owner@postgresql.org This is a multi-part message in MIME format. --------------68285CBD2F17E6F2C12C1103 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit On 20.09.2017 23:19, Alexander Korotkov wrote: > On Wed, Sep 20, 2017 at 11:07 PM, Tom Lane > wrote: > > Darafei Praliaskouski > writes: > > I have some questions about the circles example though. > > >  * What is the reason for isnan check and swap of box ordinates > for circle? It wasn't in the code previously. > > I hadn't paid any attention to this patch previously, but this comment > excited my curiosity, so I went and looked: > > +       bbox->high.x = circle->center.x + circle->radius; > +       bbox->low.x = circle->center.x - circle->radius; > +       bbox->high.y = circle->center.y + circle->radius; > +       bbox->low.y = circle->center.y - circle->radius; > + > +       if (isnan(bbox->low.x)) > +       { > +               double tmp = bbox->low.x; > +               bbox->low.x = bbox->high.x; > +               bbox->high.x = tmp; > +       } > > Maybe I'm missing something, but it appears to me that it's > impossible for > bbox->low.x to be NaN unless circle->center.x and/or > circle->radius is a > NaN, in which case bbox->high.x would also have been computed as a > NaN, > making the swap entirely useless.  Likewise for the Y case.  There > may be > something useful to do about NaNs here, but this doesn't seem like it. > > Yeah, +1. > It is possible for bbox->low.x to be NaN when circle->center.x is and circle->radius are both +Infinity.  Without this float-order-preserving swapping one regression test for KNN with ORDER BY index will be totally broken (you can try it: https://github.com/glukhovn/postgres/tree/knn). Unfortunately, I do not remember exactly why, but most likely because of the incorrect index structure. -- Nikita Glukhov Postgres Professional: http://www.postgrespro.com The Russian Postgres Company --------------68285CBD2F17E6F2C12C1103 Content-Type: text/html; charset=utf-8 Content-Transfer-Encoding: 8bit

On 20.09.2017 23:19, Alexander Korotkov wrote:

On Wed, Sep 20, 2017 at 11:07 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Darafei Praliaskouski <me@komzpa.net> writes:
> I have some questions about the circles example though.

>  * What is the reason for isnan check and swap of box ordinates for circle? It wasn't in the code previously.

I hadn't paid any attention to this patch previously, but this comment
excited my curiosity, so I went and looked:

+       bbox->high.x = circle->center.x + circle->radius;
+       bbox->low.x = circle->center.x - circle->radius;
+       bbox->high.y = circle->center.y + circle->radius;
+       bbox->low.y = circle->center.y - circle->radius;
+
+       if (isnan(bbox->low.x))
+       {
+               double tmp = bbox->low.x;
+               bbox->low.x = bbox->high.x;
+               bbox->high.x = tmp;
+       }

Maybe I'm missing something, but it appears to me that it's impossible for
bbox->low.x to be NaN unless circle->center.x and/or circle->radius is a
NaN, in which case bbox->high.x would also have been computed as a NaN,
making the swap entirely useless.  Likewise for the Y case.  There may be
something useful to do about NaNs here, but this doesn't seem like it.
 
Yeah, +1.


It is possible for bbox->low.x to be NaN when circle->center.x is and
circle->radius are both +Infinity.  Without this float-order-preserving swapping
one regression test for KNN with ORDER BY index will be totally broken (you can
try it: https://github.com/glukhovn/postgres/tree/knn).  Unfortunately, I do not
remember exactly why, but most likely because of the incorrect index structure.

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