pg.ddx.io  pgsql-hackers@postgresql.org mailing list archive  
help / color / mirror / Atom feed
From: Tom Lane <tgl@sss.pgh.pa.us>
To: Darafei Praliaskouski <me@komzpa.net>
Cc: pgsql-hackers@postgresql.org, Heikki Linnakangas <hlinnaka@iki.fi>
Cc: Fedor Sigaev <teodor@sigaev.ru>
Cc: Alexander Korotkov <aekorotkov@gmail.com>
Cc: Nikita Glukhov <n.gluhov@postgrespro.ru>
Subject: Re: compress method for spgist - 2
Date: Wed, 20 Sep 2017 16:07:33 -0400
Message-ID: <31999.1505938053@sss.pgh.pa.us> (raw)
In-Reply-To: <20170920190059.1354.29457.pgcf@coridan.postgresql.org>
References: <54907069.1030506@sigaev.ru>
	<CAPpHfdsoy2bJMSTtvQySJ9zN2YJmzWJBLvdHV=ZvMOm6BR0UFg@mail.gmail.com>
	<20170920190059.1354.29457.pgcf@coridan.postgresql.org>
List-Unsubscribe: <mailto:majordomo@postgresql.org?body=unsub%20pgsql-hackers>

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.

> How about removing circle from the scope of this patch, so it is smaller and cleaner?

Neither of those patches would be particularly large, and since they'd need
to touch adjacent code in some places, the diffs wouldn't be independent.
I think splitting them is just make-work.

			regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers



view thread (41+ messages)  latest in thread

Message-ID: <31999.1505938053@sss.pgh.pa.us>
Permalink:  ../31999.1505938053@sss.pgh.pa.us/
Also on:    postgresql.org/message-id/31999.1505938053@sss.pgh.pa.us

 · 

reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: pgsql-hackers@postgresql.org
  Cc: tgl@sss.pgh.pa.us, me@komzpa.net, hlinnaka@iki.fi, teodor@sigaev.ru, aekorotkov@gmail.com, n.gluhov@postgrespro.ru
  Subject: Re: compress method for spgist - 2
  In-Reply-To: <31999.1505938053@sss.pgh.pa.us>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox