Re: compress method for spgist - 2

From: Alexander Korotkov <aekorotkov(at)gmail(dot)com>
To: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
Cc: Darafei Praliaskouski <me(at)komzpa(dot)net>, pgsql-hackers <pgsql-hackers(at)postgresql(dot)org>, Heikki Linnakangas <hlinnaka(at)iki(dot)fi>, Fedor Sigaev <teodor(at)sigaev(dot)ru>, Nikita Glukhov <n(dot)gluhov(at)postgrespro(dot)ru>
Subject: Re: compress method for spgist - 2
Date: 2017-09-20 20:19:01
Message-ID: CAPpHfduv0XZ_k+D3NoFMr=wFVM22D=wFZCMMswoLwBe2jAbuJw@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox
Thread:
Lists: pgsql-hackers

On Wed, Sep 20, 2017 at 11:07 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:

> Darafei Praliaskouski <me(at)komzpa(dot)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.

> 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.
>

I've extracted polygon opclass into separate patch (attached). I'll rework
and resubmit circle patch later.
I'm not particularly sure that polygon.sql is a good place for testing
sp-gist opclass for polygons... But we've already done so for box.sql.

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

Attachment Content-Type Size
0001-spgist-compress-method-7.patch application/octet-stream 18.1 KB
0002-spgist-polygon-7.patch application/octet-stream 26.5 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Andrew Gierth 2017-09-20 20:25:12 close_ps, NULLs, and DirectFunctionCall
Previous Message Tom Lane 2017-09-20 20:07:33 Re: compress method for spgist - 2