Re: [HACKERS] compress method for spgist - 2 - Mailing list pgsql-hackers

From Alexander Korotkov
Subject Re: [HACKERS] compress method for spgist - 2
Date
Msg-id CAPpHfdv+7QxdaVmR=wM=zAnaR=w1uvaHhmzuN8mM7c9g14QUfQ@mail.gmail.com
Whole thread Raw
In response to Re: [HACKERS] compress method for spgist - 2  (Darafei Praliaskouski <me@komzpa.net>)
List pgsql-hackers
On Wed, Sep 20, 2017 at 10:00 PM, Darafei Praliaskouski <me@komzpa.net> wrote:
The following review has been posted through the commitfest application:
make installcheck-world:  not tested
Implements feature:       not tested
Spec compliant:           not tested
Documentation:            tested, passed

Hi,

I like the SP-GiST part of the patch. Looking forward to it, so PostGIS can benefit from SP-GiST infrastructure.

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.
 * There are tests for infinities in circles, but checks are for NaNs.

This code was migrated from original patch by Nikita.  I can assume he means that nan should be treated as greatest possible floating point value (like float4_cmp_internal() does).  However, our current implementation of geometrical datatypes don't correctly handles all the combinations of infs as nans.  Most of code was written without taking infs and nans into account.  Also, I'm not sure if this code fixes all possible issues with infs and nans in SP-GiST opclass for circles.  This is why I'm going to remove nans handling from this place.
 
 * It seems to me that circle can be implemented without recheck, by making direct exact calculations.

Right.  Holding circles in the leafs instead of bounding boxes would both allow exact calculations and take less space.
 
How about removing circle from the scope of this patch, so it is smaller and cleaner?

Good point.  Polygons are enough for compress function example.  Opclass for circles could be submitted as separate patch.

------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company 

pgsql-hackers by date:

Previous
From: Tom Lane
Date:
Subject: Re: [HACKERS] [Proposal] Allow users to specify multiple tables in VACUUM commands
Next
From: Jeff Janes
Date:
Subject: Re: [HACKERS] SCRAM in the PG 10 release notes