Thread: 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.paquier@gmail.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@sss.pgh.pa.us> wrote: > Michael Paquier <michael.paquier@gmail.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@sss.pgh.pa.us> wrote: >> Michael Paquier <michael.paquier@gmail.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@vmware.com> 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;
------
With best regards,
Alexander Korotkov.
On 12/15/2014 08:37 AM, Michael Paquier wrote:On Mon, Dec 15, 2014 at 3:09 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:Michael Paquier <michael.paquier@gmail.com> writes:Recalling my memories of this morning, things are rather independent.- Point to polygon distance operatorI 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.
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:
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@vmware.com >> wrote: >> >> On 12/15/2014 08:37 AM, Michael Paquier wrote: >> >>> On Mon, Dec 15, 2014 at 3:09 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: >>> >>>> Michael Paquier <michael.paquier@gmail.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@gmail.com> writes:
> On Mon, Dec 15, 2014 at 4:12 PM, Heikki Linnakangas <hlinnakangas@vmware.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@sss.pgh.pa.us> wrote:
------
With best regards,
Alexander Korotkov.
Alexander Korotkov <aekorotkov@gmail.com> writes:
> On Mon, Dec 15, 2014 at 4:12 PM, Heikki Linnakangas <hlinnakangas@vmware.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@sss.pgh.pa.us> wrote: >> >> Alexander Korotkov <aekorotkov@gmail.com> writes: >>> On Mon, Dec 15, 2014 at 4:12 PM, Heikki Linnakangas < >> hlinnakangas@vmware.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@vmware.com> wrote:
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.On 12/15/2014 05:22 PM, Alexander Korotkov wrote:On Mon, Dec 15, 2014 at 6:20 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Alexander Korotkov <aekorotkov@gmail.com> writes:On Mon, Dec 15, 2014 at 4:12 PM, Heikki Linnakangas <hlinnakangas@vmware.comknn-gistwrote:
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 thewith recheck patch.Lack of recheck is major limitation of KNN-GiST now. People are notaskingfor that because they don't know what is needed to implement exact KNNforPostGIS. 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.
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.
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@vmware.com >> wrote: >> >> On 12/15/2014 05:22 PM, Alexander Korotkov wrote: >> >>> On Mon, Dec 15, 2014 at 6:20 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: >>> >>>> >>>> Alexander Korotkov <aekorotkov@gmail.com> writes: >>>> >>>>> On Mon, Dec 15, 2014 at 4:12 PM, Heikki Linnakangas < >>>>> >>>> hlinnakangas@vmware.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@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. +