Re: Commit fest 2014-12, let's begin!

Hi all,

Let's begin the commit fest of December:
https://commitfest.postgresql.org/action/commitfest_view?id=25

Looking at the stats at the top of the above page, there are currently
56 patches to be dealt with:
- Needs Review: 48
- Waiting on Author: 5
- Ready for Committer: 3
In short we are in cruel need of reviewers to make things move on.

To reviewers:
- This commit fest may be a good occasion for you to study new parts
of the code, there are many simple patches in need of love.
- If you are marked as a reviewer for a given patch, but you know that
you won't have time to look at it, it is advised that yoy remove your
name to give room to someone else.
- Be sure to keep the information of the patch you are reviewing fresh
on the CF app. This needs at most 2 minutes.
- Feel free to ping the patch submitter if what you reviewed does not
get updated. People have their daily jobs and are busy, so try to not
be too pushy. A delay of one week may give a good base, but that's
holiday season in many places.

And to patch submitters:
- Please review one patch of equivalent difficulty with your own patch
to help keeping a balance between reviews each patch
- Be sure to keep the information about your patch up-to-date
- Feel free to ping your reviewer if you have no news and need more feedback

To committers: there are 2 patches marked as ready for committer from
previous commit fest:
- Foreign table inheritance
- Point to polygon distance operator

This is going to be a period of vacations for many people here, so
let's do our best.
Regards,
--
Michael

Michael Paquier <michael(dot)paquier(at)gmail(dot)com> writes:
> Let's begin the commit fest of December:
> https://commitfest.postgresql.org/action/commitfest_view?id=25
> ...
> To committers: there are 2 patches marked as ready for committer from
> previous commit fest:
> - Foreign table inheritance

I should probably take this one.

> - Point to polygon distance operator

I looked at that briefly during the last fest, but was unsure whether it
was too entangled with the GiST patches that Heikki was looking at.

regards, tom lane

On Mon, Dec 15, 2014 at 3:09 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Michael Paquier <michael(dot)paquier(at)gmail(dot)com> writes:
>> - Point to polygon distance operator
> I looked at that briefly during the last fest, but was unsure whether it
> was too entangled with the GiST patches that Heikki was looking at.
Recalling my memories of this morning, things are rather independent.
--
Michael

On 12/15/2014 08:37 AM, Michael Paquier wrote:
> On Mon, Dec 15, 2014 at 3:09 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> Michael Paquier <michael(dot)paquier(at)gmail(dot)com> writes:
>>> - Point to polygon distance operator
>> I looked at that briefly during the last fest, but was unsure whether it
>> was too entangled with the GiST patches that Heikki was looking at.
> Recalling my memories of this morning, things are rather independent.

Right. I also looked at it briefly, but I wasn't sure if we really want
it. AFAICT, no-one has actually asked for that operator, it was written
only to be an example of an operator that would benefit from the
knn-gist with recheck patch. If there is some other, real, use for the
knn-gist with recheck patch, then I'm OK with that, but otherwise it's
dubious to add an operator just so that it can then be made faster by
another patch. That said, it seems quite harmless, so might as well
commit it.

- Heikki

On Mon, Dec 15, 2014 at 4:12 PM, Heikki Linnakangas <hlinnakangas(at)vmware(dot)com
> wrote:
>
> On 12/15/2014 08:37 AM, Michael Paquier wrote:
>
>> On Mon, Dec 15, 2014 at 3:09 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>
>>> Michael Paquier <michael(dot)paquier(at)gmail(dot)com> writes:
>>>
>>>> - Point to polygon distance operator
>>>>
>>> I looked at that briefly during the last fest, but was unsure whether it
>>> was too entangled with the GiST patches that Heikki was looking at.
>>>
>> Recalling my memories of this morning, things are rather independent.
>>
>
> Right. I also looked at it briefly, but I wasn't sure if we really want
> it. AFAICT, no-one has actually asked for that operator, it was written
> only to be an example of an operator that would benefit from the knn-gist
> with recheck patch. If there is some other, real, use for the knn-gist with
> recheck patch, then I'm OK with that, but otherwise it's dubious to add an
> operator just so that it can then be made faster by another patch. That
> said, it seems quite harmless, so might as well commit it.

Lack of recheck is major limitation of KNN-GiST now. People are not asking
for that because they don't know what is needed to implement exact KNN for
PostGIS. Now they have to invent kluges like this:

WITH closest_candidates AS (
SELECT
streets.gid,
streets.name,
streets.geom
FROM
nyc_streets streets
ORDER BY
streets.geom <->
'SRID=26918;POINT(583571.905921312 4506714.34119218)'::geometry
LIMIT 100
)
SELECT gid, name
FROM closest_candidates
ORDER BY
ST_Distance(
geom,
'SRID=26918;POINT(583571.905921312 4506714.34119218)'::geometry
)
LIMIT 1;

See blog posts:
http://blog.light42.com/wordpress/?p=102
http://workshops.boundlessgeo.com/postgis-intro/knn.html

------
With best regards,
Alexander Korotkov.

On 12/15/2014 03:45 PM, Alexander Korotkov wrote:
> On Mon, Dec 15, 2014 at 4:12 PM, Heikki Linnakangas <hlinnakangas(at)vmware(dot)com
>> wrote:
>>
>> On 12/15/2014 08:37 AM, Michael Paquier wrote:
>>
>>> On Mon, Dec 15, 2014 at 3:09 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>>
>>>> Michael Paquier <michael(dot)paquier(at)gmail(dot)com> writes:
>>>>
>>>>> - Point to polygon distance operator
>>>>>
>>>> I looked at that briefly during the last fest, but was unsure whether it
>>>> was too entangled with the GiST patches that Heikki was looking at.
>>>>
>>> Recalling my memories of this morning, things are rather independent.
>>
>> Right. I also looked at it briefly, but I wasn't sure if we really want
>> it. AFAICT, no-one has actually asked for that operator, it was written
>> only to be an example of an operator that would benefit from the knn-gist
>> with recheck patch. If there is some other, real, use for the knn-gist with
>> recheck patch, then I'm OK with that, but otherwise it's dubious to add an
>> operator just so that it can then be made faster by another patch. That
>> said, it seems quite harmless, so might as well commit it.
>
> Lack of recheck is major limitation of KNN-GiST now. People are not asking
> for that because they don't know what is needed to implement exact KNN for
> PostGIS. Now they have to invent kluges like this:
>
> WITH closest_candidates AS (
> SELECT
> streets.gid,
> streets.name,
> streets.geom
> FROM
> nyc_streets streets
> ORDER BY
> streets.geom <->
> 'SRID=26918;POINT(583571.905921312 4506714.34119218)'::geometry
> LIMIT 100
> )
> SELECT gid, name
> FROM closest_candidates
> ORDER BY
> ST_Distance(
> geom,
> 'SRID=26918;POINT(583571.905921312 4506714.34119218)'::geometry
> )
> LIMIT 1;
>
> See blog posts:
> http://blog.light42.com/wordpress/?p=102
> http://workshops.boundlessgeo.com/postgis-intro/knn.html

Ugh. Ok, sold :-). I'll review...

- Heikki

Alexander Korotkov <aekorotkov(at)gmail(dot)com> writes:
> On Mon, Dec 15, 2014 at 4:12 PM, Heikki Linnakangas <hlinnakangas(at)vmware(dot)com
>> wrote:
>> Right. I also looked at it briefly, but I wasn't sure if we really want
>> it. AFAICT, no-one has actually asked for that operator, it was written
>> only to be an example of an operator that would benefit from the knn-gist
>> with recheck patch.

> Lack of recheck is major limitation of KNN-GiST now. People are not asking
> for that because they don't know what is needed to implement exact KNN for
> PostGIS. Now they have to invent kluges like this:
> [ query using ORDER BY ST_Distance ]

It's not apparent to me that the proposed operator is a replacement for
ST_Distance. The underlying data in an example like this won't be either
points or polygons, it'll be PostGIS datatypes.

In short, I believe that PostGIS could use what you're talking about,
but I agree with Heikki's objection that nobody has asked for this
particular operator.

regards, tom lane

On Mon, Dec 15, 2014 at 6:20 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>
> Alexander Korotkov <aekorotkov(at)gmail(dot)com> writes:
> > On Mon, Dec 15, 2014 at 4:12 PM, Heikki Linnakangas <
> hlinnakangas(at)vmware(dot)com
> >> wrote:
> >> Right. I also looked at it briefly, but I wasn't sure if we really want
> >> it. AFAICT, no-one has actually asked for that operator, it was written
> >> only to be an example of an operator that would benefit from the
> knn-gist
> >> with recheck patch.
>
> > Lack of recheck is major limitation of KNN-GiST now. People are not
> asking
> > for that because they don't know what is needed to implement exact KNN
> for
> > PostGIS. Now they have to invent kluges like this:
> > [ query using ORDER BY ST_Distance ]
>
> It's not apparent to me that the proposed operator is a replacement for
> ST_Distance. The underlying data in an example like this won't be either
> points or polygons, it'll be PostGIS datatypes.
>
> In short, I believe that PostGIS could use what you're talking about,
> but I agree with Heikki's objection that nobody has asked for this
> particular operator.
>

"polygon <-> point" is for sure not ST_Distance replacement. I was giving
this argument about KNN-GiST with recheck itself. "polygon <-> point" is
needed just as in-core example of KNN-GiST with recheck.

------
With best regards,
Alexander Korotkov.

On 12/15/2014 05:22 PM, Alexander Korotkov wrote:
> On Mon, Dec 15, 2014 at 6:20 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>
>> Alexander Korotkov <aekorotkov(at)gmail(dot)com> writes:
>>> On Mon, Dec 15, 2014 at 4:12 PM, Heikki Linnakangas <
>> hlinnakangas(at)vmware(dot)com
>>>> wrote:
>>>> Right. I also looked at it briefly, but I wasn't sure if we really want
>>>> it. AFAICT, no-one has actually asked for that operator, it was written
>>>> only to be an example of an operator that would benefit from the
>> knn-gist
>>>> with recheck patch.
>>
>>> Lack of recheck is major limitation of KNN-GiST now. People are not
>> asking
>>> for that because they don't know what is needed to implement exact KNN
>> for
>>> PostGIS. Now they have to invent kluges like this:
>>> [ query using ORDER BY ST_Distance ]
>>
>> It's not apparent to me that the proposed operator is a replacement for
>> ST_Distance. The underlying data in an example like this won't be either
>> points or polygons, it'll be PostGIS datatypes.
>>
>> In short, I believe that PostGIS could use what you're talking about,
>> but I agree with Heikki's objection that nobody has asked for this
>> particular operator.
>
> "polygon <-> point" is for sure not ST_Distance replacement. I was giving
> this argument about KNN-GiST with recheck itself. "polygon <-> point" is
> needed just as in-core example of KNN-GiST with recheck.

Right. I don't think point <-> polygon is too useful by itself, but we
need an example in core that could make use KNN-GiST recheck patch. We
can't write a regression test for it otherwise, for starters.

Actually, we probably could've used the circle <-> polygon for that just
as well...

- Heikki

On Mon, Dec 15, 2014 at 7:47 PM, Heikki Linnakangas <hlinnakangas(at)vmware(dot)com
> wrote:
>
> On 12/15/2014 05:22 PM, Alexander Korotkov wrote:
>
>> On Mon, Dec 15, 2014 at 6:20 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>
>>>
>>> Alexander Korotkov <aekorotkov(at)gmail(dot)com> writes:
>>>
>>>> On Mon, Dec 15, 2014 at 4:12 PM, Heikki Linnakangas <
>>>>
>>> hlinnakangas(at)vmware(dot)com
>>>
>>>> wrote:
>>>>> Right. I also looked at it briefly, but I wasn't sure if we really want
>>>>> it. AFAICT, no-one has actually asked for that operator, it was written
>>>>> only to be an example of an operator that would benefit from the
>>>>>
>>>> knn-gist
>>>
>>>> with recheck patch.
>>>>>
>>>>
>>> Lack of recheck is major limitation of KNN-GiST now. People are not
>>>>
>>> asking
>>>
>>>> for that because they don't know what is needed to implement exact KNN
>>>>
>>> for
>>>
>>>> PostGIS. Now they have to invent kluges like this:
>>>> [ query using ORDER BY ST_Distance ]
>>>>
>>>
>>> It's not apparent to me that the proposed operator is a replacement for
>>> ST_Distance. The underlying data in an example like this won't be either
>>> points or polygons, it'll be PostGIS datatypes.
>>>
>>> In short, I believe that PostGIS could use what you're talking about,
>>> but I agree with Heikki's objection that nobody has asked for this
>>> particular operator.
>>>
>>
>> "polygon <-> point" is for sure not ST_Distance replacement. I was giving
>> this argument about KNN-GiST with recheck itself. "polygon <-> point" is
>> needed just as in-core example of KNN-GiST with recheck.
>>
>
> Right. I don't think point <-> polygon is too useful by itself, but we
> need an example in core that could make use KNN-GiST recheck patch. We
> can't write a regression test for it otherwise, for starters.
>
> Actually, we probably could've used the circle <-> polygon for that just
> as well...

Did you mean searching for circles or polygons in the last sentence?

------
With best regards,
Alexander Korotkov.

On 12/15/2014 08:32 PM, Alexander Korotkov wrote:
> On Mon, Dec 15, 2014 at 7:47 PM, Heikki Linnakangas <hlinnakangas(at)vmware(dot)com
>> wrote:
>>
>> On 12/15/2014 05:22 PM, Alexander Korotkov wrote:
>>
>>> On Mon, Dec 15, 2014 at 6:20 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>>
>>>>
>>>> Alexander Korotkov <aekorotkov(at)gmail(dot)com> writes:
>>>>
>>>>> On Mon, Dec 15, 2014 at 4:12 PM, Heikki Linnakangas <
>>>>>
>>>> hlinnakangas(at)vmware(dot)com
>>>>
>>>>> wrote:
>>>>>> Right. I also looked at it briefly, but I wasn't sure if we really want
>>>>>> it. AFAICT, no-one has actually asked for that operator, it was written
>>>>>> only to be an example of an operator that would benefit from the
>>>>>>
>>>>> knn-gist
>>>>
>>>>> with recheck patch.
>>>>>>
>>>>>
>>>> Lack of recheck is major limitation of KNN-GiST now. People are not
>>>>>
>>>> asking
>>>>
>>>>> for that because they don't know what is needed to implement exact KNN
>>>>>
>>>> for
>>>>
>>>>> PostGIS. Now they have to invent kluges like this:
>>>>> [ query using ORDER BY ST_Distance ]
>>>>>
>>>>
>>>> It's not apparent to me that the proposed operator is a replacement for
>>>> ST_Distance. The underlying data in an example like this won't be either
>>>> points or polygons, it'll be PostGIS datatypes.
>>>>
>>>> In short, I believe that PostGIS could use what you're talking about,
>>>> but I agree with Heikki's objection that nobody has asked for this
>>>> particular operator.
>>>>
>>>
>>> "polygon <-> point" is for sure not ST_Distance replacement. I was giving
>>> this argument about KNN-GiST with recheck itself. "polygon <-> point" is
>>> needed just as in-core example of KNN-GiST with recheck.
>>>
>>
>> Right. I don't think point <-> polygon is too useful by itself, but we
>> need an example in core that could make use KNN-GiST recheck patch. We
>> can't write a regression test for it otherwise, for starters.
>>
>> Actually, we probably could've used the circle <-> polygon for that just
>> as well...
>
> Did you mean searching for circles or polygons in the last sentence?

circle as the key, on an index on a polygon column. A circle with radius
0 is equivalent to a point, after all.

I'm looking at the knn-gist with recheck patch now, and I see that it
actually adds two more operators: polygon <-> point (commutator of what
I just committed) and circle <-> point. We might want to split those off
into a separate patch too.

- Heikki

On 12/15/2014 02:37 AM, Michael Paquier wrote:
> Hi all,
>
> Let's begin the commit fest of December:
> https://commitfest.postgresql.org/action/commitfest_view?id=25

I have been reading this list for a long time and I have to say that I
thought about reviewing patches for a long time....but I am really
scared about few things....

1 - I have no knowledge at all about postgresql code so I can be doing
things incorrectly (saying that something is fine or not fine, but
making a mistake)

2 - English is not my main language...and I am not really good at
English, reading is one thing, but writing is not as easy for me so
maybe a review cannot be correctly understood

Besides that, how long can take a review, simple one of course,
considering that I starting vacations in 2 weeks.

I really hope I can start helping here but I am afraid that maybe I can
make things slower than today cause of my lack of source code..

Best regards

Rodrigo Gonzalez

Rodrigo Gonzalez wrote:

> I have been reading this list for a long time and I have to say that I
> thought about reviewing patches for a long time....but I am really
> scared about few things....
>
> 1 - I have no knowledge at all about postgresql code so I can be doing
> things incorrectly (saying that something is fine or not fine, but
> making a mistake)
>
> 2 - English is not my main language...and I am not really good at
> English, reading is one thing, but writing is not as easy for me so
> maybe a review cannot be correctly understood
>
> Besides that, how long can take a review, simple one of course,
> considering that I starting vacations in 2 weeks.
>
> I really hope I can start helping here but I am afraid that maybe I can
> make things slower than today cause of my lack of source code..

If you never start, you will never get enough experience to do
meaningful reviews. Don't worry about making mistakes. If you don't
catch a problem, somebody else might. If you catch a problem and report
it to the patch author, you save somebody else the time to report that
problem. Eventually, with experience, your reviews will be good enough
that they will be valuable on their own right. Also, if you get enough
experience, you might eventually be able to write your own patches (or
you will discover you're not for the programming thing in the first
place.)

We have patches to review all year long; just have a look at the
http://commitfest.postgresql.org site whenever you think you will have
time. If you return from vacations in 4-5 weeks, I'm pretty sure we
will have patches to review at that time, too.

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Mon, Dec 15, 2014 at 03:58:39PM +0200, Heikki Linnakangas wrote:
> >WITH closest_candidates AS (
> > SELECT
> > streets.gid,
> > streets.name,
> > streets.geom
> > FROM
> > nyc_streets streets
> > ORDER BY
> > streets.geom <->
> > 'SRID=26918;POINT(583571.905921312 4506714.34119218)'::geometry
> > LIMIT 100
> >)
> >SELECT gid, name
> >FROM closest_candidates
> >ORDER BY
> > ST_Distance(
> > geom,
> > 'SRID=26918;POINT(583571.905921312 4506714.34119218)'::geometry
> > )
> >LIMIT 1;
> >
> >See blog posts:
> >http://blog.light42.com/wordpress/?p=102
> >http://workshops.boundlessgeo.com/postgis-intro/knn.html
>
> Ugh. Ok, sold :-). I'll review...

Just to summarize, the index can only be created on the <-> operator
because that indexes on the center of the bounding box:

http://postgis.net/docs/manual-2.1/geometry_distance_centroid.html

You can't index on ST_Distance() because that computes the minimum
distance between the two objects, which is different for different
points:

http://postgis.net/docs/manual-2.1/ST_Distance.html

I am embarrassed by the LIMIT 100 hack they have to use above to find
the closest polygon to a given point, and this recheck patch is going to
fix that. I think this will remove the most significant PostGIS wart.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ Everyone has their own god. +